So basically just move the NonContextual code directly into the AbstractInjector. I'm fine with that.
On Wed, Nov 13, 2013 at 3:52 PM, Emond Papegaaij <[email protected]>wrote: > NonContextual.of(instance.getClass()).inject(instance); > > iow, exactly what's in the methods in NonContextualManager > > > On Wed, Nov 13, 2013 at 9:47 PM, John Sarman <[email protected]> wrote: > > > 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 > > > > >>> > >>>> > > > > >>> > >>> > > > > >>> > >>> > > > > >>> > >> > > > > >>> > > > > > > >>> > > > > > >>> > > > > >> > > > > >> > > > > > > > > > >
