Add support for add-list-attribute to render expressions

[brettryan]

When using expressions using the following example:

<put-list-attribute name="breadcrumbs" cascade="true" inherit="true">
  <add-list-attribute>
    <add-attribute expression="${customer.id}"/>
    <add-attribute expression="/customers/${customer.id}/info"/>
  </add-list-attribute>
  <add-list-attribute>
    <add-attribute value="Orders"/>
    <add-attribute expression="/customers/${customer.id}/orders"/>
  </add-list-attribute>
</put-list-attribute>

We first encounter an NPE when attempting to access this object. This is due to Attribute#toString returning the value null which with the container attempting to turn this into a binary array cases an NPE. This has been resolved by returning the string "null".

For the list attributes I have chosen to recursively process any lists encountered within [AbstractAttributeEvaluator#evaluate](https://tiles.apache.org/framework/apidocs/org/apache/tiles/evaluator/AbstractAttributeEvaluator.html#evaluate(org.apache.tiles.Attribute,%20org.apache.tiles.request.Request) to clone each attribute to a new instance with the value set to the evaluated expression.

NOTE: This solution comes with one caveat that is semi-intentional and you may choose to reject the pull request on this point. Previously a ListAttribute would be exposed to the view requiring consumers to make references to the attributes value property before accessing the index to the list.

[michaelsembwever]

can you take commit be97c75 out of the pull request please.
pull requests need to be atomic to just the required change at hand.
(extra stuff, like code formatting and such, is welcome in a separate PR whom's goal is explicitly such)

[brettryan]

Will do. You can cherry-pick commit brettryan@41759e8 if it's easier, I think I'll have to create a separate pull request though Github doesn't give me the option when I specify a commit range. I'll see what I can' see and let you know.

[michaelsembwever]

Will do. You can cherry-pick commit brettryan/tiles@41759e8 if it's easier, I think I'll have to create a separate pull request though Github doesn't give me the option when I specify a commit range. I'll see what I can' see and let you know.

I'm reviewing just the individual commits for now, so i'm not waiting on you to pull out that first commit. But it'll need to be done before we accept+push any final patch.

I agree that github isn't the best for such things. I find it's easier to keep github branches as atomic as possible. I also don't have any problem with your third commit here being folded into the second, just include the NPE as well in the commit msg. (At the end of the day we'll be accepting and pushing just one patch so to work with just one commit message makes sense already).

[brettryan]

No prob, thanks. I've never had to rebase before, just trying to squash my commit messages and not having the best of luck, but, I'm tired and unwell :)

I'll see what I can do throughout the week.

[michaelsembwever]

No prob, thanks. I've never had to rebase before, just trying to squash my commit messages and not having the best of luck, but, I'm tired and unwell :)

Take it easy and no hurry.
For my own part i've just started a new job and am in the middle of an international move and the process of selling two houses, so i'm really struggling to give you the within-24hr responses that you deserve. But with a little patience I hope we'll get this across the line, it's an improvement that does make sense i believe.

[brettryan]

On 12 Jul 2015, at 22:56, mck [email protected] wrote:

No prob, thanks. I've never had to rebase before, just trying to squash my commit messages and not having the best of luck, but, I'm tired and unwell :)

Take it easy and no hurry.
For my own part i've just started a new job and am in the middle of an international move and the process of selling two houses, so i'm really struggling to give you the within-24hr responses that you deserve.

Hell, don't worry about me, I understand how it is. We all offer what little time we have across many projects.
But with a little patience I hope we'll get this across the line, it's an improvement that does make sense i believe.

No rush, I'm using a custom build for my purpose at the moment.

[nlebas]

Well, I think we should at least have a test case for that. See this commit cc11955 as an example. The tests can be run with mvn -Prun-selenium clean verify (sorry, we still depend on a servlet container)

As for the incompatibility... well it seems a bit larger than exposed here. If people are using insert-attribute like our test case, this change would break their code. Moreover, it may break the possibility of lists of cascaded definitions like:

<put-list-attribute name="body" >
  <add-attribute value="blog.post" type="definition"/>
  <add-attribute value="blog.comments" type="definition"/>
</put-list-attribute>

Basically this change may drop support for any list attribute that is not a list of strings.

That being said, I like the idea of a special treatment for lists of strings, that would map them directly to List available in EL. Good job!

[nlebas]

It appears there is no regression after all. The hack of cloning the Attribute does it, although it probably slows things down slightly.

Could you please add the test case and I'll +1 this.

[michaelsembwever]

@ brettryan have you had any luck with writing a testcase?
speak up if you need help with it.

[brettryan]

Sorry, been out of action, wife's in hospital. I'll try get onto it when I can but it won't be this week unfortunately.

On 3 Aug 2015, at 18:41, mck [email protected] wrote:

@ brettryan have you had any luck with writing a testcase?
speak up if you need help with it.

—
Reply to this email directly or view it on GitHub.