Ed Burns wrote:
>> It turns out that implementing this check is not worth the complexity it
>> bring to the implementation. My time is better spent in trying to bring
>> your new API into the spec.
That is fine, however I believe that you underestimate the trickiness.
There is also the issue that this will be yet another new contract that
component authors will have to implement. This aspect is a little sad,
since this would be yet another minor version of JSF that is not
backwards compatible for component authors.
I don't think that the issue is so much the complexity of the check--the
check is simple. Rather, I believe that the real problem is the amount
of potentially unsafe EL out there. The EL example you provided is a
good example. It is unclear to me how you would plan to support EL that
traverses the component tree (as in your example). Personally, I would
consider such a non-goal, due to the significant complexity that it adds.
-- Blake Sullivan
On 3/19/12 6:56 PM, Edward Burns wrote:
>>>>>> On Fri, 16 Mar 2012 17:10:57 -0700, Blake Sullivan<blake.sullivan_at_oracle.com> said:
> B> On 3/16/12 7:34 AM, Edward Burns wrote:
> KM> So, I suppose the question is really whether or not we should support
> KM> the ability to use findComponent() and evaluate the properties outside
> KM> of a tree traversal.
> B> We have never been able to support this in the general case, which is
> B> why invokeOnComponent() was invented. We should improve the
> B> documentation and should maybe throw an IllegalStateException when
> B> attempting to evaluate EL attributes on a non-current component during
> B> the development project stage.
> EB> Ok, I've made the following change to the documentation to
> EB> UIComponent.findComponent():
> EB>
> EB> Make the second paragraph of the javadoc for that method be:
> EB> This method is not intended to be used with components that reside
> EB> inside of an iterating component. To take action on a component inside
> EB> of an iteration, or to find a component given a simple clientId, see
> EB> invokeOnComponent.
> B> It's actually more basic than that--you can't perform any operation on a
> B> component returned by findComponent() that depends directly or
> B> indirectly on the component being in the correct traversal context. In
> B> practice, this means that attempting to retrieve an EL-bound attribute
> B> value is not safe. Iteration is only the most obvious of these
> B> problems, and the one that exists in the spec components.
>
>>> Here is the revised text.
>>>
>>> + *<p class="changed_added_2_2">WARNING: The found
>>> + *<code>UIComponent</code> instance, if any, is returned
>>> + *<strong>without</strong> regard for its tree traversal context.
>>> + * Retrieving an EL-bound attribute from the component is not safe.
>>> + * EL expressions can contain implicit objects, such as
>>> + *<code>#{component}</code>, which assume they are being evaluated
>>> + * within the scope of a tree traversal context. Evaluating
>>> + * expressions with these kinds of implicit objects outside of a
>>> + * tree traversal context produces undefined results. See {_at_link
>>> + * #invokeOnComponent} for a method that<strong>does</strong>
>>> + * correctly account for the tree traversal context when operating
>>> + * on the found<code>UIComponent</code> instance. {_at_link #invokeOnComponent}
>>> + * is also useful to find components given a simple<code>clientId</code>.
>>>
>>>
> EB> Now, to comply with your recommendation to throw an
> EB> IllegalStateException, let's look at section "5.6.2 ELResolver for
> EB> Facelets and Programmatic Access". In there, you see a diagram showing
> EB> our ELResolver chain. Blake, would the following comply with your
> EB> recommendation?
> EB> Modify the table in section 5.6.2.1 so that for all ELResolver methods,
> EB> if ProjectStage is development, and base is a UIComponent, throw an
> EB> IllegalStateException if base is not equal to the component returned
> EB> from UIComponent.getCurrentComponent()?
> B> Yes. And this should also ensure that implementation code that needs to
> B> mess with the current component, is doing so correctly.
>
> It turns out that implementing this check is not worth the complexity it
> bring to the implementation. My time is better spent in trying to bring
> your new API into the spec.
>
> B> You need new component apis to manage the pushing and popping of the El
> B> context as the components execute. Currently, with the JSF spec, if
> B> application code executes while in an EL-context, that application code
> B> is playing with fire if it then makes a call such as invokeOnComponent
> B> or visitTree that further modifies the EL-context. This is because the
> B> new modifications will be layered on top of the current modifications.
> B> This is incorrect, we really want to pop the current context, assert the
> B> new context, invoke the requisite code and then pop the new context and
> B> reapply the old. If we can do the above more efficiently by only
> B> applying the pops and pushes back to the lowest shared ancestor, all the
> B> better. The Trinidad ComponentContextManager
> B> <https://myfaces.apache.org/trinidad/trinidad-api/apidocs/org/apache/myfaces/trinidad/context/ComponentContextManager.html>
> B> does this.
>
> I'll see how hard this starts to look after trying to implement it.
>
> Ed
>