Re: [Gallery3] Unit test updates
Shad Laws <shad-xpYdmXCiSuZWk0Htik3J/[email protected]> Sun, 23 Jun 2013 17:33:39 +0200
| Newsgroups | gmane.comp.web.gallery.devel |
|---|---|
| Message-ID | <CA+z51A6yqEDh6O-yrvZF_uiocccObyNMHwkkxB6XV_BxtLEngA@mail.gmail.com> |
--===============7987181647302455369== Content-Type: multipart/alternative; boundary=047d7b6da7825a073704dfd406d7 --047d7b6da7825a073704dfd406d7 Content-Type: text/plain; charset=UTF-8 Hey Bharat, Overall, makes sense to me. A couple points: - My guess is that K3 intentionally dropped the postrouting events with the MVC-->HMVC change since routing occurs with every new request <shrug>. - I'm not sure I like "gallery_request_ready" as I feel like it should run with every request (which it doesn't - in K3-speak, it fires only in the initial request and not on sub-requests). Perhaps "initial_request_ready" instead? - The shutdown function currently uses PHP's register_shutdown_function() in the bootstrap ( https://github.com/gallery/gallery3/blob/kohana_3/application/bootstrap.php#L245). I'm in no way committed to its placement... if you'd like it elsewhere, that works for me, too :-). - Your partition of the tasks by event makes sense to me. Is there any reason the bot detection part is in Gallery::ready() instead of Hook_GalleryEvent::gallery_ready()? - Agreed - user_homes and sso should still work just fine with the new events. Oh, and re: rambly stream of consciousness, I'm probably not in a great position to critique emails like that as of late... :-) Thoughts? Take care, Shad On 21 June 2013 19:36, Bharat Mediratta <[email protected]> wrote: > (sorry for the rambly stream of consciousness here, sorry) > > Ok, backing up to a high level - the issue is that we need to reboot our > routes every time there's a module change, right? So by the time we get to > Controller::execute() we need to have initialized our routes already, which > is why we do it in init.php. And we need our own Route::init() function > that goes off and gets routes from all the modules. > > Makes sense to have two events. But I think it's a little weird to say > "before_initial_request" and "after_initial_request" because the after one > implies to me that the request is done. In K2 it was "prerouting" and > "postrouting" but those were Kohana events, not Gallery events. > > There are two ways to think about our events. One is to instruct modules > to take an action, like "please inject your routes now". The other is to > let modules know that we're at a milestone, like "Hey the Gallery is > ready". Kohana takes the milestone approach, but Gallery takes the action > approach (with the exception of "gallery_ready" and "gallery_shutdown"). > > It's irritating that K3 doesn't have a "postrouting" style hook for us. > > One quick note is that I think we should put these events in index.php > since they're about the lifecycle of the app, so it'd look like this: > > // Initialize the framework. > require APPPATH . "bootstrap" . EXT; > > *Gallery::ready();* > > // Go! > echo Request::factory(true, array(), false) > ->execute() > ->send_headers(true)->body(); > > *Gallery::shutdown();* > > Then in Gallery_Controller::execute() we can have a new event which is "* > gallery_request_ready*" which means "the request is ready but hasn't been > executed yet". That should allow us to load the theme and set the request > locale after we have a request. So in Gallery_Hook_GalleryEvent: > > gallery_ready() > - set date.timezone > - Identity::load_user() > > gallery_request_ready(): > - bot detection > - Theme::load_themes() > - Locales::set_request_locale() > > > I think this is generally where you got.. does that sound right to you? > > Notes: > - I don't see where Gallery::shutdown is called in the new code. Did > that get dropped? > - the 3.0 user_homes module uses gallery_ready to redirect the user to a > new location before the request occurs - I think we can still do that in > the 3.1 gallery_ready > - the 3.0 sso module uses gallery_ready to change the active user - I > think the 3.1 gallery_ready works for that as well > > > > On Thu, Jun 20, 2013 at 11:06 PM, Shad Laws <shad-xpYdmXCiSuZWk0Htik3J/[email protected]> wrote: > >> I tried playing with this, and now remember why we put it in >> Controller::execute() - if we put it before the initial request generation, >> we (unsurprisingly) don't have access to a populated Request object. In >> particular, this screws up Theme::load_themes(), which uses Request to >> figure out if we're in admin mode or have an override. It also screws up >> our check for a robot user_agent - while it doesn't bomb the code, >> Request::user_agent() checks against a still-unset value. >> >> A few options: >> - hack Theme::load_themes() and the robot check to no longer need >> Request. While technically possible, this sounds messy... I don't think >> it's a good approach. >> - move a couple things out of the "ready" event, and leave them in >> Controller::execute(). This gets the job done without major hacks, but the >> fact that we need to do this in the first place hints that a better >> approach would be to... >> - have two events, one right before the initial Request and one right >> after. This is my current favorite approach. >> >> Next question: if we do the two-event option, what should they be called? >> Some possibilities: >> - Before request: init (exact Kohana analog), routes (most common >> application), bootstrap, before_initial_request >> - After request: gallery_ready (current name), ready (shorter name), >> after_initial_request >> >> Thoughts? >> Shad >> >> On 21 June 2013 01:12, Bharat Mediratta <[email protected]> wrote: >> >>> >>> I think that's reasonable. I've never been 100% happy with putting the >>> "ready" event in Controller::execute. >>> >>> >>> On Thu, Jun 20, 2013 at 3:44 PM, Shad Laws <shad-xpYdmXCiSuZWk0Htik3J/[email protected]> wrote: >>> >>>> I like the idea of reducing the init.php's down to zero and putting it >>>> in an event, and agree that trying to keep track of this isn't a good idea. >>>> In fact, we could probably just move the execution of the already-existing >>>> "ready" event from Controller::execute() to either bootstrap.php or >>>> index.php (just before the initial request generation). I wonder - should >>>> it be renamed "init" to make its analog more obvious? >>>> >>>> Then, we just add a Route override that clears out the Route list, and >>>> use it in Module to activate/deactivate things as well as in the unit test. >>>> >>>> Thoughts? >>>> >>>> Take care, >>>> Shad >>>> >>>> On 21 June 2013 00:12, Bharat Mediratta <[email protected]> wrote: >>>> >>>>> >>>>> I've been wondering about that as well, but I wasn't thinking about it >>>>> from the unit test perspective. I was thinking more along the lines of >>>>> activating and deactivating modules during a request's lifetime. Eg, if we >>>>> deactivate and reactivate a module several times, what is a developer >>>>> supposed to do about their code in init.php? Should devs guard against >>>>> their code being called multiple times? It feels messy to me. >>>>> >>>>> I think a better approach would be to minimize what goes into >>>>> init.php, preferably down to zero. We can leave the facility around, but >>>>> since we're only using it for routes I think we could easily have an event >>>>> hook which sets routes, then we can call that from Module whenever we add a >>>>> module. >>>>> >>>>> If we want to be smart about removing routes when a module is >>>>> deactivated (not essential for us to do right now, IMO) we could always >>>>> track which routes were added when we call the hook and remove them when >>>>> the module goes away, similar to how we do Graphics::deactivate_rules >>>>> and BlockManager::deactivate_blocks... >>>>> >>>>> >>>>> >>>>> >>>>> On Thu, Jun 20, 2013 at 10:52 AM, Shad Laws <shad-xpYdmXCiSuZWk0Htik3J/[email protected]> wrote: >>>>> >>>>>> Hey Bharat, >>>>>> >>>>>> It looks like, except for the REST-related unit tests I'm still >>>>>> wrapping up, the only tests left are controller auth and XSS - woohoo! >>>>>> >>>>>> I did, however, hit a minor snag. I got everything running well and >>>>>> passing, hit a reset button, and then everything broke with the REST tests >>>>>> - crap! After a bit of digging, I found the problem: the REST tests won't >>>>>> pass on a fresh installation. They only pass if you activate the rest >>>>>> module, *then* run unit tests. >>>>>> >>>>>> The issue is the rest module's init.php file. Like the other >>>>>> init.php files (e.g. in tag), it's designed to run *before* the bootstrap >>>>>> routes are defined. That way, they get precedence. However, if you roll a >>>>>> fresh install (which has rest disabled) straight into a unit test, it bombs >>>>>> - the bootstrap runs first, then rest's init.php later... and it doesn't >>>>>> work :-/ >>>>>> >>>>>> One thought I have is to override the Route class so we can clear >>>>>> them and restart from scratch, then make the unit tests re-load them all... >>>>>> but that sounds kinda hacky and flaky (e.g. what happens if an init.php >>>>>> does something besides load routes?). Any better thoughts? >>>>>> >>>>>> Take care, >>>>>> Shad >>>>>> >>>>> >>>>> >>>> >>> >> > --047d7b6da7825a073704dfd406d7 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Hey Bharat,<div><br></div><div>Overall, makes sense to me. =C2=A0A couple p= oints:</div><div>- My guess is that K3 intentionally dropped the postroutin= g events with the MVC-->HMVC change since routing occurs with every new = request <shrug>.</div> <div>- I'm not sure I like "gallery_request_ready" as I feel = like it should run with every request (which it doesn't - in K3-speak, = it fires only in the initial request and not on sub-requests). =C2=A0Perhap= s "initial_request_ready" instead?</div> <div>- The shutdown function currently uses PHP's register_shutdown_fun= ction() in the bootstrap =C2=A0(<a href=3D"https://github.com/gallery/galle= ry3/blob/kohana_3/application/bootstrap.php#L245">https://github.com/galler= y/gallery3/blob/kohana_3/application/bootstrap.php#L245</a>). =C2=A0I'm= in no way committed to its placement... if you'd like it elsewhere, th= at works for me, too :-).</div> <div>- Your partition of the tasks by event makes sense to me. =C2=A0Is the= re any reason the bot detection part is in Gallery::ready() instead of Hook= _GalleryEvent::gallery_ready()?<br></div><div><div>- Agreed - user_homes an= d sso should still work just fine with the new events.</div> </div><div><br></div><div>Oh, and re: rambly stream of consciousness, I'= ;m probably not in a great position to critique emails like that as of late= ... :-)</div><div><br></div><div>Thoughts?</div><div><br></div><div>Take ca= re,</div> <div>Shad<br><br><div class=3D"gmail_quote">On 21 June 2013 19:36, Bharat M= ediratta <span dir=3D"ltr"><<a href=3D"mailto:[email protected]" target= =3D"_blank">[email protected]</a>></span> wrote:<br><blockquote class= =3D"gmail_quote" style=3D"margin:0 0 0 .8ex;border-left:1px #ccc solid;padd= ing-left:1ex"> <div dir=3D"ltr">(sorry for the rambly stream of consciousness here, sorry)= <div><br><div>Ok, backing up to a high level - the issue is that we need to= reboot our routes every time there's a module change, right? =C2=A0So = by the time we get to Controller::execute() we need to have initialized our= routes already, which is why we do it in init.php. =C2=A0And we need our o= wn Route::init() function that goes off and gets routes from all the module= s. =C2=A0</div> <div><br></div><div>Makes sense to have two events. =C2=A0But I think it= 9;s a little weird to say "before_initial_request" and "afte= r_initial_request" because the after one implies to me that the reques= t is done. =C2=A0In K2 it was "prerouting" and "postrouting&= quot; but those were Kohana events, not Gallery events.</div> </div><div><br></div><div>There are two ways to think about our events. =C2= =A0One is to instruct modules to take an action, like "please inject y= our routes now". =C2=A0The other is to let modules know that we're= at a milestone, like "Hey the Gallery is ready". =C2=A0Kohana ta= kes the milestone approach, but Gallery takes the action approach (with the= exception of "gallery_ready" and "gallery_shutdown").<= /div> <div><br></div><div>It's irritating that K3 doesn't have a "po= strouting" style hook for us.</div><div><br></div><div>One quick note = is that I think we should put these events in index.php since they're a= bout the lifecycle of the app, so it'd look like this:</div> <div><br></div><div><div><font face=3D"courier new, monospace">// Initializ= e the framework.</font></div><div><font face=3D"courier new, monospace">req= uire APPPATH . "bootstrap" . EXT;</font></div><div><font face=3D"= courier new, monospace"><br> </font></div><div><font face=3D"courier new, monospace"><b>Gallery::ready()= ;</b></font></div><div><font face=3D"courier new, monospace"><br></font></d= iv><div><font face=3D"courier new, monospace">// Go!</font></div><div> <font face=3D"courier new, monospace">echo Request::factory(true, array(), = false)</font></div> <div><span style=3D"font-family:'courier new',monospace">=C2=A0 -&g= t;execute()</span><br></div><div><font face=3D"courier new, monospace">=C2= =A0 ->send_headers(true)->body();</font></div><div><font face=3D"cour= ier new, monospace"><br> </font></div><div><font face=3D"courier new, monospace"><b>Gallery::shutdow= n();</b></font></div><div><font face=3D"courier new, monospace"><br></font>= </div><div>Then in Gallery_Controller::execute() we can have a new event wh= ich is "<b>gallery_request_ready</b>" which means "the reque= st is ready but hasn't been executed yet". =C2=A0That should allow= us to load the theme and set the request locale after we have a request. = =C2=A0So in Gallery_Hook_GalleryEvent:</div> <div><br></div><div>gallery_ready()</div><div>- set date.timezone</div><div= >- Identity::load_user()</div><div><br></div><div>gallery_request_ready():<= /div><div>- bot detection<br> </div><div>- Theme::load_themes()</div><div>- Locales::set_request_locale()= <br></div><div><br></div><div><br></div><div>I think this is generally wher= e you got.. does that sound right to you?</div> <div><br></div><div>Notes:</div><div>- =C2=A0I don't see where Gallery:= :shutdown is called in the new code. =C2=A0Did that get dropped?</div><div>= - the 3.0 user_homes module uses gallery_ready to redirect the user to a ne= w location before the request occurs - I think we can still do that in the = 3.1 gallery_ready</div> <div>- the 3.0 sso module uses gallery_ready to change the active user - I = think the 3.1 gallery_ready works for that as well</div><div><br></div></di= v></div><div class=3D"gmail_extra"><br><br><div class=3D"gmail_quote"> On Thu, Jun 20, 2013 at 11:06 PM, Shad Laws <span dir=3D"ltr"><<a href= =3D"mailto:shad-xpYdmXCiSuZWk0Htik3J/[email protected]" target=3D"_blank">shad-xpYdmXCiSuZWk0Htik3J/[email protected]</a>></= span> wrote:<br><blockquote class=3D"gmail_quote" style=3D"margin:0 0 0 .8e= x;border-left:1px #ccc solid;padding-left:1ex"> I tried playing with this, and now remember why we put it in Controller::ex= ecute() - if we put it before the initial request generation, we (unsurpris= ingly) don't have access to a populated Request object. =C2=A0In partic= ular, this screws up Theme::load_themes(), which uses Request to figure out= if we're in admin mode or have an override. =C2=A0It also screws up ou= r check for a robot user_agent - while it doesn't bomb the code, Reques= t::user_agent() checks against a still-unset value.<div> <br></div><div>A few options:</div><div>- hack Theme::load_themes() and the= robot check to no longer need Request. =C2=A0While technically possible, t= his sounds messy... I don't think it's a good approach.</div><div>-= move a couple things out of the "ready" event, and leave them in= Controller::execute(). =C2=A0This gets the job done without major hacks, b= ut the fact that we need to do this in the first place hints that a better = approach would be to...</div> <div>- have two events, one right before the initial Request and one right = after. =C2=A0This is my current favorite approach.</div><div><br></div><div= >Next question: if we do the two-event option, what should they be called? = =C2=A0Some possibilities:</div> <div>- Before request: init (exact Kohana analog), routes (most common appl= ication), bootstrap, before_initial_request</div><div>- After request: gall= ery_ready (current name), ready (shorter name), after_initial_request</div> <div><div><br></div><div>Thoughts?</div><span><font color=3D"#888888"><div>= Shad</div></font></span><div><div><div><br><div class=3D"gmail_quote">On 21= June 2013 01:12, Bharat Mediratta <span dir=3D"ltr"><<a href=3D"mailto:= [email protected]" target=3D"_blank">[email protected]</a>></span> wro= te:<br> <blockquote class=3D"gmail_quote" style=3D"margin:0 0 0 .8ex;border-left:1p= x #ccc solid;padding-left:1ex"><div dir=3D"ltr"><br><div>I think that's= reasonable. =C2=A0I've never been 100% happy with putting the "re= ady" event in Controller::execute.</div> </div><div class=3D"gmail_extra"><br><br><div class=3D"gmail_quote"> On Thu, Jun 20, 2013 at 3:44 PM, Shad Laws <span dir=3D"ltr"><<a href=3D= "mailto:shad-xpYdmXCiSuZWk0Htik3J/[email protected]" target=3D"_blank">shad-xpYdmXCiSuZWk0Htik3J/[email protected]</a>></spa= n> wrote:<br><blockquote class=3D"gmail_quote" style=3D"margin:0 0 0 .8ex;b= order-left:1px #ccc solid;padding-left:1ex"> I like the idea of reducing the init.php's down to zero and putting it = in an event, and agree that trying to keep track of this isn't a good i= dea. =C2=A0In fact, we could probably just move the execution of the alread= y-existing "ready" event from Controller::execute() to either boo= tstrap.php or index.php (just before the initial request generation). =C2= =A0I wonder - should it be renamed "init" to make its analog more= obvious?<div> <br></div><div>Then, we just add a Route override that clears out the Route= list, and use it in Module to activate/deactivate things as well as in the= unit test.</div><div><br></div><div>Thoughts?</div><div><br></div><div> Take care,</div><div>Shad</div><div><div><div><div><div><br><div class=3D"g= mail_quote">On 21 June 2013 00:12, Bharat Mediratta <span dir=3D"ltr"><<= a href=3D"mailto:[email protected]" target=3D"_blank">[email protected]</= a>></span> wrote:<br> <blockquote class=3D"gmail_quote" style=3D"margin:0 0 0 .8ex;border-left:1p= x #ccc solid;padding-left:1ex"><div dir=3D"ltr"><br><div>I've been wond= ering about that as well, but I wasn't thinking about it from the unit = test perspective. =C2=A0I was thinking more along the lines of activating a= nd deactivating modules during a request's lifetime. =C2=A0Eg, if we de= activate and reactivate a module several times, what is a developer suppose= d to do about their code in init.php? =C2=A0Should devs guard against their= code being called multiple times? =C2=A0It feels messy to me.</div> <div><br></div><div>I think a better approach would be to minimize what goe= s into init.php, preferably down to zero. =C2=A0We can leave the facility a= round, but since we're only using it for routes I think we could easily= have an event hook which sets routes, then we can call that from Module wh= enever we add a module.</div> <div><br></div><div>If we want to be smart about removing routes when a mod= ule is deactivated (not essential for us to do right now, IMO) we could alw= ays track which routes were added when we call the hook and remove them whe= n the module goes away, similar to how we do=C2=A0Graphics::deactivate_rule= s and=C2=A0BlockManager::deactivate_blocks...</div> <div><br></div><div><br></div></div><div class=3D"gmail_extra"><br><br><div= class=3D"gmail_quote">On Thu, Jun 20, 2013 at 10:52 AM, Shad Laws <span di= r=3D"ltr"><<a href=3D"mailto:shad-xpYdmXCiSuZWk0Htik3J/[email protected]" target=3D"_blank">shad@s= hadlaws.com</a>></span> wrote:<br> <blockquote class=3D"gmail_quote" style=3D"margin:0 0 0 .8ex;border-left:1p= x #ccc solid;padding-left:1ex">Hey Bharat,<div><br></div><div>It looks like= , except for the REST-related unit tests I'm still wrapping up, the onl= y tests left are controller auth and XSS - woohoo!</div> <div><br></div><div>I did, however, hit a minor snag. =C2=A0I got everythin= g running well and passing, hit a reset button, and then everything broke w= ith the REST tests - crap! =C2=A0After a bit of digging, I found the proble= m: the REST tests won't pass on a fresh installation. =C2=A0They only p= ass if you activate the rest module, *then* run unit tests.</div> <div><br></div><div>The issue is the rest module's init.php file. =C2= =A0Like the other init.php files (e.g. in tag), it's designed to run *b= efore* the bootstrap routes are defined. =C2=A0That way, they get precedenc= e. =C2=A0However, if you roll a fresh install (which has rest disabled) str= aight into a unit test, it bombs - the bootstrap runs first, then rest'= s init.php later... and it doesn't work :-/</div> <div><br></div><div>One thought I have is to override the Route class so we= can clear them and restart from scratch, then make the unit tests re-load = them all... but that sounds kinda hacky and flaky (e.g. what happens if an = init.php does something besides load routes?). =C2=A0Any better thoughts?</= div> <div><br></div><div>Take care,</div><div>Shad</div> </blockquote></div><br></div> </blockquote></div><br></div></div></div> </div></div></blockquote></div><br></div> </blockquote></div><br></div></div></div></div> </blockquote></div><br></div> </blockquote></div><br></div> --047d7b6da7825a073704dfd406d7-- --===============7987181647302455369== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline ------------------------------------------------------------------------------ This SF.net email is sponsored by Windows: Build for Windows Store. http://p.sf.net/sfu/windows-dev2dev --===============7987181647302455369== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline __[ g a l l e r y - d e v e l ]_________________________ [ list info/archive --> http://gallery.sf.net/lists.php ] [ gallery info/FAQ/download --> http://gallery.sf.net ] --===============7987181647302455369==--