Re: [Gallery3] Unit test updates
Bharat Mediratta <[email protected]> Fri, 21 Jun 2013 10:36:52 -0700
| Newsgroups | gmane.comp.web.gallery.devel |
|---|---|
| Message-ID | <CAESa+_m3JsK+b3_N2UoBW=ibeptCsLVJuayZSxq59jmUKRft+A@mail.gmail.com> |
--===============5547446167599321266== Content-Type: multipart/alternative; boundary=089e01294b528c9d6504dfad8439 --089e01294b528c9d6504dfad8439 Content-Type: text/plain; charset=ISO-8859-1 (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 >>>>> >>>> >>>> >>> >> > --089e01294b528c9d6504dfad8439 Content-Type: text/html; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable <div dir=3D"ltr">(sorry for the rambly stream of consciousness here, sorry)= <div><br><div style>Ok, backing up to a high level - the issue is that we n= eed to reboot our routes every time there's a module change, right? =A0= 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. =A0And we need our o= wn Route::init() function that goes off and gets routes from all the module= s. =A0</div> <div style><br></div><div style>Makes sense to have two events. =A0But I th= ink it's a little weird to say "before_initial_request" and &= quot;after_initial_request" because the after one implies to me that t= he request is done. =A0In K2 it was "prerouting" and "postro= uting" but those were Kohana events, not Gallery events.</div> </div><div style><br></div><div style>There are two ways to think about our= events. =A0One is to instruct modules to take an action, like "please= inject your routes now". =A0The other is to let modules know that we&= #39;re at a milestone, like "Hey the Gallery is ready". =A0Kohana= takes the milestone approach, but Gallery takes the action approach (with = the exception of "gallery_ready" and "gallery_shutdown"= ).</div> <div style><br></div><div style>It's irritating that K3 doesn't hav= e a "postrouting" style hook for us.</div><div style><br></div><d= iv style>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 lik= e this:</div> <div style><br></div><div style><div><font face=3D"courier new, monospace">= // Initialize the framework.</font></div><div><font face=3D"courier new, mo= nospace">require APPPATH . "bootstrap" . EXT;</font></div><div><f= ont face=3D"courier new, monospace"><br> </font></div><div style><font face=3D"courier new, monospace"><b>Gallery::r= eady();</b></font></div><div><font face=3D"courier new, monospace"><br></fo= nt></div><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 style><span style=3D"font-family:'courier new',monospace">=A0 = ->execute()</span><br></div><div><font face=3D"courier new, monospace">= =A0 ->send_headers(true)->body();</font></div><div style><font face= =3D"courier new, monospace"><br> </font></div><div style><font face=3D"courier new, monospace"><b>Gallery::s= hutdown();</b></font></div><div style><font face=3D"courier new, monospace"= ><br></font></div><div style>Then in Gallery_Controller::execute() we can h= ave a new event which is "<b>gallery_request_ready</b>" which mea= ns "the request is ready but hasn't been executed yet". =A0Th= at should allow us to load the theme and set the request locale after we ha= ve a request. =A0So in Gallery_Hook_GalleryEvent:</div> <div style><br></div><div style>gallery_ready()</div><div style>- set date.= timezone</div><div style>- Identity::load_user()</div><div style><br></div>= <div style>gallery_request_ready():</div><div style>- bot detection<br> </div><div style>- Theme::load_themes()</div><div style>- Locales::set_requ= est_locale()<br></div><div style><br></div><div style><br></div><div style>= I think this is generally where you got.. does that sound right to you?</di= v> <div style><br></div><div style>Notes:</div><div style>- =A0I don't see= where Gallery::shutdown is called in the new code. =A0Did that get dropped= ?</div><div style>- the 3.0 user_homes module uses gallery_ready to redirec= t the user to a new location before the request occurs - I think we can sti= ll do that in the 3.1 gallery_ready</div> <div style>- the 3.0 sso module uses gallery_ready to change the active use= r - I think the 3.1 gallery_ready works for that as well</div><div style><b= r></div></div></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. =A0In particula= r, this screws up Theme::load_themes(), which uses Request to figure out if= we're in admin mode or have an override. =A0It also screws up our chec= k for a robot user_agent - while it doesn't bomb the code, Request::use= r_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. =A0While technically possible, this= sounds messy... I don't think it's a good approach.</div><div>- mo= ve a couple things out of the "ready" event, and leave them in Co= ntroller::execute(). =A0This gets the job done without major hacks, but the= fact that we need to do this in the first place hints that a better approa= ch would be to...</div> <div>- have two events, one right before the initial Request and one right = after. =A0This is my current favorite approach.</div><div><br></div><div>Ne= xt question: if we do the two-event option, what should they be called? =A0= Some 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 class=3D"HOEnZb"><font color= =3D"#888888"><div>Shad</div></font></span><div><div class=3D"h5"><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">bharat= @menalto.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"><div dir=3D"ltr"><br><div>I think that's= reasonable. =A0I've never been 100% happy with putting the "ready= " 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. =A0In fact, we could probably just move the execution of the already-e= xisting "ready" event from Controller::execute() to either bootst= rap.php or index.php (just before the initial request generation). =A0I won= der - 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. =A0I was thinking more along the lines of activating and = deactivating modules during a request's lifetime. =A0Eg, if we deactiva= te and reactivate a module several times, what is a developer supposed to d= o about their code in init.php? =A0Should devs guard against their code bei= ng called multiple times? =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. =A0We can leave the facility arou= nd, but since we're only using it for routes I think we could easily ha= ve an event hook which sets routes, then we can call that from Module whene= ver 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=A0Graphics::deactivate_rules a= nd=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. =A0I got everything r= unning well and passing, hit a reset button, and then everything broke with= the REST tests - crap! =A0After a bit of digging, I found the problem: the= REST tests won't pass on a fresh installation. =A0They only pass if yo= u activate the rest module, *then* run unit tests.</div> <div><br></div><div>The issue is the rest module's init.php file. =A0Li= ke the other init.php files (e.g. in tag), it's designed to run *before= * the bootstrap routes are defined. =A0That way, they get precedence. =A0Ho= wever, 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 :-/</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?). =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> --089e01294b528c9d6504dfad8439-- --===============5547446167599321266== 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 --===============5547446167599321266== 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 ] --===============5547446167599321266==--