Re: [Gallery3] Unit test updates
Bharat Mediratta <[email protected]> Sun, 23 Jun 2013 15:33:58 -0700
| Newsgroups | gmane.comp.web.gallery.devel |
|---|---|
| Message-ID | <CAESa+_=B827AmDnbXJAKZSpDmZZcrcS+Wt-+EEVr=rvDjf-TUg@mail.gmail.com> |
--===============6191088213185421049== Content-Type: multipart/alternative; boundary=e89a8ff2511cbac47004dfd9e653 --e89a8ff2511cbac47004dfd9e653 Content-Type: text/plain; charset=ISO-8859-1 On Sun, Jun 23, 2013 at 8:33 AM, Shad Laws <shad-xpYdmXCiSuZWk0Htik3J/[email protected]> wrote: > 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? > That makes total sense. I forgot about the fact that you can go through that process multiple times. It's irritating to me that they made the routing and execution a single inseparable unit. > - 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 :-). > Ok that makes sense too - that way it gets called even if there's an exception or an error. We should keep it that way. > - 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()? > No, just arbitrary. > - 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? > All sounds good to me! > > 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 >>>>>>> >>>>>> >>>>>> >>>>> >>>> >>> >> > --e89a8ff2511cbac47004dfd9e653 Content-Type: text/html; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable <div dir=3D"ltr"><br><div class=3D"gmail_extra"><br><br><div class=3D"gmail= _quote">On Sun, Jun 23, 2013 at 8:33 AM, Shad Laws <span dir=3D"ltr"><<a= href=3D"mailto:shad-xpYdmXCiSuZWk0Htik3J/[email protected]" target=3D"_blank">shad-xpYdmXCiSuZWk0Htik3J/[email protected]</a>&= gt;</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>Overall, make= s sense to me. =A0A couple points:</div><div>- My guess is that K3 intentio= nally dropped the postrouting events with the MVC-->HMVC change since ro= uting 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). =A0Perhaps &= quot;initial_request_ready" instead?</div> </blockquote><div><br></div><div style>That makes total sense. =A0I forgot = about the fact that you can go through that process multiple times. =A0It&#= 39;s irritating to me that they made the routing and execution a single ins= eparable unit.</div> <div>=A0</div><blockquote class=3D"gmail_quote" style=3D"margin:0 0 0 .8ex;= border-left:1px #ccc solid;padding-left:1ex"> <div>- The shutdown function currently uses PHP's register_shutdown_fun= ction() in the bootstrap =A0(<a href=3D"https://github.com/gallery/gallery3= /blob/kohana_3/application/bootstrap.php#L245" target=3D"_blank">https://gi= thub.com/gallery/gallery3/blob/kohana_3/application/bootstrap.php#L245</a>)= . =A0I'm in no way committed to its placement... if you'd like it e= lsewhere, that works for me, too :-).</div> </blockquote><div><br></div><div style>Ok that makes sense too - that way i= t gets called even if there's an exception or an error. =A0We should ke= ep it that way.</div><div>=A0</div><blockquote class=3D"gmail_quote" style= =3D"margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div>- Your partition of the tasks by event makes sense to me. =A0Is there = any reason the bot detection part is in Gallery::ready() instead of Hook_Ga= lleryEvent::gallery_ready()?<br></div></blockquote><div><br></div><div styl= e> No, just arbitrary.</div><div>=A0</div><blockquote class=3D"gmail_quote" st= yle=3D"margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div>= </div><div><div>- Agreed - user_homes and sso should still work just fine w= ith 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></blockquote><div><br> </div><div style>All sounds good to me!</div><div style>=A0</div><blockquot= e class=3D"gmail_quote" style=3D"margin:0 0 0 .8ex;border-left:1px #ccc sol= id;padding-left:1ex"><div><br></div><div>Take care,</div> <div>Shad<div><div class=3D"h5"><br><br><div class=3D"gmail_quote">On 21 Ju= ne 2013 19:36, Bharat Mediratta <span dir=3D"ltr"><<a href=3D"mailto:bha= [email protected]" target=3D"_blank">[email protected]</a>></span> wrote:= <br><blockquote class=3D"gmail_quote" style=3D"margin:0 0 0 .8ex;border-lef= t:1px #ccc solid;padding-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? =A0So by = the time we get to Controller::execute() we need to have initialized our ro= utes already, which is why we do it in init.php. =A0And we need our own Rou= te::init() function that goes off and gets routes from all the modules. =A0= </div> <div><br></div><div>Makes sense to have two events. =A0But I think it's= a little weird to say "before_initial_request" and "after_i= nitial_request" because the after one implies to me that the request i= s done. =A0In K2 it was "prerouting" and "postrouting" = but those were Kohana events, not Gallery events.</div> </div><div><br></div><div>There are two ways to think about our events. =A0= One is to instruct modules to take an action, like "please inject your= routes now". =A0The other is to let modules know that we're at a = milestone, like "Hey the Gallery is ready". =A0Kohana takes the m= ilestone approach, but Gallery takes the action approach (with the exceptio= n 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">=A0 ->e= xecute()</span><br></div><div><font face=3D"courier new, monospace">=A0 -&g= t;send_headers(true)->body();</font></div><div><font face=3D"courier 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". =A0That should allow us= to load the theme and set the request locale after we have a request. =A0S= o 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>- =A0I don't see where Gallery::sh= utdown is called in the new code. =A0Did that get dropped?</div><div>- the = 3.0 user_homes module uses gallery_ready to redirect the user to a new loca= tion before the request occurs - I think we can still do that in the 3.1 ga= llery_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. =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><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. =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> </blockquote></div><br></div></div></div> </blockquote></div><br></div></div> --e89a8ff2511cbac47004dfd9e653-- --===============6191088213185421049== 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 --===============6191088213185421049== 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 ] --===============6191088213185421049==--