search

eclipse-ee4j / mojarra

Mojarra, a Jakarta Faces implementation
Other
159 stars 109 forks source link

FacesConfig#version() cannot return null #5210

Closed BalusC closed 1 year ago

BalusC commented 1 year ago

https://github.com/jakartaee/faces/issues/1784

BalusC commented 1 year ago

@arjantijms and @tandraschko WDYT? Wondering between returning JSF_2_3 or adding something like NULL or LATEST as this isn't intended to be ever used anywhere and would be the first thing ever in Faces or perhaps even EE which is already deprecated at moment of addition :roll_eyes:

tandraschko commented 1 year ago

then lets return JSF_2_3 for now, we will remove it in 4.1 anyway?

BalusC commented 1 year ago

Right.

The only issue is that it's an API change and hence this PR targeted 4.1 instead of 4.0. But indeed we're going to remove it nonetheless. I'm only wondering what to do with 4.0. Leave it broken in this regard?

tandraschko commented 1 year ago

Cant we just add it to 4.0?

BalusC commented 1 year ago

I'm all for it because it's clearly a bug, but technically speaking, it's still an API change and these should go in a minor/major version.

@arjantijms can't we make an exception for this one?

tandraschko commented 1 year ago

for me its ok and already commited to MF as long as we dont change method signatures, its not a problem IMO

BalusC commented 1 year ago

Fair point. Ok, so be it.

arjantijms commented 1 year ago

I gave this some extra thought, and I now think it's an implementation change/bug fix, and not an API one.

The body of the version() method is implementation. By definition, it should return whatever the main annotation has as a default value:

See https://github.com/eclipse-ee4j/mojarra/blob/8b5e83b8016a4130b08ea22005ce4966c732e119/impl/src/main/java/jakarta/faces/annotation/FacesConfig.java#L94

BalusC commented 1 year ago

Bon. Let's approve it then.