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 > >>> > >>>> > >>> > >>> > >>> > >>> > >>> > >> > >>> > > > >>> > > >>> > >> > >> >
