From: Shad L. <sh...@sh...> - 2013-06-24 10:08:22
|
Hey Bharat, Hmm, I'm not sure I see routing and execution as one inseparable unit. It seems to me that they decided to remove their event handler and instead make the cascading file system (which has far less exceptions to rules than before) and the config file system beefier to pick up the slack. For example, extending the Controller class as we do is a nice way to split the two. Alternatively, we can just generate the request and wait a tick before executing it. Here's my sketch of the end of index.php: // Initialize the framework. require APPPATH . "bootstrap" . EXT; // Initialize the Gallery modules. register_shutdown_function("Module::event", "shutdown"); Module::event("gallery_ready"); // Build the initial request. $request = Request::factory(true, array(), false); Module::event("initial_request_ready"); // Generate and send the response. echo $request->execute() ->send_headers(true) ->body(); The functions Gallery::ready() and Gallery::shutdown() are removed, as they were just one-line wrappers around their respective module events. I like this more direct approach. But, this did show me an odd wrinkle with module events: the module order seems weird. - active modules list: purifier, high_priority_module, low_priority_module, gallery (same as Kohana's list) - current event order: gallery, purifier, high_priority_module, low_priority_module (put gallery first, leave rest as-is) - proposed event order: gallery, low_priority_module, high_priority_module, purifier (reverse of active list, same as Kohana's config file search) In other words, I think it should be changed to run the events in reverse module order, thereby giving the high-priority modules the last word. Is there a reason we don't already do it this way? Among other advantages, if we can change this, then the routes get loaded in the right order :-). Take care, Shad On 24 June 2013 00:33, Bharat Mediratta <bh...@me...> wrote: > > > > On Sun, Jun 23, 2013 at 8:33 AM, Shad Laws <sh...@sh...> 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 <bh...@me...> 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 <sh...@sh...> 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 <bh...@me...> 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 <sh...@sh...> 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 <bh...@me...> 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 <sh...@sh...>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 >>>>>>>> >>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>> >> > |