the point on INonContextualManager was to internalize NonContextual - in case cdi implementation provides a better way to perform non-contextual injection.
now that NonContextualManager is @ApplicationScoped and CdiConfiguration does not have a setter for it this functionality is lost. and even if cdi configuration had a setter, there is no longer a guarantee that someone did not inject one and use it before a new one was set on the configuration. that is why these things were not managed by cdi in the initial code... -igor On Mon, Nov 11, 2013 at 11:16 AM, John Sarman <[email protected]> wrote: > Also Noncontextual.of is never used anywhere except in the Wicket CDI api, > so I disagree. It solely used by the NonContextualManager which is > ApplicationScoped and the BeanManager is injected. The CdiCleanup also > uses it and is passed the BeanManager during configuration, both cases do > not make it unnice since it is an internal api. > > > On Mon, Nov 11, 2013 at 2:11 PM, John Sarman <[email protected]> wrote: > >> Actually I just changed them back to the way Igor originally implemented >> it. >> >> >> On Mon, Nov 11, 2013 at 2:10 PM, Emond Papegaaij < >> [email protected]> wrote: >> >>> Hi John, >>> >>> There is nothing wrong with looking up the BeanManager in a static >>> context. >>> It's just that the current implementation of >>> CDI.current().getBeanManager() >>> is broken in Weld. I expect this to be fixed soon. The workaround moves >>> the >>> lookup to a single place and tries a portable JNDI lookup first. This will >>> work in all EE containers. More importantly, the user doesn't notice the >>> workaround at all. >>> >>> Your commit changes the signature of NonContextual.of. You are now >>> required >>> to pass in the BeanManager, which is not very nice, because NonContextual >>> is to do CDI injection in places where CDI can't do it for you. In these >>> places, you often don't have a BeanManager available. I think the >>> workaround should stay until the bug is fixed in Weld. >>> >>> Best regards, >>> Emond >>> >>> >>> On Mon, Nov 11, 2013 at 7:45 PM, John Sarman <[email protected]> >>> wrote: >>> >>> > Edmond, >>> > I updated the cdi code to not ever use the CDI.current(). I reworked >>> your >>> > test earlier to just inject the BeanManager and that properly allows the >>> > multiple wars to see the InjectableTargets from a shared lib. I could >>> > only conclude that CDI.current should not be called from a static >>> method, >>> > so instead of joining the weld team or submitting an issue, I just >>> removed >>> > that possibility from the code. That latest code is in >>> > https://github.com/jsarman/wicket master branch. This also removed the >>> > need >>> > for the BeanManagerLookup workaround. >>> > >>> > >>> > >>> > >>> > On Mon, Nov 11, 2013 at 8:49 AM, John Sarman <[email protected]> >>> wrote: >>> > >>> > > Nevermind, I should not write emails this early, without an unsend. >>> > > Servlet A should see same BeanManager as Servlet B, if both servlets >>> are >>> > > deployed from same war file. That is ApplicationScoped. >>> > > >>> > > >>> > > On Mon, Nov 11, 2013 at 8:47 AM, John Sarman <[email protected]> >>> > wrote: >>> > > >>> > >> Ok, I read through your test code, so if you are saying that servlet >>> a >>> > >> gets same beanmanager as servlet b then yeah thats bad. >>> > >> >>> > >> >>> > >> On Mon, Nov 11, 2013 at 8:44 AM, John Sarman <[email protected] >>> > >wrote: >>> > >> >>> > >>> I was looking at your bug, but in the case you specified where the >>> > >>> cached BeanManager is passed, seems to be the correct behavior >>> because >>> > the >>> > >>> CdiConfiguration is ApplicationScoped. The CDI class states this: >>> > >>> /** >>> > >>> * Get the CDI BeanManager for the current context >>> > >>> * >>> > >>> * @return >>> > >>> */ >>> > >>> public abstract BeanManager getBeanManager(); >>> > >>> >>> > >>> So the cached BeanManager passed back is because it is accessed in >>> an >>> > >>> ApplicationScoped class. ApplicationScoped != Wickets Application >>> > scope. >>> > >>> >>> > >>> >>> > >>> >>> > >>> >>> > >>> >>> > >>> On Mon, Nov 11, 2013 at 8:26 AM, Emond Papegaaij < >>> > >>> [email protected]> wrote: >>> > >>> >>> > >>>> You are right, InitialContext.lookup was returning null. I've >>> fixed it >>> > >>>> by falling >>> > >>>> back to CDI.current().getBeanManager() in that case. The >>> workaround is >>> > >>>> needed because of a very nasty bug in de Wildfly-Weld integration: >>> > >>>> https://issues.jboss.org/browse/WFLY-2481 >>> > >>>> >>> > >>>> Best regards, >>> > >>>> Emond >>> > >>>> >>> > >>>> On Monday 11 November 2013 08:18:20 John Sarman wrote: >>> > >>>> > As far as the Test failing, I think the workaround code to use >>> jndi >>> > >>>> first >>> > >>>> > may have caused the test to fail. So far it seems that all the >>> > >>>> request >>> > >>>> > pull 50 is not in the 6 branch. >>> > >>>> > What forced the need for the workaround? >>> > >>>> > >>> > >>>> > John >>> > >>>> > >>> > >>>> > On Mon, Nov 11, 2013 at 8:00 AM, John Sarman >>> > >>>> <[email protected]> wrote: >>> > >>>> > > It is not a forced requirement, just an option for full cdi >>> > >>>> injection. >>> > >>>> > > >>> > >>>> > > >>> > >>>> > > On Mon, Nov 11, 2013 at 3:50 AM, Emond Papegaaij < >>> > >>>> > > >>> > >>>> > > [email protected]> wrote: >>> > >>>> > >> Hi John, >>> > >>>> > >> >>> > >>>> > >> I've just merged the pull request in the wicket-6.x branch >>> (still >>> > >>>> under >>> > >>>> > >> experimental). The version still is 0.2-SNAPSHOT, as the >>> versions >>> > >>>> are >>> > >>>> > >> automatically increased on release. The reason I've merged the >>> > pull >>> > >>>> > >> request is to give us all a common baseline to discuss. Could >>> you >>> > >>>> please >>> > >>>> > >> verify I did not break anything merging it? All testcases but >>> one >>> > >>>> pass. >>> > >>>> > >> The >>> > >>>> > >> failing testcase (CdiConfigurationTest.testMultiAppLoad) >>> tries to >>> > >>>> access >>> > >>>> > >> the >>> > >>>> > >> BeanManager from an unmanaged thread, resulting in an NPE. >>> > >>>> > >> >>> > >>>> > >> I've already noticed one aspect I do not like: the >>> requirement to >>> > >>>> > >> annotate >>> > >>>> > >> your app with @WicketApp. With a Producer method, it should be >>> > >>>> possible >>> > >>>> > >> to use the actual application names, without the requirement >>> to >>> > >>>> duplicate >>> > >>>> > >> them on your application class. >>> > >>>> > >> >>> > >>>> > >> Best regards, >>> > >>>> > >> Emond >>> > >>>> > >> >>> > >>>> > >> On Sunday 10 November 2013 16:44:28 John Sarman wrote: >>> > >>>> > >> > Edmond, >>> > >>>> > >> > On July, I worked vigorously to get to the 0.3 snapshot, >>> which >>> > >>>> was >>> > >>>> what >>> > >>>> > >> >>> > >>>> > >> I >>> > >>>> > >> >>> > >>>> > >> > consider the first beta ready version of the move to cdi1.1. >>> > The >>> > >>>> 0.1 >>> > >>>> > >> >>> > >>>> > >> and >>> > >>>> > >> >>> > >>>> > >> > 0.2 snapshot was 0.1, getting it to work and learning how to >>> > >>>> request >>> > >>>> > >> >>> > >>>> > >> pull >>> > >>>> > >> >>> > >>>> > >> > requests. 0.2 was adding some slight fixes and testing. >>> After >>> > >>>> that >>> > >>>> I >>> > >>>> > >> > realized that I was treating the @ApplicationScoped as same >>> > >>>> scope that >>> > >>>> > >> > ThreadContext gives to a Wicket App. That is entirely >>> wrong. >>> > So >>> > >>>> the >>> > >>>> > >> > previous version only properly supports at most 1 Wicket >>> app, >>> > the >>> > >>>> > >> >>> > >>>> > >> second >>> > >>>> > >> >>> > >>>> > >> > could override the Configuration of the first (not >>> acceptable). >>> > >>>> In >>> > >>>> my >>> > >>>> > >> >>> > >>>> > >> 0.3 >>> > >>>> > >> >>> > >>>> > >> > version, I added the code to prevent that, by using the >>> Wicket >>> > >>>> app >>> > >>>> key >>> > >>>> > >> > generated as the key to the configuration properties for an >>> > app. >>> > >>>> This >>> > >>>> > >> > allows for multiple Wicket apps to be deployed in a Servlet. >>> > >>>> However, >>> > >>>> > >> >>> > >>>> > >> for >>> > >>>> > >> >>> > >>>> > >> > whatever reason, that checkin could not properly merge into >>> > the 7 >>> > >>>> > >> >>> > >>>> > >> branch. >>> > >>>> > >> >>> > >>>> > >> > I have to remedy this even if I just have to copy paste the >>> > >>>> code, to >>> > >>>> > >> >>> > >>>> > >> make >>> > >>>> > >> >>> > >>>> > >> > git happy ( I blame myself, not Git). In the meantime, I >>> > >>>> recommend >>> > >>>> > >> >>> > >>>> > >> looking >>> > >>>> > >> >>> > >>>> > >> > at my latest (albeit broken) pull request >>> > >>>> > >> > https://github.com/apache/wicket/pull/50 and port that >>> > version. >>> > >>>> It >>> > >>>> adds >>> > >>>> > >> > thorough testing, fixes the multiple deploy issue, >>> reintroduces >>> > >>>> the >>> > >>>> > >> > auto >>> > >>>> > >> > Conversation, and extends the ConversationalComponent by >>> > >>>> introducing >>> > >>>> > >> >>> > >>>> > >> the >>> > >>>> > >> >>> > >>>> > >> > @Conversational, which by default works the same as the >>> Cdi-1.0 >>> > >>>> > >> > ConverationalComponent, but also allows the propagation and >>> > >>>> auto >>> > >>>> > >> >>> > >>>> > >> feature to >>> > >>>> > >> >>> > >>>> > >> > be modified for an Object that uses the annotation, without >>> > >>>> affecting >>> > >>>> > >> >>> > >>>> > >> the >>> > >>>> > >> >>> > >>>> > >> > global defaults set during Configuration. The 0.3 also >>> > >>>> introduces >>> > >>>> the >>> > >>>> > >> > CdiWicketFilter. The CdiWicketFilter allows the >>> configuration >>> > >>>> settings >>> > >>>> > >> >>> > >>>> > >> to >>> > >>>> > >> >>> > >>>> > >> > be managed in web.xml. It also instantiates the >>> > >>>> WicketApplication >>> > >>>> > >> > using >>> > >>>> > >> > Cdi so that the Application is injected before the init() >>> > method. >>> > >>>> The >>> > >>>> >>> > >>> >>> > >>> >>> > >> >>> > > >>> > >>> >> >>
