Since the NonContextualManager is responsible for injection post construction and predestroying, what would you replace it with?
On Wed, Nov 13, 2013 at 3:22 PM, Emond Papegaaij <[email protected]>wrote: > On Tue, Nov 12, 2013 at 11:32 PM, Igor Vaynberg <[email protected] > >wrote: > > > the point on INonContextualManager was to internalize NonContextual - > > in case cdi implementation provides a better way to perform > > non-contextual injection. > > > > I don't think that going to happen. Even if it does happen, I see no reason > why Wicket must be able to use that implementation specific way of > injecting objects. The current implementation works fine and is supported > by all CDI providers. > > 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... > > > > That's what alternatives are for. But again: I see no reason to replace > NonContextual. > > I would like to remove NonContextualManager (and the interface) as I don't > see any added value. > > Best regards, > Emond > > 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 > > >>> > >>>> > > >>> > >>> > > >>> > >>> > > >>> > >> > > >>> > > > > >>> > > > >>> > > >> > > >> > > >
