Remove hook_boot()

If hook_boot() is removed, will hook_init() be run on pages served from page cache?

That's the big distinction between the two right now in D7; just want to ensure that functionality will still exist.

Use-case is user auto-log in and/or provisioning in a single sign on system.

A user logged into a single sign on system independent of Drupal hits a Drupal page that happens to be cached. There needs to be a way to dynamically log in or provision a user (whether based on a cookie, GET/POST params, request headers, etc) regardless of the cached state of the Drupal page.

My organization's written a sub-module for simpleSAMLphp Authentication that hasn't been committed that performs this using an implementation of hook_boot().

+1 here. Pre-dispatching use cases (like the one described in #3) should be handled by a KernelEvents::REQUEST subscriber.

hook_boot() and the entire notion of bootstrap modules is indeed superfluous with the new kernel bootstrap — as soon as we've bootstrapped the kernel, we're essentially running the equivalent of the former/current hook_boot() phase with access to many services (though not all, since many services still call into procedural functions) and even with access to the module/hook system, but without any .module files being loaded yet (which ultimately makes a better differentiation than the current hook_boot()).

So yeah, +1 for removing hook_boot() and the entire bootstrap modules concept. Most developers do not understand the concept and the crippled environment of hook_boot() in the first place (and I can certainly remember all of the lessons I had to learn about hook_boot() The Hard Way™ some years ago).

hook_boot() implementations indeed show only Ban module (#1794754: Convert ban_boot() to an event listener) and test module implementations; I did not look into the latter, but I'm sure we can convert them.

I agree here as well. hook_boot()'s use cases now belong either in a high-priority kernel.request event or in a manual settings.php or similar. (It's reasonable to ask dev tool users to edit settings.php to enable a feature.) Now we just need someone to write it.

Well, what do you expect from the guy who added some conf overrides into Drupal 7 so he can disable hook_boot from firing? If I never see hook_boot ever again it's too soon.

i want to give this a go

@session tests: They will fail because i think i am adding the headers too late in response..This should be in KernelEvents::REQUEST to work..but i cant find a way to add headers there, since response is not initialized yet.
@translation test..this could be removed as well imo..not sure if is still needed..anyways i tried to convert it..not sure if it actually tests anything that matters.
@_drupal_bootstrap_page_header ...maybe completely remove it?

I was tempted start removing hook_exit and friends as well. but i see there are other issues for those

If you need to muck with the response headers from the request hook, then first of all I think you're doing something wrong. :-) That said, a single subscriber object can response to both a request event and a response event. The request listener can do what it needs to do and save data to an object property and then the response listener can check that object property and manipulate the response object accordingly.

Although I'd still question if the use case is even valid... It probably isn't, or could be done exclusively in the response listener since you have the request available to you there anyway.

yeah, i know this is wrong, but those tests where adding headers at hook_boot()..i did the correct thing, added those headers to a response listener, but 4 of 6 session tests that check this fail:/
see session_test_boot
anyway, i just woke up so probably i am not thinking clearly

That test is no longer appropriate and should either be removed outright or converted into just a response listener.

Patch attached:

• Removes translation_test - as I agree with @rootatwc - this is not doing anything now.
• Removes DRUPAL_BOOTSTRAP_PAGE_HEADER as this is pointless now
• Add @todo's to SessionTest so that once cached pages use Symfony events we can reinstate (if necessary)
• Add's creates LanguageTestManager service to test domain negotiation

Interdiff is a little difficult becuase I've renamed some of the files.

+++ b/core/includes/bootstrap.inc
@@ -2412,17 +2393,6 @@ function _drupal_bootstrap_variables() {
-function _drupal_bootstrap_page_header() {
...
-  if (!drupal_is_cli()) {
-    ob_start();
-  }

Do we need to move the output buffering to the next bootstrap phase as first operation, or is that [additionally] handled elsewhere in the sf HttpKernel?

Hmm, yes the session tests solution was pretty obvious..thanks alexpott.
Dunno about #16, though, tests in my localhost just passed

#15: 1833442-14.remove-hook-boot.patch queued for re-testing.

Re #16, I *think* we no longer need ob_start() - my understanding is that it was left in place to deal with page caching, because page caching used to use ob_get_clean() to get the page body, but it no longer does. I am confused about the failing session test as it passes locally for me too :-/

reroll after omit_vary_cookie converted to config

Rerolled against the settings() patched and cleaned up some minor code style issues.

Hmm, 2 extra test failures:/ sigh

+++ b/core/modules/system/tests/modules/session_test/lib/Drupal/session_test/EventSubscriber/SessionTestSubscriber.phpundefined
@@ -0,0 +1,59 @@
+ * Ban subscriber for controller requests.

Lets fix this next time someone rerolls:)

After quite some debugging the actual problem is that the LanguageTestBundle is not executed on the LanguageUILanguageNegotiationTest.
The reason was that language_test.info/language_test.module was in the wrong folder.

Fixed also some basic codestyle issues.

Wrapped failing test in a @todo just like the rest ones
lets see now

#28: drupal-1833442-27.patch queued for re-testing.

+++ b/core/lib/Drupal/Core/Language/LanguageManager.php
@@ -19,8 +19,8 @@
-  private $request;
-  private $languages;
+  protected $request;

I agree with this change, obviously, but it's not really on topic for this issue and will likely collide with #1899994: Disentangle language initialization from path resolution and similar.

Otherwise this seems fine to me, once testbot approves.

i hate upgrade path test failure...
@Crell: i ll remove it in a reroll later then
#28: drupal-1833442-27.patch queued for re-testing.

removed changes pointed in #32

Give hook_boot() das boot!

hook_language_init is dead, so i just removed changes there from the patch

FAILED: [[SimpleTest]]: [MySQL] Setup environment: failed to clear checkout directory.

bot's new thingie?

#38: drupal-1833442-38.patch queued for re-testing.

had to change TestLanguageManager as well.
come on bot

back to rtbc

Please don't set your own patches to RTBC. Even if it's small changes.

When removing ob_start() it seems needs to remove ob_get_clean()

Also it's not clear from issue summary - how page cache affected

+++ b/core/includes/bootstrap.incundefined
@@ -2441,17 +2423,6 @@ function _drupal_bootstrap_variables() {
-  if (!drupal_is_cli()) {
-    ob_start();
-  }

ob_start() is not there anymore?

@andypost see #20
Only instance of ob_get_clean() in core is in theme_render_template() which is not relevant

OK, I'll RTBC it then. :-)

yay! i ll work on a small change notice later

Change notice here
http://drupal.org/node/1909596
Corrections welcomed

Yay, glad to see this in :)

+++ b/core/includes/bootstrap.inc
@@ -1422,7 +1415,7 @@ function drupal_serve_page_from_cache(stdClass $cache) {
   // revalidation. If a Vary header has been set in hook_boot(), it is assumed
   // that the module knows how to cache the page.
-  if (!isset($hook_boot_headers['vary']) && !settings()->get('omit_vary_cookie')) {
+  if (!isset($boot_headers['vary']) && !settings()->get('cache.page.omit_vary_cookie')) {
     header('Vary: Cookie');
   }

1) I've the impression that the settings key name was mistakenly updated to use the config key name? The setting is still called 'omit_vary_cookie' in settings.php.

2) Minor: Stray "hook_boot()" in preceding code comment.

nice catch

status^^

two stray hook_boot comments actually:

# git grep hook_boot
core/includes/bootstrap.inc:  // revalidation. If a Vary header has been set in hook_boot(), it is assumed
core/modules/language/lib/Drupal/language/Tests/LanguageUILanguageNegotiationTest.php:      // language_test.module hook_boot() to simulate this.

#52: drupal-hook_boot_followup-1833442-52.patch queued for re-testing.

Would it be possible to add to the change record a couple of examples of the code to add in settings.php?

There is no standard format..a module might ask you to require_once a file and then run a function..another to just paste 3 lines...

#56 rtbc anyone?

Actually, does this need a test?

Opened #1919074: No test coverage for omit_vary_cookie for tests

Eh? Let's just do it here and be done with it so that we don't have to coordinate 2 patches.

I just think, writing tests for omit vary cookie is not in the scope of this issue. lets commit #56 and work on tests later, since i think vary cookie is broken now..
But feel free to close the other issue as duplicate, and reassign this issue as bug with Needs tests tag.

@rootatwc
I don't think that this bug-fix is something which have to be in ASAP, so what about have a proper test first, so we for sure know that it's fixed?
The good think about drupal core development is that you can stick to those rules, as you don't have real customers at the moment.

Mh I have sadly no idea how to write tests for that, even putting settings() into the test does not help here :(

added example to change record
needs review

My understanding is that the "before" and "after" code samples in the change record aren't actually equivalent since in Drupal 7 it will run on cached pages but in Drupal 8 it won't?

And that therefore, the exact equivalent of the Drupal 7 code would have to be something you paste into settings.php...

#71
it works on cached pages too!
from Symfony\Component\HttpKernel\KernelEvents

     * The REQUEST event occurs at the very beginning of request
     * dispatching
     *
     * This event allows you to create a response for a request before any
     * other code in the framework is executed. The event listener method
     * receives a Symfony\Component\HttpKernel\Event\GetResponseEvent
     * instance.

$events[KernelEvents::REQUEST][] = ...;

I just tested and that wasn't the case. (See, for example, #1929506: Banned IP addresses can still access cached pages, which is an interesting side effect of that.)

Also, I thought one of the goals here was to "force" people to use settings.php for that kind of behavior anyway? (Since with all the non-Drupal page caching mechanisms that exist these days, you can only reliably write that kind of code in a site-specific scenario anyway.)

#73
so
what is the right solution for that? (

Just done manual testing and got a result
with cache enabled for anonymous users example with KernelEvent::REQUEST works even cached page executed

We have writeSettings() now.

awesome!

Committed fe96498 and pushed to 8.x. Thanks!