-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Fixing OpenWebBeans test on Jetty 10.0.x #4706
Conversation
...t-owb-cdi-webapp/src/main/java/org/eclipse/jetty/cdi/owb/OwbServletContainerInitializer.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CDI ... sigh
Are you sure the SCI is needed? If so, why? Why doesn't using their org.apache.webbeans.servlet.WebBeansConfigurationListener work instead? |
tests/test-webapps/test-owb-cdi-webapp/src/main/webapp/WEB-INF/jetty-env.xml
Outdated
Show resolved
Hide resolved
@janbartel @lorban please re-review. |
For those wanting to know more about the CDI integrations, it is worthwhile reading #3804 and the PRs and other issues linked from it. The CDI integrations in both Weld and OWB were essentially calling internal interfaces to get their decorators registered. I created some more generic ways of integrating CDI... unfortunately OWB and Weld couldn't agree on the best way, so we ended up with two new integrations, plus some legacy support in 9.4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the title is now wrong? Not restoring SCI, but fixing its removal?
@gregw updated the title, and cleaned up the fix a bit too (using init-param, not system property now) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good, but perhaps a comment in the XML would help keep this clear how it works
tests/test-webapps/test-owb-cdi-webapp/src/main/webapp/WEB-INF/jetty-env.xml
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assuming we can ever get a test run of this that works, this LGTM.
+ OWB needs init-parameter to allow it's own ServletContainerInitializer found at org.apache.webbeans.servlet.WebBeansConfigurationListener$Auto to execute and properly add the required Listener for OWB to function. + See OWB-1296 Signed-off-by: Joakim Erdfelt <[email protected]>
df2eb86
to
f3bd661
Compare
Signed-off-by: Joakim Erdfelt [email protected]