Remove hook_exit()

Working on a patch.

I don't think we should do this for two reasons already discussed in #1860026: Remove hook_exit() for cached page requests (which others in that issue agreed with also):

1. We lose the ability to use hook_module_implements_alter().
2. We lose the nice parallel with hook_init() (one runs at the beginning of non-cached page requests, once Drupal is fully loaded, and the other runs at the end of non-cached page requests, when Drupal is done).

overlay_exit() seems to be the only implementation of hook_exit(). I converted it to Drupal\overlay\EventSubscriber\OverlaySubscriber::onTerminate(), and I removed references to hook_exit() in documentation, and the invocation in Drupal\Core\EventSubscriber\RequestCloseSubscriber::onTerminate(). This is my first attempt at writing an event subscriber, so I hope I understood how to work with those correctly.

Looking at overlay_exit(), I tried mapping it to a response listener, not terminate, since it needs to mess with the session. I think that's actually more accurate in this case.

You are right. I changed that in this patch.

hook_init(), IMO, should also be removed in favor of just using kernel.request.

+1

There were some test errors related to parsing the output of Simpletest browser requests, which I couldn't quite figure out. I also need to find a way to run these tests locally on a Git clone (without having to install Drupal into that folder, if that is possible).

// Edit: The Simpletest browser errors occur when testing #1911260: Move PermissionAccessCheck to User.module as well, so they may not be related to either issue.

Removing hook_init() with hook_exit() as well makes a lot of sense
+1 on both

It might be worse DX but, thats how we do things now that we are a symfony application..there is no need to add extra layers.
Also that would actually make some people learn about the kernel and event listeners with an easy way

also see #1905334-8: Only load all modules when a hook gets invoked

See #1911728: Remove hook_init().

#6: drupal_1911178_01.patch queued for re-testing.

#6: drupal_1911178_01.patch queued for re-testing.

Failures due to #1912028: CKEditorPluginManagerTest fails because plugins are discovered in slightly different order

#6: drupal_1911178_01.patch queued for re-testing.

#1912028: CKEditorPluginManagerTest fails because plugins are discovered in slightly different order committed

#6: drupal_1911178_01.patch queued for re-testing.

-    $this->assertLogMessage('system', "%module module disabled.", array('%module' => $module), WATCHDOG_INFO);
+    if ($module != 'dblog') {
+      $this->assertLogMessage('system', "%module module disabled.", array('%module' => $module), WATCHDOG_INFO);
+    }

I am not sure, how this worked before..when dblog is disabled we should not have an entry in the database from itself..since..it is disabled?

Looks also good to me.

-function drupal_exit($destination = NULL) {
+function drupal_exit() {

Let's not remove a parameter without also updating the callers.

Also, we should get to the bottom of this:

-    $this->assertLogMessage('system', "%module module disabled.", array('%module' => $module), WATCHDOG_INFO);
+    if ($module != 'dblog') {
+      $this->assertLogMessage('system', "%module module disabled.", array('%module' => $module), WATCHDOG_INFO);
+    }

I feel that testing for log messages was a horrible idea in the first place. Instead, that should have been done in a dblog test. That is, however, something for a different issue, so the solution that only checks for log messages if dblog is not the module that is being disabled makes sense. Like @rootatwc I don't quite know why it didn't fail before applying this patch, though.

Latest patch forgot to remove HookExitTest.php.

I would really like to know why the test in EnableDisableTest now fails. We reset the implementation cache *after* the logging call, so this should not fail.

Well, i seriously have no clue what happens and i cant debug it..EnableDisableTest gives me many failures and exceptions in my PHP 5.4.4, even if bot say its green, i had to actually run this test by hardcoding module to enable to array('dblog', 'simpletest') to track down what goes wrong..i am gonna try to debug later, but i keep my expectations low:)
in a quick manual test though:
Before the patch:
-Dblog, is disabled: No entry in database
After patch:
-Dblog, is disabled: No entry in database

So, seems this test was failing to catch the failure before...not quite sure what hook_exit has to do with it

@Xano, the current test is not testing dblog functionality; it's testing watchdog() behavior during module operations using dblog as a proxy for that. (It could of course use a dedicated testing module.)

I am able to reproduce the failure locally. Unlike @rootatwc though I'm seeing the expected behavior with a manual test. Here's what I see:

1. Either with or without patch, disabling the module manually via the user interface produces the watchdog message, as expected (note this applies to Syslog module too).
2. The test, however, only fails with the patch.

Thus, it seems to be a tests-only issue and could be some kind of preexisting static caching bug that is only revealed here... although for the life of me I can't figure out why.

If it helps, I found that hacking the test to contain $modules = array_intersect_key($modules, array_flip(array('node', 'datetime', 'simpletest', 'dblog'))); before running them is sufficient to speed it up significantly while debugging this. I am not sure why 'node' and 'datetime' were needed, but without them I saw local test failures too. This is PHP 5.3 though.

FYI, minor point: after applying the patch there is a reference to overlay_exit() remaining (just in a code comment).

David: hook_module_implements_alter() is an abomination to begin with. :-) However, with compiler passes you can accomplish all the same things: You can set the priority for your listener to be before or after an existing listener. You can remove an existing subscriber definition. You can do all of the things you could do before.

It looks to me like you can remove existing ones easily, yes. However, as far as I can tell, reordering via hardcoded priorities does not have the same flexibiity or robustness that hook_module_implements_alter() provides. Suppose Module A wants to run after Module B, and Module B has a priority of -5, so Module A sets its own priority to -10 to accomplish that. That all works well until Module B decides it needs to run after Module C, and bumps its own priority down to -15 as a result. That means Module A will now break.

I believe that's what @sun was getting at with his comment at #1860026-15: Remove hook_exit() for cached page requests. As far as I can see, there is no good answer to that yet...

-  // it to the same content rendered in overlay_exit(), at the end of the page
-  // request. This allows us to check if anything actually did change, and, if
-  // so, trigger an immediate Ajax refresh of the parent window.
+  // it to the same content rendered in OverlaySubscriber::onResponse(),
+  // at the end of the page request. This allows us to check if anything
+  // actually did change, and, if so, trigger an immediate Ajax refresh
+  // of the parent window.

Thanks for clarifying, @David_Rothstein - that's more or less what I meant.

@Crell: In the past, I commonly referred to this problem space as 2D vs. 3D — within core, we tend to think about the core vs. contrib space only. However, the reality for contributed module developers is more than "me vs. core". It is actually enhanced by another dimension, it's "me vs. core vs. other contrib". At times, I also referred to 4D, since "other contrib" actually means "random other contrib". The difference is the detail that "other contrib" is undefined. You do not (and should not have to) know with which other modules you're competing with. I've the impression that you're trying to brush that reality aside by insisting on "there's only core vs. contrib", and all I can say is that we'll run into major architectural problems in the real world of contrib and customizations, if we don't address the multidimensional problem. Whether the required solution is called a "promise-based approach" or a "modular event handler" is a theorem we shouldn't care about.

I agree that it is off-topic here. I already confirmed the original RTBC here and expressed my support for these hook_init() + hook_exit() changes in #1911728-20: Remove hook_init(). Nevertheless, yes, I'd have a lot to reply to #32 and will continue to raise awareness for the architectural problem space. :)

Back to this issue, I suspect that the dblog watchdog hook implementation invocation difference is caused by architectural differences between ModuleHandler vs. DrupalKernel (or actually HttpKernel). The hook registry mostly operates on runtime memory, whereas HttpKernel events are built/compiled in. Former hook_exit() and request-termination listeners operate in different scopes. I'm running http://drupal.org/node/1766036#comment-7076866 in parallel as one of two possibilities to validate that suspicion, but a >1.5h total test suite time in combination with feature freeze doesn't fly so well, so still waiting for that patch to actually get picked up for testing... ;)

Finally some results, and I'm relatively sure that this is the test failure you're looking for:

#1766036-44: ♪♫ Question the sun ♥, the moon, and the stars ♫♪ [Test that stuff.]

Exactly 1 fail in EnableDisableTest, caused by one module that is disabled. The only module for which we wouldn't find a watchdog record is dblog module.

Apparently, some other issues ran into that specific problem as well, so there happens to be a dedicated issue (and a more complete patch) for it now:
#1929656: ModuleHandler::setModuleList() should call resetImplementations() in order to avoid edge cases

#30: drupal-remove_hook_exit-1911178-29.patch queued for re-testing.

Patch is still green, i guess now we have #1929656: ModuleHandler::setModuleList() should call resetImplementations() in order to avoid edge cases this can be rtbc'ed again?

All that said... it's irrelevant. That's a hook-vs-event question. We've already ripped out other hooks that now have equivalent events, and the trade-off is the same. This is no different. I don't know why we're making such a big fuss about it here, when hook_book and hook_init vs. kernel.request has the same edge-case differences.

I can't find other examples at the moment except hook_boot() (which is a special case and wasn't even a real hook) and maybe hook_url_inbound_alter() (kind of an edge case also)... Otherwise, doing it for hook_exit() (and hook_init() as proposed in the equivalent issue) seems to be the first place where it has been proposed?

Although hook_module_implements_alter() is not as important for these hooks as it is for ones like hook_form_alter() (where it has become essential), we're still talking about losing flexibility and introducing inconsistencies. How do you explain to someone why Drupal chooses to do some things as hooks and others as events?

I can sort of see how this helps #1905334: Only load all modules when a hook gets invoked, but only tangentially. It makes it harder for people to write code that runs on every request and uses the full Drupal API. But ultimately, they can do it either way - and whether or not a site has any code that chooses (or needs) to have all modules loaded is the determining factor.

@Crell the issue is still there but we now have #1929656: ModuleHandler::setModuleList() should call resetImplementations() in order to avoid edge cases which fixes it...the two patches even share some lines of code (#18). This issue is very RTBC from technical perspective

#1929656: ModuleHandler::setModuleList() should call resetImplementations() in order to avoid edge cases

Sorry Crell, seems i was lying, the other issue is major
here is the patch without the conflicting code..for when the other issue is committed

other patch committed

+use Symfony\Component\HttpKernel\Event\FilterResponseEvent;

+class OverlaySubscriber implements EventSubscriberInterface {

+  public function onResponse(FilterResponseEvent $event) {

What's a FilterResponseEvent, and why does this event subscriber use that?

@sun See Symfony\Component\HttpKernel\KernelEvents::RESPONSE.

Failed at Drupal\system\Tests\Bundle\BundleTest..me smells random failure

#43: drupal-remove_hook_exit-1911178-43.patch queued for re-testing.

Otherwise, doing it for hook_exit() (and hook_init() as proposed in the equivalent issue) seems to be the first place where it has been proposed?

Yes that's right. hook_boot() wasn't removed in favour of an event, it was just removed as not making sense any more.

Needs more discussion. At least two of the things in #30 that supposedly can't be done without this I'm pretty sure can.

Well, the DX of writing a request event listener is certainly worse compared to writing a hook implementation.

However, I believe that this change will make sense, if we also fix our Kernel implementation. It helps to clarify and differentiate when exactly these events and code paths are getting invoked.

Quoting myself from #1911728-20: Remove hook_init() (shortened):

Our DrupalKernel is bogus currently, because unlike Kernel, it does not invoke Bundle::boot() and other Bundle methods.

The new clear differentiation is or should be:

1. Kernel + Bundle::boot() and Bundle::shutdown() are the new hook_boot() and hook_exit() (request-agnostic; uncached).
2. HttpKernel Request events are the new hook_init() and hook_exit() (request-specific; cached).
3. Note: There's an additional Bundle::terminate() (request/response-specific), but if I'm not mistaken, Symfony is discussing to remove the current tight integration/dependency between Kernel and HttpKernel.

Consequently, converting hook_init() and hook_exit() looks fine to me — there aren't many use-cases to begin with. That is, if we also fix our Bundles.

Bundle::boot() gets the DI container injected into the Bundle class before it is invoked, so technically you should be able to do whatever you want? In the same way you were able to do whatever you want in hook_boot(). Both with the same limitations or environment specifics. Am I missing something?

Likewise, Bundle::shutdown() (just a few lines below) is doing the reverse procedure; first shutdown the Bundle, then destruct the injected container.

I'm not sure exactly how that relates to hook_exit being needlessly redundant.

The point I'm trying to make is that the current hook_exit() should be replaced with:

1) Request onTerminate event listeners
2) Bundle::shutdown()

So, yes, hook_exit() is actually redundant on multiple fronts. Also, unlike these two concepts, its purpose and scope is not clear. Instead, these two concepts are very clear:

2) Bundle::boot() and Bundle::shutdown() are essentially invoked whenever a Kernel instance is bootstrapped or shut down. This includes stuff like command line scripts. That's the direct equivalent of one half of former hook_boot() and hook_exit(), which equally was invoked for every Drupal bootstrap.

1) Former request-aware hook_boot(), hook_init(), and hook_exit() operations are moved where they belong: Request event listeners. That makes their scope and environment, and their code execution paths crystal clear.

This looks pretty straightforward to me.

However, please note that I linked to Symfony's Kernel in #55 — our overridden DrupalKernel implementation does not currently account for these fundamental Bundle concepts; i.e., Bundle::boot() and Bundle::shutdown() are not invoked by DrupalKernel currently.

In general, I think we've overlooked and missed quite a couple of essential Bundle concepts in our architecture (due to Symfony's lack of architectural docs). I wouldn't even be surprised if the new DestructableInterface that we're currently inventing turns out to be unnecessary and obsolete.

Agreed! Tentatively moving back to RTBC, as I hope that this additional discussion helps and is sufficient.

That would revert #1716048: Do not boot bundle classes on every request.

At least two of the things in #30 that supposedly can't be done without this I'm pretty sure can.

i am sure #30 is the wrong comment number..

I dont understand though, what we need to talk about..
Dont we all agree here that onTerminate == hook_exit? Yeah its DX might be a worse, but we ca'nt have two ways of accomplishing the same thing.
Alse fixing Bundle:boot() and Bundle::shutdown() is not a requirement for this issue

Hmm yes you're right, I meant #32.

- Forever dooming #1905334: Only load all modules when a hook gets invoked to be impossible, meaning we cannot ever serve a request without bootstraping the whole module system.

That issue almost works already. We'd need to only load modules when a hook is implemented as opposed to simply invoked if we keep any 'every request' hooks. What removing hook_exit() would save is having to check the module_implements cache at all though.

- Making it impossible to ever remove drupal_container(), because hook_exit() doesn't have injected DIC access.

There's no reason we couldn't do hook_exit($container).

I've got very little interest in further cementing our use of bundles, if we could find a nicer way for modules to register stuff to the DIC I'd actually prefer to get rid of them from modules altogether. If that's the 'DX' answer to this issue it needs to stay open 'cos there's no point doing this then making it even worse later.

Yes using bundles is fairly cemented in core but I've never liked it and if we could find a decent way to roll that back before release I'd be very happy. If not that might end up being a project for Drupal 9.

#57 is suggesting we start using the magic bundle methods for DX reasons, something we explicitly decided against in #1716048: Do not boot bundle classes on every request. Checking whether a class exists with a file access then calling methods on it if it does is considerably worse than the hook system.

I don't particularly mind using onTerminate instead of hook_exit() but all the additional justification for it in this issue is just wrong at this point.

Opened #1911178: Remove hook_exit().

Not that one, #1939660: Use YAML as the primary means for service registration.

Dont we all agree here that onTerminate == hook_exit? Yeah its DX might be a worse, but we ca'nt have two ways of accomplishing the same thing.
...
OK... so "hook_exit can't do anything that onTerminate can't, let's eliminate hook_exit"... is that not sufficient justification?

"Two ways of accomplishing the same thing" can be interpreted in a couple ways. Unless we get rid of hooks entirely throughout all of Drupal (and yes, I know there's a proposed core conversation about doing that in Drupal 9), we will have two ways of accomplishing the same thing no matter what. Hooks and events serve the same basic purpose and we will have both in Drupal 8. But I'd much rather have harmless duplication for a couple hooks than to remove them and introduce confusion. We currently tell Drupal developers that when an event happens that you want to respond to, you should look for a hook to implement. Now we're going to say sometimes (usually) you implement a hook, but occasionally hooks won't work and you need to do something else entirely to respond to particular events? Why make it difficult like that?

Also, hook_module_implements_alter() is still an example of something you can do with hook_exit() but not onTerminate().

I haven't quite figured out what #1939660: Use YAML as the primary means for service registration would mean for all this. It would involve .yml files which would introduce some consistency with other things (such as routes). But I'm not sure if registering events for the beginning and end of the request is conceptually related to registering routes for particular pages.

I'm comfortable with this patch given that hook_exit() is almost never used.

given that hook_exit() is almost never used.

Well in the very small slice of contrib that is indexed by drupalcontrib.org, it's marked as used 18 times, which I wouldn't characterize as almost never: http://drupalcontrib.org/api/drupal/drupal%21modules%21system%21system.a...

But it does seem to have a replacement, as long as we document how to switch in a change notice without loss of functionality, including how to replace a hook_exit() that depends on $destination (invoked from a redirect), that seems ok with me. I see in the patch how a normal hook_exit() would get replaced, but not one that would have depended on $destination.

Just to be clear on hook_module_implements_alter(), that doesn't actually work for hook_exit() at the moment in Drupal 7, because it's invoked by bootstrap_invoke_all(), which ignores module_implements(), because module_implements() depends on a full bootstrap. That's one of several motivations for #1888734: Get rid of all 'bootstrap' hooks although I consider that issue to be orthogonal to this one (i.e. we could just load all modules earlier and not call hook_boot() for pages served from internal cache).

1. If you're looking at frequency of usage, don't forget this isn't just about hook_exit() but also hook_init() (see parallel issue: #1911728: Remove hook_init() - the two issues come as a pair).

2. Just to be clear on hook_module_implements_alter(), that doesn't actually work for hook_exit() at the moment in Drupal 7, because it's invoked by bootstrap_invoke_all(), which ignores module_implements(), because module_implements() depends on a full bootstrap.

   That is only true if you care about cached page requests (which as we know aren't reliable anyway). If your hook_exit() implementation only does anything meaningful on uncached requests, then hook_module_implements_alter() works fine for you in Drupal 7 and would in Drupal 8 too.

The Overlay hunk in here still makes me uncomfortable, thanks to David who linked to #1911728: Remove hook_init() since that reminded me why (and see comment on that issue for details).

Overlay does not need to run on a generic request listener, it just needs to run at the very end of serving an HTML response (not any other kind of response).

Do we have an existing event or hook for that? If not let's add one and move it to there. Just replacing one much-abused hook for a much-abused event listener isn't going to get us anywhere.

I don't actually see in #1668866: Replace drupal_goto() with RedirectResponse how we're intending to replace drupal_exit($url) and there's no potential change notice queued up for documenting the change? It's tough since no D7 core implementations of hook_exit() used the $url parameter, but contrib does, so that's why maybe we're not making sure to consider it's replacement?

@Crell: Thanks, I would have never known that comment was how we're going to replace hook_exit($url) since it lacks context around it. I was looking for something more concrete since I was searching for "$url" and "hook_exit" in that issue and nothing was coming up relevant. Would it be ok if we had a change notice pre-written with actual examples for "Here's how you'd handle hook_exit() and here's how you'd handle hook_exit() when you want to inspect the redirect path?" The core conversions here help with the first one, but not with the latter. I think it's fine if we can write these change notices after the commit, but I'm still just concerned that we're replacing something but don't exactly know how to tell contrib developers how to replace it. And I may just be missing something larger, I dunno. :/

i agree with Crell that fixing overlay is out of the scope of this issue..
i dont think there is an event that just runs only for html pages ( i did not search though) and adding a new hook just for overlay does not makes any sense to me:)

just a reroll of patch in #43 which no longer applies

in #1668866: Replace drupal_goto() with RedirectResponse i converted overlay's hook_drupal_goto_alter to a response subscriber..so either way i think overlay gets an event_subscriber...unless of course we keep hook_drupal_goto_alter?
Anyway, i just had a flashback about this issue and i thought i should bump

also here is a reroll for services.yml instead of bundles

Hmm the overlay listener still looks nasty but given it's the only implementation here and there's no obvious place for it to live otherwise at the moment, I can live with committing it like this.

This will need a change notice, hopefully with more detail than 'meh, you can implement a response listener' ;)

Removing Tag.

This patch seems to have resulted in the overlay code never getting called in the exact situation where it needs to (pages that end in a drupal_goto()). See #1991008: Regression: Adding/removing a shortcut (or otherwise changing the toolbar) while inside the overlay doesn't update the toolbar.

My guess is that #1668866: Replace drupal_goto() with RedirectResponse would fix that, but I have no idea... If we keep what's here, does that issue need to be bumped to critical? Modules need a way to respond to all page requests, including ones that end in a redirect.