jakartaee / faces

Jakarta Faces
Other
100 stars 55 forks source link

Remove class scanning and rely on CDI only? #1584

Open tandraschko opened 2 years ago

tandraschko commented 2 years ago

AFAIK both mojarra and myfaces has some scanning code to look for @FacesConverter and so on

In MyFaces we added a mechanism in the past, to skip this scanning and use a CDI Extension, which collects this infos: https://github.com/apache/myfaces/blob/master/impl/src/main/java/org/apache/myfaces/config/annotation/CdiAnnotationProviderExtension.java

should we say that Faces only looks for @FacesConverter via CDI?

BalusC commented 2 years ago

+1

This indeed still needs to be crystallized. For the record this is the complete set of candiates currently still manually scanned by Mojarra:

tandraschko commented 2 years ago

@arjantijms @BalusC i moved this to a new milestone now and will also move all existing issues we can rename 4.1 to 5.0 later if we really want to

i would also like to remove the 4.0 label as we should work with milestones

arjantijms commented 2 years ago

Thanks, it's perhaps a pity we haven't done this for 4.0, but so be it. Would still be very good to do for the next version though.

volosied commented 11 months ago

I'm not following the change. Could you elaborate? Isn't FacesConverter.class one of the annotations scanned by the CDI extension already? How else should it be looked for? Or are you wanting to update the spec to match the existing behavior?

tandraschko commented 11 months ago

Nope it isnt. Just check the specs and MyFaces: per default we manually scan the whole classpath but we added a context param to enable a CDI AnnotationProvider. This should be the only impl now as we fully rely on CDI.

volosied commented 11 months ago

Ah, I see what you mean. We have the following property: USE_CDI_FOR_ANNOTATION_SCANNING.

tandraschko commented 9 months ago

are we ok with this new and breaking behavior? @BalusC @arjantijms we already have a PR in MF for it

tandraschko commented 8 months ago

@BalusC @arjantijms are you ok with it or move to 5.0?

BalusC commented 8 months ago

Let's move to 5.0. This isn't just a 'maintenance' change, this will require spec changes as well.

We should also take the opportunity to drop managed=true attribute. And for Mojarra, the impl really needs to be fixed to not anymore wrap them in a CDI delegate as that doesn't at all work for converter/validator attributes: https://github.com/eclipse-ee4j/mojarra/issues/4814

tandraschko commented 8 months ago

Yeah +1 Can you make a issue for the managed=true?

hantsy commented 2 weeks ago

If the scan is moved to CDI, how about the Spring users?

tandraschko commented 2 weeks ago

i think in case of both implementaionts, we just switch the default way. both have a SPI for it.