Skip to content
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

Remove UIComponent.bindings field #1725

Open
arjantijms opened this issue Nov 15, 2022 · 9 comments · Fixed by eclipse-ee4j/mojarra#5174
Open

Remove UIComponent.bindings field #1725

arjantijms opened this issue Nov 15, 2022 · 9 comments · Fixed by eclipse-ee4j/mojarra#5174
Assignees
Labels
bug Something isn't working mojarra-implemented API issue implemented by Mojarra
Milestone

Comments

@arjantijms
Copy link
Contributor

We accidentally left in UIComponent.bindings field, even though it should have been removed.

See:

https://github.com/eclipse-ee4j/mojarra/blob/master/impl/src/main/java/jakarta/faces/component/UIComponent.java#L2385

With high priority we should remove this field.

@edburns
Copy link

edburns commented Sep 19, 2023

Discussed at 2023-09-19 Platform Project meeting:
These three would have been removed in Faces 4.1. The recently approved deprecation process does indeed cover this kind of case.
Emily observed

  • The spec committee needs to grant this exception.
  • Will bring this up at the next spec committee meeting, with the understanding this is pre-approved.
  • It is a wasted effort to do 4.1 to accommodate these changes.

@BalusC BalusC added mojarra-implemented API issue implemented by Mojarra myfaces-implemented API issue implemented by MyFaces labels Oct 5, 2023
@volosied volosied modified the milestones: 4.1, 5.0 Feb 23, 2024
@volosied
Copy link
Contributor

Changed this to the 5.0 milestone instead as we can't remove it in a minor release.

@volosied volosied reopened this Feb 23, 2024
@arjantijms
Copy link
Contributor Author

arjantijms commented Feb 24, 2024

we can't remove it in a minor release.

I think there are no real rules in Jakarta EE or the specification process that forbid this. If we want we can delete this.

@pnicolucci
Copy link
Contributor

we can't remove it in a minor release.

I think there are no real rules in Jakarta EE or the specification process that forbid this. If we want we can delete this.

Hey @arjantijms, this document covers versioning for Jakarta EE: https://jakarta.ee/committees/specification/versioning/

API changes must conform to the following convention:

Major version increments: Backwards incompatible changes
Minor version increments: Additions to existing interfaces, and additions of new interfaces.
Patch version increments: Bug fixes

Since we're removing something in the API to me it falls into the "Major version increments" section.

@arjantijms
Copy link
Contributor Author

My interpretation was slightly different

Backwards incompatible changes are allowed, but their introduction should be done in a controlled manner. All specifications should aim to use a versioning strategy conforming to the following pattern: Major.Minor.Patch

Emphasis mine. That is different from must.

For the cases where backwards incompatible changes are being made, we follow a deprecation process that broadly has two steps:
A feature is marked as deprecated in release N.
A feature is then removed in release N+1 (or beyond)

I felt like this is the only thing required. Deprecate first, then remove in next release. Where the next release has to be part of a Jakarta EE release. E.g. with Faces 4.1 in EE 11, we can't deprecate in Faces 4.2, then remove a week later in Faces 4.3, etc etc and then have Faces 4.6 as part of EE 12.

@Emily-Jiang
Copy link
Contributor

You can introduce the backward compatibility but this will lead to a major release. If you want to remove this protected field, you can do a 5.0 release not 4.1 release.

@BalusC
Copy link
Member

BalusC commented Mar 2, 2024

It's indeed a pita but it's what it is. It has been reverted from 4.1 now.

@BalusC BalusC removed the mojarra-implemented API issue implemented by Mojarra label Mar 2, 2024
@pnicolucci
Copy link
Contributor

We look to be good for Faces 4.1 for MyFaces: https://github.com/apache/myfaces/blob/4.1.x/api/src/main/java/jakarta/faces/component/UIComponent.java#L145

@tandraschko @volosied I'm removing the myfaces-implemented tag from here as well so we remember to revisit for Faces.next

@tandraschko
Copy link

created eclipse-ee4j/mojarra#5514 as seems that you still didnt remove PropertyKeys.binding?

we need to check it because it seems set/getValueExpression stores its stuff there

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working mojarra-implemented API issue implemented by Mojarra
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants