Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
hook_init() has been the Drupal equivalent of Symfony's kernel.request event. We don't want two ways to achieve exactly the same thing, so similar to #1911178: Remove hook_exit(), we should remove hook_init() and convert all implementations to event subscribers.
Comment | File | Size | Author |
---|---|---|---|
#66 | drupal-hook-init-1911728-65.patch | 38.29 KB | ParisLiakos |
#66 | interdiff.txt | 1.03 KB | ParisLiakos |
#61 | drupal-hook-init-1911728-61.patch | 38.35 KB | ParisLiakos |
#61 | interdiff.txt | 1.1 KB | ParisLiakos |
#59 | drupal-hook-init-1911728-59.patch | 38.35 KB | ParisLiakos |
Comments
Comment #1
XanoComment #2
XanoAll implementations of hook_init() are located in .module files, with phptemplate_init() and twig_init() being two notable exceptions. Can theme engines have bundles?
The patch converts most implementations to kernel.request listeners. Let's see if I have taken the correct approach, before I go any further.
Comment #3
dawehnerGreat progress so far!
Does that mean that you can no longer use theme() in the request events?
ups, don't change the namespace, as the file wouldn't be found anymore.
I guess we should keep this information, as it's helpful.
The codestyle says "Contains \Drupal\..."
Now that you have a subscriber you can inject the actual request into that object in the container, see some of the core module bundles as example.
Is there a specific reason for the weight? If we don't need it, let's skip it.
Another example where you could inject the user.data service into the class.
This should have a Implements EventSubscriberInterface ...
Missing starting "\"
Maybe document the subscriber here as well?
We should keep this part of the documentation somewhere in the overlay.
We should have a test that checks that a css can be added at that point.
There is a service in the container for state, which you could use.
Comment #5
Xano\Symfony\Component\HttpKernel\KernelEvents::REQUEST says that the event is fired "at the very beginning of request dispatching", which to me indicates that all code has been loaded already, which would include theme().
I haven't been able to confirm a container is available in subscriber classes. If they are, can you point me to the documentation that describes this?
I believe a more generic test to see if all code has been loaded is more appropriate. kernel.request should be after after loading code, and before dispatching the request, but it's Drupal core that has to make sure that the code has indeed been loaded. This is not specific to adding CSS.
Comment #6
dawehnerSo let me introduce you to the wonderful world of the dependecy injection container. Take the UserBundle one as example:
So what you see on the UserData class is
so the actual class never knows of the container, and some magic in the container allows you to have all the dependencies in place.
This could be done in all this places as well.
Comment #8
dawehnerLet me show you what i mean.
Comment #9
Xano@dawehner: thank you!
This patch injects services into subscribers, instead of making them use global functions to get services.
Comment #11
XanoRename \Drupal\Core\DependencyInjection\UpdateBundle to \Drupal\Core\DependencyInjection\UpdateAPIBundle, and add \Drupal\update\UpdateBundle for update.module. This should solve a couple of test issues.
Comment #13
XanoEverything works locally, except MenuTrailTest, which expects a redirect to 403 and 404 pages. However, IIRC, we shouldn't redirect to such pages, but just display their output on the requested page. Is that correct?
Comment #15
dawehnerShouldn't we update these kind of documentation? No idea but dropping this information could be wrong.
We might should discuss about different kind of names, but yeah UpdateAPI is not too bad.
http://drupal.org/node/1666604 gives you a nice way to provide diffs for renamed files.
wrong indentation.
It's important to not name the services here, as the key concept of the DI is to not couple objects with the outside world. Use better something like "The uses data object."
Constructs an OverlaySubscriber object.
Should be better "public function __construct(..."
You forgot to use the userData service from that object.
Trailing "\"
"\"
See before, sorry.
It's just nice to see proper injected stuff!
Missing "\"
Comment #16
chx CreditAttribution: chx commented> hook_init() has been the Drupal equivalent of Symfony's kernel.request event.
That's one bold statement! Are you sure? I still can run
drupal_bootstrap(DRUPAL_BOOTSTRAP_FULL
and not run the kernel handler if I want... well, anything that doesn't involve processing a request much.Comment #17
Xanohook_boot() used to be the event hook to let modules act on bootstrapping. It was purposefully deleted in #1833442: Remove hook_boot(). The hook_init() documentation explicitely states This hook is run at the beginning of the page request., which makes it sound very similar to Symfony's kernel.request, which The REQUEST event occurs at the very beginning of request dispatching.
However, traditionally, hook_init() was invoked at the end of doing a full bootstrap. At the time of writing, it is both invoked there, and in \Drupal\Core\EventSubscriber\LegacyRequestSubscriber::onKernelRequestLegacy(). The latter invocation actually happens when (drumroll) kernel.request is fired.
Regardless of the differences between hook_init() and kernel.request, based on that last discovery, I believe there should be no technical problem at all converting hook_init() implementations to kernel.request listeners, as all current implementations are already executed as part of a kernel.request listener anyway.
Comment #18
chx CreditAttribution: chx commentedHm, I made a mistake. Please, please disregard my comment, sorry, I overstepped my boundaries.
Comment #19
XanoRegarding the TrailTest failures: the test expects the trail to be that of the requested page when handling 403s and 404s. However, 403s and 404s are handled by subrequests in ExceptionController, which resets the menu's active trail before handling the subrequests, causing the trail to be recalculated based on the subrequest, rather than the original. I just haven't found out yet why this is no problem when the patch is not applied.
Comment #20
sun@chx's remark in #16 is valid and correct. However:
Bundle::boot()
is the newhook_boot()
/hook_init()
. We merely have an architectural problem in DrupalKernel right now, as it extends Kernel, but our overridden implementation breaks a range of essential bundle concepts from Symfony. As a result,Bundle::boot()
and other Bundle methods are never called.That's a separate issue though — we've to refactor DrupalKernel as well as CoreBundle in relatively significant ways.
Long story short:
- Kernel +
Bundle::boot()
andBundle::shutdown()
are the newhook_boot()
andhook_exit()
(request-agnostic; uncached).- HttpKernel Request events are the new
hook_init()
andhook_exit()
(request-specific; cached).Consequently, removing
hook_init()
looks fine to me. There aren't many (appropriate) use-cases for it to begin with.Comment #21
XanoI don't know why TrailTest fails after the patch, but not before. I *do* know this is because of
drupal_static_reset('menu_set_active_trail')
in ExceptionController, which is a hack to solve a missing localized_options item somewhere in the breadcrumb system. Does anyone know if there is an issues about this particular problem?Comment #22
sunAs it seems like #20 wasn't completely clear to everyone, please note this additional discussion:
#1911178-52: Remove hook_exit()
Comment #23
catchNearly every hook_init() implementation is just the first hook picked that runs on every request, it doesn't actually need to run directly on full bootstrap before other stuff, and it doesn't need to run on non-HTML requests.
Converting all of those (including the test implementations) to a request listener is not an improvement at all. Instead we should look for more appropriate hooks.
The test implementations that log to watchdog or run drupal_set_message() could happen in hook_page_build() for example.
Comment #24
Crell CreditAttribution: Crell commentedNothing prevents a request or response listener from only doing anything on html responses. The request content type is easily available, and an if (not a type I care about) { return; } is easy to write. Certainly no harder than in a D7 hook.
That said, it's certainly valid to move code to a more sensible place than hook_init/kernel.request if there is a more sensible place. That doesn't mean hook_init() isn't still redundant.
Comment #25
catchYes I'd like to remove it as well, but I don't want the change notice for this to be "put all your stuff that was randomly put into hook_init() indiscriminately into a request listener instead", there are many more alternatives than that which are more appropriate - including other hooks.
Comment #26
Crell CreditAttribution: Crell commentedFully agreed with #25. So let's move forward with removing this and hook_exit(), and then make sure the change notice(s) are clear that there may be better approaches than start/end hooks/events for many use cases.
Comment #27
catchThat applies to the patch here as well though, not just the change notice.
Comment #28
Damien Tournoud CreditAttribution: Damien Tournoud commentedActually, most of those seem legit. There are two main issues that I can see, but they should not block this patch:
$request
directly and as such stop breaking the encapsulation.Those two are good follow-ups. Let's remove the hook first, and then fix the mess that the hook was in the first place. If we don't do that, we are never going to remove the hook, and arguably that would make the situation worse. This patch didn't create the mess, it is just replacing one hook with poorly defined semantics (does hook_init() run during the bootstrap or at the beginning of the request processing, or is that the same thing? or isn't it?) with a event that has a clear definition. This is a win on its own.
Comment #29
catchIt's considerably easier to change from hook_init() to hook_page_build(), than to change from hook_init() to a request listener. I don't see any reason to have to open a bunch of issues to move all those implementations a second time.
Comment #30
Crell CreditAttribution: Crell commentedEr, hook_page_build() is not long for this world between SCOTCH and WSCCI, no?
I agree with Damien. API change first, non-API-changing refactoring second.
Comment #31
catchWhen it's removed, stuff that's in there can be moved, I bet it won't be to onRequest.
Let's look at one example from this patch
This is exactly what people are complaining about on the hook_exit() issue, except there is absolutely no reason to move to the event in this case anyway. Unjustifiably adding those 30 lines of boilerplate is only going to increase the irksomeness whenever there's a case that's actually justified.
Comment #32
effulgentsia CreditAttribution: effulgentsia commentedI posted a patch in #1954892-10: Replace 'theme callback' and hook_custom_theme() with a clean theme negotiation system that moves drupal_theme_initialize() and module_invoke_all('init') from the legacy request subscriber to the HtmlPageController.
I agree with the premise of this issue that generic (non-html) initialization, such as system_init() can and should be migrated to request listeners.
However, per #23, most hook_init() implementations are used as something that should be called prior to the 'page callback' / _content controller when an HTML response (or AJAX response wrapping HTML content) is being built. In cases where the "prior to" part isn't a requirement, we can move to hook_page_build(), but in principle, we should have some support for a "prior to" version of hook_page_build(). In that patch, I retained hook_init() to serve that purpose, though we can rename it to hook_page_init() or something like that if it would help.
Should we deal with renaming that hook as a separate issue, and make this issue just about identifying what else besides system_init() should be moved into a request listener?
Comment #33
Eronarn CreditAttribution: Eronarn commentedSomething to think about with request listeners is that they can stop propagation, preventing other responders from receiving the event. This can be a huge difference from hook_init. We should probably make it clear via documentation that A) your module should avoid stopping propagation B) your module isn't guaranteed to get a bite at the apple, unlike hook_init (barring redirects/crashes).
Comment #34
ParisLiakos CreditAttribution: ParisLiakos commentedwell, seems that only system module's implementation needs to be a listener..not sure yet about overlay, i ll ask bot
Comment #36
ParisLiakos CreditAttribution: ParisLiakos commentedsigh
Comment #38
catch@ParisLiakos, thanks! Looks much better. And yeah one request listener is about the most I expected.
@effulgentsia: a 'pre page callback' hook seems sensible but I think we could add that elsewhere.
Comment #39
ParisLiakos CreditAttribution: ParisLiakos commentedok so..i had to move the overlay_init to the event subscriber...mainly because it redirects using drupal_goto...that would help the RedirectResponse issue..so:
You want to mess up with request/response?
Use an event listener!
You want to add css/js, set messages?
Use hook_page_build!
You want the first hook that fires to do random stuff, because, you know, you have a test module and cant be arsed to create a listener...?:P
Use hook_custom_theme!
this should be green
Comment #40
attiks CreditAttribution: attiks commentedSome minor issues
comment too long
Html -> HTML
comment too long
Comment #41
ParisLiakos CreditAttribution: ParisLiakos commentedfixed comment wrapping
Comment #42
Crell CreditAttribution: Crell commentedThanks, Paris!
Comment #43
ParisLiakos CreditAttribution: ParisLiakos commentedReroll for the language constant commit
Comment #44
alexpottNeeds reroll
Comment #45
jibranreroll
conflict
fixed
Comment #47
ParisLiakos CreditAttribution: ParisLiakos commented#45: drupal_1911728_45.patch queued for re-testing.
Comment #49
jibran#45: drupal_1911728_45.patch queued for re-testing.
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
I installed locally it worked.Comment #50
David_Rothstein CreditAttribution: David_Rothstein commentedThat seems like it needs a code comment (at the very least)...
Something's wrong there since the comment is still referring to hook_init(). Also, hook_page_build() doesn't sound right since doesn't it run later in the page request, not earlier?
Shouldn't this test be refactored rather than removed? We still want to make sure that the theme system can be initialized early in the page request...
Something is wrong with the spacing and grammar here. Should be: "When this flag is set, various operations do not take place, such as css/js preprocessing and translation."
Comment #52
jibran#45: drupal_1911728_45.patch queued for re-testing.
Comment #53
jibranAs per #50
Comment #54
ParisLiakos CreditAttribution: ParisLiakos commentedthanks for the review!
Fixed most of the above besides
I am not sure how to refactor this test..
test that an event subscriber that fires before
Drupal\Core\EventSubscriber\LegacyRequestSubscriber
can call theme() successfully?it doesnt make any sense to me.
Comment #55
Crell CreditAttribution: Crell commentedWe're already working on changing how theme initialization works anyway, so let's not try to re-design tests for something we want to change in the first place.
Comment #56
David_Rothstein CreditAttribution: David_Rothstein commentedWell, what's the earliest that it's safe to call theme() now without blowing things up (like Drupal 6 used to blow up)? That's the essential question the test is trying to answer...
If you have to give your subscriber a particular random priority (relative to LegacyRequestSubscriber) in order for a theme() call to work I can appreciate how that's not really this issue's problem to solve, but it might be this issue's problem to document... and either way the test still shouldn't have to be removed (it could just get a hardcoded priority and a @todo). It still make sense to have a test showing that the theme system can be initialized early.
Also, the test code being removed in this patch includes this as well:
Which is a different issue entirely, and removing it leaves _theme_test_load_registry() around as a dead function. I don't fully understand what it's doing though... Git blame said it was added in #812016: Themes cannot always participate in drupal_alter() and the code makes sense if you read it in that issue, but it may have been corrupted since then.
Comment #57
David_Rothstein CreditAttribution: David_Rothstein commentedThis patch adds the test back as an event listener. It actually seems to work regardless of the ordering with respect to LegacyRequestSubscriber.
I didn't do anything to deal with the _theme_registry_callback() thing though; that's still an issue.
Also, I noticed while putting this together, in one of the other event listeners:
Is this actually inheriting the doc from anything?
Comment #58
ParisLiakos CreditAttribution: ParisLiakos commentedno, its just a copy paste failure.
Let me expand on my meaning
on #54
but since we need to move this issue forward, i am fine with anything so lets fix the following and move on:
should be the full namespace with preceeding backslash
just noticed i forgot to update this to the event's request object
Also, this wrong inheritdoc should be fixed and _theme_registry_callback thing figured out.marking NW for that..
Comment #59
ParisLiakos CreditAttribution: ParisLiakos commentedComment #61
ParisLiakos CreditAttribution: ParisLiakos commentedyeah, that was stupid of me
Comment #63
ParisLiakos CreditAttribution: ParisLiakos commented#61: drupal-hook-init-1911728-61.patch queued for re-testing.
Comment #64
David_Rothstein CreditAttribution: David_Rothstein commentedShould probably get rid of the $args variable itself also, since it's not used...
Comment #66
ParisLiakos CreditAttribution: ParisLiakos commentedtrue that, removed
seems bot is having issues
Comment #67
Crell CreditAttribution: Crell commentedI think this addresses all of David's concerns, so let's try this again.
Comment #68
David_Rothstein CreditAttribution: David_Rothstein commentedOnly skimmed it over, but yes, this version looks good to me too.
Comment #69
alexpottCommitted 42daa83 and pushed to 8.x. Thanks!
Comment #70
ParisLiakos CreditAttribution: ParisLiakos commentedchange notice here https://drupal.org/node/2013014
feel free to improve!