Remove hook_exit() for cached page requests

I think this will fail hard because of the caching stuff that happens with hook_exit. Just wanted to help (and learn some more symfony stuff while doing it)

I also removed this

-  // When serving cached pages with the 'page_cache_without_database'
-  // configuration, system variables need to be loaded. This is a major
-  // performance decrease for non-database page caches, but with Statistics
-  // module, it is likely to also have 'statistics.settings:access_log.enabled'
-  // enabled, in which case we need to bootstrap to the session phase anyway.
-  drupal_bootstrap(DRUPAL_BOOTSTRAP_VARIABLES);

Because I understood the page_cache_without_database is totally broken already.

+++ b/core/includes/common.incundefined
@@ -2270,6 +2270,7 @@ function l($text, $path, array $options = array()) {
  *   will be a fully-qualified URL that is the destination of the redirect.
  *   This should be passed along to hook_exit() implementations.
+ *   // Should be changed
  */
 function drupal_exit($destination = NULL) {
   if (drupal_get_bootstrap_phase() == DRUPAL_BOOTSTRAP_FULL) {

I assume that drupal_exit() should either be killed completely or changed to call $kernel->terminate() ?

+++ b/core/modules/statistics/lib/Drupal/statistics/EventSubscriber/StatisticsCloseSubscriber.phpundefined
@@ -0,0 +1,62 @@
+   * @param Symfony\Component\HttpKernel\Event\PostResponseEvent $event

@param \Symfony\...

+++ b/core/modules/system/tests/modules/system_test/lib/Drupal/system_test/EventSubscriber/Backup 1 of SystemTestCloseSubscriber	
@@ -0,0 +1,40 @@
+ * Definition of Drupal\system_test\EventSubscriber\SystemTestCloseSubscriber.

Should be Contains... now.

Simply doesn't work. The terminate stuff is never called (see failing tests).

aspilicious: $kernel->terminate should be $kernel->terminate() :)
aspilicious: with a request and response object
aspilicious: and the fact that we don't have a response object is probably the reason why we need to kill drupal_exit() and instead do return new Response().

Yeah offcourse my code is incorrect but I'm still wondering why the onTerminate stuff NEVER gets called. The modified hook_exit tests never adds a watchdog record.

This should at least pass some tests...

OK, I'll bite - why is this a critical issue, and is it even a good idea? I think there can be value in having a "this is Drupal speaking and I'm done my processing of the request" event that is separate from the overall request. And from one look at the patch, it's pretty clear that hook_exit() is a way better developer experience than the event listener.

The one thing that has always confused me about hook_exit() though is that it runs for both cached and non-cached pages, and there is no way to run it for non-cached pages only (i.e., no end-of-request equivalent to hook_init()). So perhaps this is an opportunity to figure out how to distinguish them: What if we kept hook_exit(), but only had it run on non-cached pages; then if you want your code to run on all requests (like Statistics module) you'd use onTerminate() for that?

That would go along pretty nicely with #1833442: Remove hook_boot() (since I certainly don't think anyone has proposed getting rid of hook_init()).

I don't see where there is an alternative. We cannot ship with hook_exit() still in there.

Why can't we have a default system EventListener that calls hook_exit()?

* Such the code path would be the same
* The nice DX is kept
* The meaning of hook_exit slightly changes to not run on page cached pages
** (but that would happen anyway)

The "nice DX" of hook_exit() is a red herring, I think. I actually really like the listener model. It's a bit more lines of code, but cleaner.

The problem with hook_exit() is that, like any other hook, we have to be concerned about bootstrap state, code loading order, function_exists(), and all that jazz. An event listener has none of those issues. (OK, it has a slight problem super early in the boot up process vis a vis the config system, but that's not relevant to this issue.)

Moving the call to drupal_exit() inside a listener doesn't resolve even one of drupal_exit()'s problems. It just moves the problem around. It does not let us eliminate the concept of "bootstrap hooks" or "bootstrap modules", which we want to do in order to simplify the knots in the bootstrap process.

Sorry, slightly OT:

An event listener has none of those issues.

Sorry, @Crell, but I fundamentally disagree there — right now, event listeners are merely hyped as the new fancy tool, but reality is that their hard-coded priorities are going to bite us badly in the ass, unless we start to do something about them. We're not facing that problem right now, because our entire development operates in a one-dimensional environment — Drupal core is in charge of all events that are fired, and it is also in charge of the priorities (order) of all subscribed event listeners. Once you leave that one-dimensional environment and enter Drupal's modular 2-3 dimensions, you are in very deep trouble, because the hard-coded priorities are not at all designed for a modular/pluggable system. They are designed for a system that expects you to manually define custom weights for your particular application — which translates into hacking [core and contrib] code for Drupal. That's why we have configurable module weights as well as hook_module_implements_alter(), and that's why I've asked a couple of times in the past already what we're going to do about that.

You can define a "priority" on each event. That way you can control the order of each. The hardest problem is choosing the priority but I don't think thats more difficult than choosing a weight of a module.

In fact this is way more flexible as the priority is event based and not module based. Makes it possible for a module to push one listener to the front and another one in the back of the queue.

Symfony also has a limited dependency system for bundes. Each bundle can define a parent. This makes it possible to override stuff written in some modules bundle.

EDIT:
So you could overwrite symfony stuff classes defined by a module by your own implementation and change the priority. Dirty but it does the job. It's a bit like writing own views handlers to overwrite 2 lines of code that you want to change.

The "nice DX" of hook_exit() is a red herring, I think. I actually really like the listener model. It's a bit more lines of code, but cleaner.

That does not answer my suggestion to have a default listener that calls hook_exit from modules, but not as "bootstrap" hook, but just as a normal hook.

What is the problem?

Fabianx: That implies doing a "full bootstrap" for kernel.terminate events, because then drupal_exit() becomes a full, not bootstrap, hook. That makes the problem worse, not better, because it means even in a cached page we need to Load All the Codes in order to fire that one hook.

That depends on how we setup our index.php, I suppose. We can call terminate() on the Kernel from index.php, which will load up and call terminate() on the HttpKernel. From the looks of it that doesn't necessarily imply a boot(), but I'm not certain.

#21: So Boost for D8 would need to install its own exit_handler in PHP then? Or is Boost like functionality handled by HttpCache (or an injected HttpCacheExtension) instead?

Just thinking of an example here.

I still think its useful to have a hook to save data at session end, think of workbench, but it is okay if the hook is during the PageFlow request like hook_init() and not within bootstrap.

So: Removing hook_exit() all together - I am reluctant on that. Changing hook_exit() to run only in cases where hook_init() does also run (full page request): YES!

For non-cached pages, between the kernel.response event (runs before output is sent) and kernel.terminate event (runs after output is sent) I think we're covered so hook_exit() is no longer needed.

For cached pages, I can't think of any use cases for still always running extra code, and catch is correct that the less work we do there the better.

My vote is to kill hook_exit() entirely, and sort out exactly where and when terminate fires when we do the HttpCache conversion and can see and touch the problem space better.

I am a bit confused by this issue. With hook_boot() and hook_exit() potentially going away, does this mean that a Drupal website cannot do any "extra" processing before/after serving a cached page to an anonymous user/bot?

Thanks @catch. So settings.php code can be used for the 'before' extra processing. I did not realize that.

Is the recommended way to do the 'post' anonymous user processing logic by sending the data to server via a separate AJAX call/request? Thanks in advance.

Someone needs to update the patch from #7 to tell hook_exit() to exit...

You can define a "priority" on each event. That way you can control the order of each. The hardest problem is choosing the priority but I don't think thats more difficult than choosing a weight of a module.

In fact this is way more flexible as the priority is event based and not module based. Makes it possible for a module to push one listener to the front and another one in the back of the queue.

You're comparing to module weights, but what about hook_module_implements_alter()? That's already a lot more flexible than priorities, and some hook_exit() implementations may require that flexibility.

***

Anyway, it seems like there's definitely consensus in this issue to remove hook_exit() on cached page views, but no consensus on the rest.

Also, I looked into the code a little more, and as far as I can see, what @Fabianx suggested in #13 is actually very close to what the code is already doing (see Drupal\Core\EventSubscriber\RequestCloseSubscriber - this code only runs for non-cached pages and contains an onTerminate event which is where hook_exit() is invoked from on those pages). So all we should really need to do here is remove the other place where hook_exit() is invoked for cached pages.

A patch that did that might be useful for comparison purposes.

Something like this, for example. The patch contains a test to verify that hook_exit() is not being called on cached pages.

The only problem is that the statistics stuff will fail; as written, statistics_exit() currently does expect to be run even for cached pages (when it writes to the access log). I'm not sure if there's an issue somewhere to move the access log stuff to an Ajax callback like was already done for the node counter in #1209532: Count node views via AJAX in the statistics module.

Then not using a hook and relying on onTerminate() would not be a regression then anyway, no?

Looks like I forgot to remove 'exit' from the list of bootstrap hooks. This reroll fixes that. It will still fail tests due to the Statistics thing.

That issue with hook_module_implements_alter() is just weird. As far as I can tell, hook_module_implements_alter() does work on hook_exit(), but only for uncached pages. So the hook can actually be invoked in a different order depending on whether it's a cached page view or not! That's pretty broken (and will be "fixed" by this issue either way). I think it means that if your module's implementation of hook_exit() only cares about acting on uncached page views (as many do) then hook_module_implements_alter() actually does work just fine for you currently, though.

It's not going to pass, but just to see where it's at...

How onTerminate() will affect a sub-requests?

onTerminate() is only called after the master response has been sent at the end of index.php (or whatever utility function that was just split off to). If you fire a couple of subrequests in one request, onTerminate() is only called once when the PHP process itself ends. So to #39, "it doens't".

I'm impressed that this still applies. :-) That would make #35 commitable, no?

Rerolled.

needs one more reroll after #1833442: Remove hook_boot() got in

Hmm i renamed HookBootExitTest.php to HookExitTest.php as well..it should be done in the other issue..dunno if i should post it as followup there or here, i guess its fine

Change notice is here: http://drupal.org/node/1911186

I didn't spend much effort on it, because I opened up #1911178: Remove hook_exit() as a follow up. As that issue notes, I now see exactly one place that hook_exit() is called, which is by the terminate event. That makes it entirely vestigial at this point. We can discuss that in the linked issue, though.