HI Ed,
Thanks for your explanation, very reasonable, I am ok with your change.
Thanks,
Zhijun
On 2/17/15, 6:17, Edward Burns wrote:
> Hello Zhijun,
>
> I'm sorry to have to make a modification to your commit in this way, but
> I think it is the best way to take advantage of this "teachable moment"
> [1] regarding JCP practices. Please accept my apologies if you already
> know all this.
>
>
> SECTION: Modified Files
> ----------------------------elp
> M jsf-api/src/main/java/javax/faces/flow/FlowScoped.java
>
> - Added text relating to JAVASERVERFACES_SPEC_PUBLIC-1361
>
> *<p class="changed_added_2_3">When replacing (rather than decorating)
> * the flow implementation with a custom {_at_link FlowHandler}
> * implementation, it is necessary to also replace the CDI extension
> * that implements the specified behavior regarding
> *<code>FlowScoped</code> beans.</p>
>
> M jsf-ri/src/main/java/com/sun/faces/flow/FlowHandlerImpl.java
> M jsf-ri/src/main/java/com/sun/faces/flow/FlowCDIContext.java
>
> - As indicated in my earlier changebundle, there already is a tight
> coupling between FlowHandlerImpl and FlowCDI context: FlowHandlerImpl
> call static methods flowEntered() and flowExited() on FlowCDIContext.
> I want to solve JAVASERVERFACES-3680 by simply extending that coupling
> to go the other way as well.
>
> You had solved 3680 by adding a method on FlowHandlerImpl and
> downcasting from within FlowCDIContext, as shown here:
>
> if ( flowHandler instanceof FlowHandlerImpl) {
> FlowHandlerImpl flowHandlerImpl = (FlowHandlerImpl)facesContext.getApplication().getFlowHandler();
>
> While I appreciate your usage of the instanceof test to avoid a
> ClassCastException, that approach will unnecessarily fail when the
> FlowHandler is decorated. It should not fail in that case.
>
> Consider this usage:
>
> public class MyFlowHandlerFactory extends FlowHandlerFactoryWrapper<FlowHandlerFactory> {
>
> private FlowHandlerFactory parent;
>
> public MyFlowHandlerFactory(FlowHandlerFactory parent) {
> this.parent = parent;
> }
>
> public FlowHandlerFactory getWrapped() {
> return parent;
> }
>
> }
>
> public class MyCustomFlowHandler extends FlowHandler {
>
> private FlowHandler parent;
>
> public MyCustomFlowHandler(FlowHandler parent) {
> this.parent = parent;
> }
>
> public Map<Object, Object> getCurrentFlowScope() {
> return parent.getCurrentFlowScope();
> }
>
> // ... similar for other methods. Just call return parent.method().
>
> }
>
> This approach would fail with your code, even though it really should
> pass because the FlowHandler implementation is indeed just doing the
> same thing as FlowHandlerImpl but is not an instance of
> FlowHandlerImpl. My approach, calling the public static method on
> FlowHandlerImpl, would be ok.
>
> Now, all of this hinges upon this comment, which I added in
> JAVASERVERFACES_SPEC_PUBLIC-1361:
>
> Also add to the spec that when replacing (rather than decorating)
> the FlowHandler, one must also replace the @FlowScoped CDI
> context/extension. The technique of doing this replacement is still
> an open question.
>
> and which I included in the spec for FlowScoped above.
>
> Please accept this modification to your commit. I think it's OK,
> unless I'm missing something.
>
>
> [1] http://en.wikipedia.org/wiki/Teachable_moment