New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DefaultResourceHandler conflicts with PrimeFaces resource handler #97

Closed
DonatasC opened this Issue Feb 11, 2015 · 5 comments

Comments

Projects
None yet
2 participants
@DonatasC

DonatasC commented Feb 11, 2015

My application (Omnifaces2.0+PrimeFaces5.1+PrimeFacesExtensions3.0) gets the following ResourceHandler chain during system startup:

GraphicResourceHandler -> PrimeFacesExtensionsResourceHandler -> PrimeResourceHandler

Issue - Primefaces version number is no longer appended to PrimeFaces resources; i.e. URL's used to be:

/javax.faces.resource/primefaces.js.html?ln=primefaces&v=5.1

now they are:

/javax.faces.resource/primefaces.js.html?ln=primefaces

Reason:

PrimeFacesExtensionsResourceHandler has overridden only 3 argument version of createResource(); PrimeResourceHandler has overriden ONLY 2 argument version of createResource().
JSF calls createResource with 2 arguments, your DefaultResourceHandler current implementation calls wrapped.createResource() with 3 arguments (!!!), but PrimeResourceHandler appends version number ONLY on 2 argument call.

Main issue: if JSF calls 2 argument version of createResource(), only 2 argument version must be called by all wrappers in the chain. DefaultResourceHandler breaks this (in case of Primefaces resources calls 3 argument version).

@BalusC

This comment has been minimized.

Show comment
Hide comment
@BalusC

BalusC Feb 11, 2015

Member

Originally, in previous OmniFaces versions, the combined, CDN and unmapped resource handler implementations had only 2-argument createResource() implemented. This caused trouble with other component libraries as reported in a.o. https://code.google.com/p/omnifaces/issues/detail?id=153

The JSF spec is unfortunately not clear in this, but technically, the PrimeFaces resource handlers are broken. They should also have implemented all 3 createResource() methods.

The problem here is indeed that the DefaultResourceHandler only looks at the available createResource() method of the directly wrapped resource handler. That directly wrapped resource handler is in turn responsible for calling the right createResource() method of its wrapped one.

In an ideal world, everyone should be overriding all 3 createResource() methods, or be using DefaultResourceHandler's approach.

In any case, technically you're right, it only unfortunately won't result in safe and DRY code. I'll brainstorm about fixing this peculiar problem from OmniFaces side on.

Member

BalusC commented Feb 11, 2015

Originally, in previous OmniFaces versions, the combined, CDN and unmapped resource handler implementations had only 2-argument createResource() implemented. This caused trouble with other component libraries as reported in a.o. https://code.google.com/p/omnifaces/issues/detail?id=153

The JSF spec is unfortunately not clear in this, but technically, the PrimeFaces resource handlers are broken. They should also have implemented all 3 createResource() methods.

The problem here is indeed that the DefaultResourceHandler only looks at the available createResource() method of the directly wrapped resource handler. That directly wrapped resource handler is in turn responsible for calling the right createResource() method of its wrapped one.

In an ideal world, everyone should be overriding all 3 createResource() methods, or be using DefaultResourceHandler's approach.

In any case, technically you're right, it only unfortunately won't result in safe and DRY code. I'll brainstorm about fixing this peculiar problem from OmniFaces side on.

@BalusC BalusC closed this in 5d17f94 Feb 11, 2015

@BalusC

This comment has been minimized.

Show comment
Hide comment
@BalusC

BalusC Feb 11, 2015

Member

I completely reworked the DefaultResourceHandler.

It's available in today's 2.1 SNAPSHOT. Can you give it a try and let me know?

Member

BalusC commented Feb 11, 2015

I completely reworked the DefaultResourceHandler.

It's available in today's 2.1 SNAPSHOT. Can you give it a try and let me know?

@DonatasC

This comment has been minimized.

Show comment
Hide comment
@DonatasC

DonatasC Feb 12, 2015

Yes, 2.1-SNAPSHOT is working correctly, PrimeFaces version number is appended again.
Thank you very much for fast fix!

DonatasC commented Feb 12, 2015

Yes, 2.1-SNAPSHOT is working correctly, PrimeFaces version number is appended again.
Thank you very much for fast fix!

@DonatasC

This comment has been minimized.

Show comment
Hide comment
@DonatasC

DonatasC Feb 12, 2015

DefaultResourceHandler source code is very elegant now :) and is correct regarding argument number :)

DonatasC commented Feb 12, 2015

DefaultResourceHandler source code is very elegant now :) and is correct regarding argument number :)

@BalusC

This comment has been minimized.

Show comment
Hide comment
@BalusC

BalusC Feb 12, 2015

Member

Great! Thank you for reporting.

Member

BalusC commented Feb 12, 2015

Great! Thank you for reporting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment