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

Bump CDI version to 4.1 and place EL integration API in a new supplemental artifact #644

Merged
merged 5 commits into from May 24, 2023

Conversation

Ladicek
Copy link
Contributor

@Ladicek Ladicek commented Jan 5, 2023

Fixes #613

The commits are best reviewed in isolation, and perhaps should be split into multiple PRs. Opening this one as a draft for discussion.

@manovotn
Copy link
Contributor

manovotn commented Jan 5, 2023

Fix remnants of BeanManager.fireEvent() removal

@Ladicek your changes are not related to that. You instead modified text with BeanManager.getEvent() that method was only moved into BeanContainer. You could just alter the wording because all of the behavior stayed the same (such as rethrowing the exception).

@Ladicek
Copy link
Contributor Author

Ladicek commented Jan 5, 2023

Fix remnants of BeanManager.fireEvent() removal

@Ladicek your changes are not related to that. You instead modified text with BeanManager.getEvent() that method was only moved into BeanContainer. You could just alter the wording because all of the behavior stayed the same (such as rethrowing the exception).

Not really. The references to BM.fireEvent() were just replaced with references to BM.getEvent(), which doesn't make sense, because getEvent() doesn't fire an event -- you have to call Event.fire() on that, and the spec text already covers Event.fire(). The references to BM.getEvent() can and should be just removed, in my opinion.

@manovotn
Copy link
Contributor

manovotn commented Jan 5, 2023

Not really. The references to BM.fireEvent() were just replaced with references to BM.getEvent(), which doesn't make sense, because getEvent() doesn't fire an event

Fair enough. I didn't recall we just replaced that.

@Ladicek
Copy link
Contributor Author

Ladicek commented Jan 6, 2023

Not really. The references to BM.fireEvent() were just replaced with references to BM.getEvent(), which doesn't make sense, because getEvent() doesn't fire an event

Fair enough. I didn't recall we just replaced that.

See febd958 :-)

@Ladicek Ladicek changed the title Bump CDI version to 5.0 and move EL integration API to a new supplemental artifact Bump CDI version to 4.1 and place EL integration API in a new supplemental artifact May 24, 2023
@Ladicek
Copy link
Contributor Author

Ladicek commented May 24, 2023

Rebased and updated per recent discussion: this PR now bumps to 4.1, deprecates the 2 EL-related methods in BeanManager (for removal in CDI 5.0), and places the EL integration API (ELAwareBeanManager) in a new supplemental artifact.

@Ladicek Ladicek marked this pull request as ready for review May 24, 2023 10:56
…tal API artifact

The existing EL integration API in `BeanManager` is deprecated for removal.
Copy link
Contributor

@manovotn manovotn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for looking into this @Ladicek!

@manovotn
Copy link
Contributor

@starksm64 if you are fine with the changes, feel free to pull the trigger on this PR.

@Ladicek Ladicek added this to the CDI 4.1 milestone May 24, 2023
@starksm64 starksm64 merged commit 3c95b0f into jakartaee:master May 24, 2023
3 checks passed
@Ladicek Ladicek deleted the major-version-el-split branch July 14, 2023 11:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move jakarta.el-api related method to new BeanManager subclass?
3 participants