bmhm commented on issue #58: [SHIRO-337] basic cdi support URL: https://github.com/apache/shiro/pull/58#issuecomment-577317147 Hi, I'm not a ASF committer, but from looking at this code, I have some questions. `SubjectProducer` is `@ApplicationScoped` and there is commented `@RequestScoped`. Why not just use `@Dependent`? Line 40 of this class is `Subject.class.cast(Proxy.newProxyInstance(` and could be changed to `(Subject) Proxy.newProxyInstance(`… This part needs some additional comments why it uses a new proxy instance. The comment should mention Shiro's ThreadContext. Why does it need a Classloader utility `Loader`? `ShiroExension::addSecurityManagerIfNeeded` could be updated to use a `AtomicReference::updateAndGet` The geronimo specs are still in there, please update to the jakarta dependencies. Geronimo's dependencies do not even have javadoc available. `beans.xml` could be updated to cdi2. I'd also rename the module to CDI2, just in case CDI 3 is not compatible. Also, you should not leave out `beans.xml` although it is allowed. It is still good habit to ship an empty one, just because some containers allow a setting to scan only jar files for beans which do have such a file (which speeds up deployment times massively). Also, some comments might be helpful which action in the extension is done for which purpose. I haven't used extensions a lot, but I think this code should still be elegible for inclusion. If merged, I'd also like to contribute another integration test using the maven invoker plugin, starting an OpenLiberty instance.
---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: [email protected] With regards, Apache Git Services
