Hi ! 2010/7/26 Kalle Korhonen <kalle.o.korho...@gmail.com>
> On Mon, Jul 26, 2010 at 3:17 AM, Christophe Cordenier > <christophe.corden...@gmail.com> wrote: > > 2010/7/26 Kalle Korhonen <kalle.o.korho...@gmail.com> > >> On Sun, Jul 25, 2010 at 11:45 PM, Christophe Cordenier > >> <christophe.corden...@gmail.com> wrote: > > Actually i was not thinking about catching the exception at the filter > level > > since Tapestry does it well and as you said, decoration should do the > trick, > > but simply let Tapestry initialize its context (ActivatePage) In fact i > was > > wondering, does your security filter implement a ComponentRequestFilter > or a > > RequestFilter ? > > Ah yes, forgive me for being a little slow here. Yes, the > SecurityFilter is a ComponentRequestFilter. If I contribute it right > after "InitializeActivePageName", the fairly comprehensive integration > tests for the module pass so it seems that would do it. Should have > found that out and tried it before myself. But wouldn't the wrong page > be stored as the active page in this case or does it not matter? > Right it matters : you will not be able to pre-allocate form control names... I have to look deeper to see where storeActivePageName() should be called to work properly with every type of request... > > This is not directly related to the issue, but assuming the above > solution is the right way to go about it and given that > InitializeActivePageName didn't exist in T5.1, what's the best way for > checking the version of the core library at runtime (so I can > contribute it differently depending on the version)? > > Kalle > > > >> > Actually, i am not against applying your patch, but as > >> PageResponseRenderer > >> > is internal, as a new committer i am definitely cautious in what i am > >> doing > >> > and committing ;) > >> > >> No worries, I fully understand where you are coming from. The code > >> worked as is for T5.1 and I already experimentally modified it to call > >> storeActivePageName() from outside just to verify that'd be enough for > >> T5.2. So it's not a matter of making it work, but making it work the > >> right way. Although we both may end up spending more time on it this > >> way, I much prefer spending my resources on validating the concept > >> than just hacking it in. > >> > >> So in the above case, we don't really want to replace the > >> RequestExceptionHandler as handling the non-security related > >> exceptions are delegated back to the original, but if somebody > >> replaces the DefaultRequestExceptionHandler with one that contains a > >> form on it, it'd throw an exception as well (just putting together > >> what you said and reading the code), right? Hopefully that proves the > >> need for the fix. > >> > >> Kalle > >> > >> > >> > 2010/7/26 Kalle Korhonen <kalle.o.korho...@gmail.com> > >> > > >> >> On Sun, Jul 25, 2010 at 5:35 AM, Christophe Cordenier > >> >> <christophe.corden...@gmail.com> wrote: > >> >> > I am currently working on this JIRA, i have seen that the > >> >> getActivatePage() > >> >> > is only called to pre allocate names in Form component so it should > >> have. > >> >> So > >> >> > i guess the page you are returning contains a Form. > >> >> > >> >> Right and thanks so much Christophe for taking a look at the issue. > >> >> > >> >> > Despite it sound right that PageResponseRenderer should set the > active > >> >> page, > >> >> > don't you think that in case of a security exception it would be > >> easier > >> >> to > >> >> > send an HTTP error and let the application developer use standard > >> web.xml > >> >> > request dispatching to configure the page to display ? > >> >> > >> >> Specifically in the case of security exception, no, I don't think > >> >> that'd be easier. While it's an option, I don't see what the > advantage > >> >> would be. The exception handler in tapestry-security catches the > >> >> exception, sets the error code and renders the configured page. > >> >> Configuring the page somewhere else (in standard web.xml) seems to me > >> >> like an unnecessary complication compared to contributing > >> >> configuration. > >> >> > >> >> Security exception aside, in the general case http error codes alone > >> >> wouldn't be enough to differentiate all the different error cases. > I'm > >> >> not strongly against exploring alternatives, but it makes sense to me > >> >> that PageResponseRenderer would set the active page and I don't see > >> >> any disadvantages in doing so, do you? > >> >> > >> >> Kalle > >> >> > >> >> > >> >> > 2010/7/20 Kalle Korhonen <kalle.o.korho...@gmail.com> > >> >> >> Pretty please, any committer? It's a one-liner to fix > >> >> >> https://issues.apache.org/jira/browse/TAP5-1201 and the patch is > >> >> >> attached. > >> >> >> > >> >> >> Kalle > >> >> >> > >> >> >> > >> >> >> On Mon, Jul 19, 2010 at 2:23 PM, Pierce Wetter <pie...@paceap.com > > > >> >> wrote: > >> >> >> > > >> >> >> > On Jul 9, 2010, at 1:40 PM, Kalle Korhonen wrote: > >> >> >> > > >> >> >> >> Sorry to be a pest, but how could I find a champion to apply > the > >> >> patch > >> >> >> >> for TAP5-1201? I understand Howard's busy and his time is > probably > >> >> >> >> better spent on bigger issues than this, but I hope that some > >> other > >> >> >> >> committer would be able to pick this up and apply the one-line > >> change > >> >> >> >> that would restore functionality that existed in T5.1. It's my > >> >> >> >> understanding its perfectly safe to do as suggested so we can't > go > >> >> >> >> that far off even if somebody wants to refactor it later. > >> >> >> > > >> >> >> > Ditto, since I'm using tapestry-security with 5.2-SNAPSHOT. > >> >> >> > > >> >> >> > > >> >> >> > > >> --------------------------------------------------------------------- > >> >> >> > To unsubscribe, e-mail: users-unsubscr...@tapestry.apache.org > >> >> >> > For additional commands, e-mail: users-h...@tapestry.apache.org > >> >> >> > > >> >> >> > > >> >> >> > >> >> >> > --------------------------------------------------------------------- > >> >> >> To unsubscribe, e-mail: users-unsubscr...@tapestry.apache.org > >> >> >> For additional commands, e-mail: users-h...@tapestry.apache.org > >> >> >> > >> >> >> > >> >> > > >> >> > > >> >> > -- > >> >> > Regards, > >> >> > Christophe Cordenier. > >> >> > > >> >> > Committer on Apache Tapestry 5 > >> >> > Co-creator of wooki @wookicentral.com > >> >> > > >> >> > >> >> --------------------------------------------------------------------- > >> >> To unsubscribe, e-mail: users-unsubscr...@tapestry.apache.org > >> >> For additional commands, e-mail: users-h...@tapestry.apache.org > >> >> > >> >> > >> > > >> > > >> > -- > >> > Regards, > >> > Christophe Cordenier. > >> > > >> > Committer on Apache Tapestry 5 > >> > Co-creator of wooki @wookicentral.com > >> > > >> > >> --------------------------------------------------------------------- > >> To unsubscribe, e-mail: users-unsubscr...@tapestry.apache.org > >> For additional commands, e-mail: users-h...@tapestry.apache.org > >> > >> > > > > > > -- > > Regards, > > Christophe Cordenier. > > > > Committer on Apache Tapestry 5 > > Co-creator of wooki @wookicentral.com > > > > --------------------------------------------------------------------- > To unsubscribe, e-mail: users-unsubscr...@tapestry.apache.org > For additional commands, e-mail: users-h...@tapestry.apache.org > > -- Regards, Christophe Cordenier. Committer on Apache Tapestry 5 Co-creator of wooki @wookicentral.com
