Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Xano’s picture

Assigned: Unassigned » Xano
Xano’s picture

Status: Active » Needs review
FileSize
44.34 KB

All 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.

dawehner’s picture

Great progress so far!

+++ b/core/includes/common.incundefined
@@ -4876,16 +4872,8 @@ function _drupal_bootstrap_full($skip = FALSE) {
-    // Prior to invoking hook_init(), initialize the theme (potentially a custom
-    // one for this page), so that:
-    // - Modules with hook_init() implementations that call theme() or

Does that mean that you can no longer use theme() in the request events?

+++ b/core/lib/Drupal/Core/DependencyInjection/UpdateBundle.phpundefined
@@ -5,17 +5,13 @@
+namespace Drupal\system;

ups, don't change the namespace, as the file wouldn't be found anymore.

+++ b/core/lib/Drupal/Core/DependencyInjection/UpdateBundle.phpundefined
@@ -5,17 +5,13 @@
- * This bundle is manually added by update.php via $conf['container_bundles']
- * and required to prevent various services from trying to retrieve data from
- * storages that do not exist yet.

I guess we should keep this information, as it's helpful.

+++ b/core/modules/action/tests/action_loop_test/lib/Drupal/action_loop_test/EventSubscriber/ActionLoopTestSubscriber.phpundefined
@@ -0,0 +1,39 @@
+ * Definition of Drupal\action_loop_test\EventSubscriber\ActionLoopTestSubscriber.

The codestyle says "Contains \Drupal\..."

+++ b/core/modules/action/tests/action_loop_test/lib/Drupal/action_loop_test/EventSubscriber/ActionLoopTestSubscriber.phpundefined
@@ -0,0 +1,39 @@
+    if (!empty($_GET['trigger_action_on_watchdog'])) {

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.

+++ b/core/modules/action/tests/action_loop_test/lib/Drupal/action_loop_test/EventSubscriber/ActionLoopTestSubscriber.phpundefined
@@ -0,0 +1,39 @@
+    $events[KernelEvents::REQUEST][] = array('onRequest', 40);

Is there a specific reason for the weight? If we don't need it, let's skip it.

+++ b/core/modules/overlay/lib/Drupal/overlay/EventSubscriber/OverlaySubscriber.phpundefined
@@ -0,0 +1,78 @@
+    $user_data = drupal_container()->get('user.data')->get('overlay', $user->uid, 'enabled');

Another example where you could inject the user.data service into the class.

+++ b/core/modules/overlay/lib/Drupal/overlay/EventSubscriber/OverlaySubscriber.phpundefined
@@ -0,0 +1,78 @@
+  static function getSubscribedEvents() {

This should have a Implements EventSubscriberInterface ...

+++ b/core/modules/overlay/lib/Drupal/overlay/OverlayBundle.phpundefined
@@ -0,0 +1,25 @@
+ * Contains Drupal\overlay\OverlayBundle.

Missing starting "\"

+++ b/core/modules/overlay/overlay.installundefined
@@ -13,7 +13,7 @@
+    // Flag for a redirect to <front>#overlay=admin/modules.

Maybe document the subscriber here as well?

+++ b/core/modules/overlay/overlay.moduleundefined
@@ -112,59 +112,6 @@ function overlay_user_update($account) {
- * Determine whether the current page request is destined to appear in the
- * parent window or in the overlay window, and format the page accordingly.
...
- * @see overlay_set_mode()

We should keep this part of the documentation somewhere in the overlay.

+++ /dev/nullundefined
@@ -1,43 +0,0 @@
-  function testThemeInitializationHookInit() {

We should have a test that checks that a css can be added at that point.

+++ b/core/modules/system/tests/modules/menu_test/lib/Drupal/menu_test/EventSubscriber/MenuTestSubscriber.phpundefined
@@ -0,0 +1,42 @@
+    if (state()->get('menu_test.record_active_trail') ?: FALSE) {
+      state()->set('menu_test.active_trail_initial', menu_get_active_trail());

There is a service in the container for state, which you could use.

Status: Needs review » Needs work

The last submitted patch, drupal_1911728_00.patch, failed testing.

Xano’s picture

Status: Needs work » Needs review
FileSize
44.32 KB
+++ b/core/includes/common.incundefined
@@ -4876,16 +4872,8 @@ function _drupal_bootstrap_full($skip = FALSE) {
-    // Prior to invoking hook_init(), initialize the theme (potentially a custom
-    // one for this page), so that:
-    // - Modules with hook_init() implementations that call theme() or

Does that mean that you can no longer use theme() in the request events?

\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().


+++ b/core/modules/action/tests/action_loop_test/lib/Drupal/action_loop_test/EventSubscriber/ActionLoopTestSubscriber.phpundefined
@@ -0,0 +1,39 @@
+ if (!empty($_GET['trigger_action_on_watchdog'])) {

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.

+++ b/core/modules/overlay/lib/Drupal/overlay/EventSubscriber/OverlaySubscriber.phpundefined
@@ -0,0 +1,78 @@
+    $user_data = drupal_container()->get('user.data')->get('overlay', $user->uid, 'enabled');

Another example where you could inject the user.data service into the class.

+++ b/core/modules/system/tests/modules/menu_test/lib/Drupal/menu_test/EventSubscriber/MenuTestSubscriber.phpundefined
@@ -0,0 +1,42 @@
+    if (state()->get('menu_test.record_active_trail') ?: FALSE) {
+      state()->set('menu_test.active_trail_initial', menu_get_active_trail());

There is a service in the container for state, which you could use.

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?

+++ /dev/nullundefined
@@ -1,43 +0,0 @@
-  function testThemeInitializationHookInit() {

We should have a test that checks that a css can be added at that point.

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.

dawehner’s picture

So let me introduce you to the wonderful world of the dependecy injection container. Take the UserBundle one as example:


    $container
      ->register('user.data', 'Drupal\user\UserData')
      ->addArgument(new Reference('database'));

So what you see on the UserData class is

  public function __construct(Connection $connection) {
    $this->connection = $connection;
  }

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.

Status: Needs review » Needs work

The last submitted patch, drupal_1911728_01.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
2.63 KB

Let me show you what i mean.

Xano’s picture

FileSize
45.26 KB

@dawehner: thank you!

This patch injects services into subscribers, instead of making them use global functions to get services.

Status: Needs review » Needs work

The last submitted patch, drupal_1911728_02.patch, failed testing.

Xano’s picture

Status: Needs work » Needs review
FileSize
48 KB

Rename \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.

Status: Needs review » Needs work

The last submitted patch, drupal_1911728_03.patch, failed testing.

Xano’s picture

Status: Needs work » Needs review
FileSize
52.2 KB

Everything 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?

Status: Needs review » Needs work

The last submitted patch, drupal_1911728_04.patch, failed testing.

dawehner’s picture

+++ b/core/includes/common.incundefined
@@ -3564,8 +3562,7 @@ function drupal_region_class($region) {
- * files are not needed on a page. This is normally done by calling
- * drupal_add_js() in a hook_init() implementation.
+ * files are not needed on a page.

Shouldn't we update these kind of documentation? No idea but dropping this information could be wrong.

+++ b/core/includes/update.incundefined
@@ -99,8 +99,8 @@ function update_prepare_d8_bootstrap() {
+  // Enable UpdateAPIBundle service overrides.
+  $GLOBALS['conf']['container_bundles'][] = 'Drupal\Core\DependencyInjection\UpdateAPIBundle';

We might should discuss about different kind of names, but yeah UpdateAPI is not too bad.

+++ b/core/includes/update.incundefined
index 0000000..6851b6b
--- /dev/null

--- /dev/null
+++ b/core/lib/Drupal/Core/DependencyInjection/UpdateAPIBundle.phpundefined

http://drupal.org/node/1666604 gives you a nice way to provide diffs for renamed files.

+++ b/core/modules/action/tests/action_loop_test/lib/Drupal/action_loop_test/ActionLoopTestBundle.phpundefined
@@ -0,0 +1,27 @@
+    }

wrong indentation.

+++ b/core/modules/overlay/lib/Drupal/overlay/EventSubscriber/OverlaySubscriber.phpundefined
@@ -0,0 +1,97 @@
+   * The user.data service.
...
+   *   The user.data service.

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."

+++ b/core/modules/overlay/lib/Drupal/overlay/EventSubscriber/OverlaySubscriber.phpundefined
@@ -0,0 +1,97 @@
+   * Implements __construct().

Constructs an OverlaySubscriber object.

+++ b/core/modules/overlay/lib/Drupal/overlay/EventSubscriber/OverlaySubscriber.phpundefined
@@ -0,0 +1,97 @@
+  function __construct(UserDataInterface $user_data) {

Should be better "public function __construct(..."

+++ b/core/modules/overlay/lib/Drupal/overlay/EventSubscriber/OverlaySubscriber.phpundefined
@@ -0,0 +1,97 @@
+    $user_data = drupal_container()->get('user.data')->get('overlay', $user->uid, 'enabled');

You forgot to use the userData service from that object.

+++ b/core/modules/overlay/lib/Drupal/overlay/EventSubscriber/OverlaySubscriber.phpundefined
@@ -0,0 +1,97 @@
+   * Implements Symfony\Component\EventDispatcher\EventSubscriberInterface::getSubscribedEvents().

Trailing "\"

+++ b/core/modules/system/lib/Drupal/system/EventSubscriber/SystemSubscriber.phpundefined
@@ -0,0 +1,54 @@
+   * Implements Symfony\Component\EventDispatcher\EventSubscriberInterface::getSubscribedEvents().

"\"

+++ b/core/modules/system/tests/modules/menu_test/lib/Drupal/menu_test/EventSubscriber/MenuTestSubscriber.phpundefined
@@ -0,0 +1,57 @@
+  /**
+   * The state service.
+   *
+   * @var \Drupal\Core\KeyValueStore\KeyValueStoreInterface
+   */
+  protected $state = NULL;
+
+  /**
+   * Implements __construct().
+   *
+   * @param \Drupal\Core\KeyValueStore\KeyValueStoreInterface
+   *   The state service.
+   */
+  function __construct(KeyValueStoreInterface $state) {
+    $this->state = $state;

See before, sorry.

+++ b/core/modules/system/tests/modules/menu_test/lib/Drupal/menu_test/EventSubscriber/MenuTestSubscriber.phpundefined
@@ -0,0 +1,57 @@
+    if ($this->state->get('menu_test.record_active_trail') ?: FALSE) {

It's just nice to see proper injected stuff!

+++ b/core/modules/system/tests/modules/system_test/lib/Drupal/system_test/SystemTestBundle.phpundefined
@@ -0,0 +1,27 @@
+ * Contains Drupal\system_test\SystemTestBundle.

Missing "\"

chx’s picture

> 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.

Xano’s picture

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.

hook_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.

chx’s picture

Hm, I made a mistake. Please, please disregard my comment, sorry, I overstepped my boundaries.

Xano’s picture

Regarding 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.

sun’s picture

@chx's remark in #16 is valid and correct. However:

Bundle::boot() is the new hook_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() and Bundle::shutdown() are the new hook_boot() and hook_exit() (request-agnostic; uncached).

- HttpKernel Request events are the new hook_init() and hook_exit() (request-specific; cached).

Consequently, removing hook_init() looks fine to me. There aren't many (appropriate) use-cases for it to begin with.

Xano’s picture

I 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?

sun’s picture

Issue tags: +API clean-up, +kernel-followup

As it seems like #20 wasn't completely clear to everyone, please note this additional discussion:

#1911178-52: Remove hook_exit()

catch’s picture

Nearly 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.

Crell’s picture

Nothing 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.

catch’s picture

Yes 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.

Crell’s picture

Fully 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.

catch’s picture

That applies to the patch here as well though, not just the change notice.

Damien Tournoud’s picture

Actually, most of those seem legit. There are two main issues that I can see, but they should not block this patch:

  • Some implementation would be better some place else (for example, those that set a message on the page, most probably want to move to a page-level subscriber -- hook_page_build()?), but this should not block this patch.
  • The implementations need to be fully rewritten to use $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.

catch’s picture

It'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.

Crell’s picture

Er, 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.

catch’s picture

Er, hook_page_build() is not long for this world between SCOTCH and WSCCI, no?

When 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

 /**
- * Implements hook_init().
- */
-function action_loop_test_init() {
-  if (!empty($_GET['trigger_action_on_watchdog'])) {
-    watchdog_skip_semaphore('action_loop_test', 'Triggering action loop');
-  }
-}
-
-/**
  * Implements hook_action_info().
  */
 function action_loop_test_action_info() {
diff --git a/core/modules/action/tests/action_loop_test/lib/Drupal/action_loop_test/ActionLoopTestBundle.php b/core/modules/action/tests/action_loop_test/lib/Drupal/action_loop_test/ActionLoopTestBundle.php
new file mode 100644
index 0000000..c91f030
--- /dev/null
+++ b/core/modules/action/tests/action_loop_test/lib/Drupal/action_loop_test/ActionLoopTestBundle.php
@@ -0,0 +1,27 @@
+<?php
+
+/**
+ * @file
+ * Contains \Drupal\action_loop_test\ActionLoopTestBundle.
+ */
+
+namespace Drupal\action_loop_test;
+
+use Symfony\Component\DependencyInjection\ContainerBuilder as SymfonyContainerBuilder;
+use Symfony\Component\DependencyInjection\Reference;
+use Symfony\Component\HttpKernel\Bundle\Bundle;
+
+/**
+ * Action loop test dependency injection container.
+ */
+class ActionLoopTestBundle extends Bundle {
+
+  /**
+   * Implements \Symfony\Component\HttpKernel\Bundle\BundleInterface::build().
+   */
+  public function build(SymfonyContainerBuilder $container) {
+    $container->register('action_loop_test.subscriber', 'Drupal\action_loop_test\EventSubscriber\ActionLoopTestSubscriber')
+      ->addArgument(new Reference('request'))
+      ->addTag('event_subscriber');
+    }
+}
diff --git a/core/modules/action/tests/action_loop_test/lib/Drupal/action_loop_test/EventSubscriber/ActionLoopTestSubscriber.php b/core/modules/action/tests/action_loop_test/lib/Drupal/action_loop_test/EventSubscriber/ActionLoopTestSubscriber.php
new file mode 100644
index 0000000..7cb6d59
--- /dev/null
+++ b/core/modules/action/tests/action_loop_test/lib/Drupal/action_loop_test/EventSubscriber/ActionLoopTestSubscriber.php
@@ -0,0 +1,54 @@
+<?php
+
+/**
+ * @file
+ * Contains \Drupal\action_loop_test\EventSubscriber\ActionLoopTestSubscriber.
+ */
+
+namespace Drupal\action_loop_test\EventSubscriber;
+
+use Symfony\Component\HttpFoundation\Request;
+use Symfony\Component\HttpKernel\KernelEvents;
+use Symfony\Component\HttpKernel\Event\GetResponseEvent;
+use Symfony\Component\EventDispatcher\EventSubscriberInterface;
+
+/**
+ * Action loop test subscriber for controller requests.
+ */
+class ActionLoopTestSubscriber implements EventSubscriberInterface {
+
+  /**
+   * The request service.
+   *
+   * @var \Symfony\Component\HttpFoundation\Request
+   */
+  protected $request = NULL;
+
+  /**
+   * Implements __construct().
+   *
+   * @param \Symfony\Component\HttpFoundation\Request $request
+   *   The request service.
+   */
+  function __construct(Request $request) {
+    $this->request = $request;
+  }
+
+  /**
+   * Implements event handler for kernel.request.
+   */
+  public function onRequest(GetResponseEvent $event) {
+    if ($this->request->query->get('trigger_action_on_watchdog')) {
+      watchdog_skip_semaphore('action_loop_test', 'Triggering action loop');
+    }
+  }
+
+  /**
+   * Implements Symfony\Component\EventDispatcher\EventSubscriberInterface::getSubscribedEvents().
+   */
+  static function getSubscribedEvents() {
+    $events[KernelEvents::REQUEST][] = array('onRequest');
+    return $events;
+  }
+
+}

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.

effulgentsia’s picture

I 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?

Eronarn’s picture

Something 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).

ParisLiakos’s picture

Assigned: Xano » ParisLiakos
Status: Needs work » Needs review
FileSize
21 KB

well, seems that only system module's implementation needs to be a listener..not sure yet about overlay, i ll ask bot

Status: Needs review » Needs work

The last submitted patch, drupal_1911728_34.patch, failed testing.

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
21.52 KB

sigh

Status: Needs review » Needs work

The last submitted patch, drupal_1911728_35.patch, failed testing.

catch’s picture

Title: Remove hook_init() in favor of kernel.request » Remove hook_init()

@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.

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
32.71 KB

ok 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

attiks’s picture

Some minor issues

+++ b/core/lib/Drupal/Core/EventSubscriber/SlaveDatabaseIgnoreSubscriber.phpundefined
@@ -0,0 +1,55 @@
+    // In Drupal's distributed database structure, new data is written to the master
...
+    // between when data is written to the master and when it is available on the slave.
...
+    // users we still get the benefits of having a slave server, just with slightly

comment too long

+++ b/core/modules/overlay/lib/Drupal/overlay/EventSubscriber/OverlaySubscriber.phpundefined
@@ -17,6 +21,92 @@
+      // Only act on Html pages.

Html -> HTML

+++ b/core/modules/overlay/lib/Drupal/overlay/EventSubscriber/OverlaySubscriber.phpundefined
@@ -17,6 +21,92 @@
+        // Unset the render parameter to avoid it being included in URLs on the page.

comment too long

ParisLiakos’s picture

FileSize
4.16 KB
32.73 KB

fixed comment wrapping

Crell’s picture

Status: Needs review » Reviewed & tested by the community

Thanks, Paris!

ParisLiakos’s picture

FileSize
32.75 KB

Reroll for the language constant commit

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Needs reroll

curl https://drupal.org/files/drupal_1911728_43.patch | git a
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 33540  100 33540    0     0  26713      0  0:00:01  0:00:01 --:--:-- 31026
error: patch failed: core/core.services.yml:424
jibran’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
32.76 KB

reroll

conflict

++<<<<<<< HEAD
 +  batch.storage:
 +    class: Drupal\Core\Utility\BatchStorage
 +    arguments: ['@database']
++=======
+   slave_database_ignore__subscriber:
+     class: Drupal\Core\EventSubscriber\SlaveDatabaseIgnoreSubscriber
+     tags:
+       - {name: event_subscriber}
++>>>>>>> 43

fixed

 +  batch.storage:
 +    class: Drupal\Core\Utility\BatchStorage
 +    arguments: ['@database']
+   slave_database_ignore__subscriber:
+     class: Drupal\Core\EventSubscriber\SlaveDatabaseIgnoreSubscriber
+     tags:
+       - {name: event_subscriber}

Status: Reviewed & tested by the community » Needs work
Issue tags: -API clean-up, -kernel-followup

The last submitted patch, drupal_1911728_45.patch, failed testing.

ParisLiakos’s picture

Status: Needs work » Needs review

#45: drupal_1911728_45.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, drupal_1911728_45.patch, failed testing.

jibran’s picture

Status: Needs work » Needs review
Issue tags: +API clean-up, +kernel-followup

#45: drupal_1911728_45.patch queued for re-testing.
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. I installed locally it worked.

David_Rothstein’s picture

 /**
- * Implements hook_init().
+ * Implements hook_custom_theme().
  */
-function action_loop_test_init() {
+function action_loop_test_custom_theme() {

That seems like it needs a code comment (at the very least)...

@@ -605,7 +552,7 @@ function overlay_get_mode() {
  *     to 'parent' or 'child' in overlay_init() when certain conditions are
  *     met, other modules which want to override that behavior can do so by
  *     setting the mode to 'none' earlier in the page request - e.g., in their
- *     own hook_init() implementations, if they have a lower weight.
+ *     own hook_page_build() implementations, if they have a lower weight.

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?

-  public static function getInfo() {
-    return array(
-      'name' => 'Theme initialization in hook_init()',
-      'description' => 'Tests that the theme system can be correctly initialized in hook_init().',
-      'group' => 'Theme',
-    );
-  }

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...

--- a/core/update.php
+++ b/core/update.php
@@ -33,8 +33,8 @@
 /**
  * Global flag indicating that update.php is being run.
  *
- * When this flag is set, various operations do not take place, such as invoking
- * hook_init(), css/js preprocessing, and translation.
+ * When this flag is set, various operations do not take place, such as  css/js
+ * preprocessing, and translation.

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."

Status: Needs review » Needs work
Issue tags: -API clean-up, -kernel-followup

The last submitted patch, drupal_1911728_45.patch, failed testing.

jibran’s picture

Status: Needs work » Needs review
Issue tags: +API clean-up, +kernel-followup

#45: drupal_1911728_45.patch queued for re-testing.

jibran’s picture

Status: Needs review » Needs work

As per #50

ParisLiakos’s picture

Assigned: ParisLiakos » Unassigned
Status: Needs work » Needs review
FileSize
3.31 KB
33.81 KB

thanks for the review!

Fixed most of the above besides

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...

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.

Crell’s picture

Status: Needs review » Reviewed & tested by the community

We'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.

David_Rothstein’s picture

Status: Reviewed & tested by the community » Needs review

Well, 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:

-  if (arg(0) == 'user' && arg(1) == 'autocomplete') {
-    // Register a fake registry loading callback. If it gets called by
-    // theme_get_registry(), the registry has not been initialized yet.
-    _theme_registry_callback('_theme_test_load_registry', array());
-  }

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.

David_Rothstein’s picture

This 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:

+  /**
+   * {@inheritdoc}
+   */
+  public function checkSlaveServer(GetResponseEvent $event) {
+    // Ignore slave database servers for this request.

Is this actually inheriting the doc from anything?

ParisLiakos’s picture

Status: Needs review » Needs work

Is this actually inheriting the doc from anything?

no, its just a copy paste failure.

Let me expand on my meaning

it doesnt make any sense to me.

on #54

  1. this test is already broken cause hook_init is no longer such a special hook that is used to be.
  2. the proper fix for this broken test is #1886448: Rewrite the theme registry into a proper service which is critical btw.This way you can correctly depend on it via the container
  3. (imo) if you call theme() from a request listener your code should break and we should do nothing to ensure it keeps working.

but since we need to move this issue forward, i am fine with anything so lets fix the following and move on:

+++ b/core/modules/system/tests/modules/theme_test/lib/Drupal/theme_test/EventSubscriber/ThemeTestSubscriber.phpundefined
@@ -0,0 +1,49 @@
+   * @see Drupal\system\Tests\Theme\ThemeEarlyInitializationTest::testRequestListener()

should be the full namespace with preceeding backslash

+++ b/core/modules/overlay/lib/Drupal/overlay/EventSubscriber/OverlaySubscriber.phpundefined
@@ -17,6 +21,93 @@
+        unset($_GET['render']);

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..

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
3.02 KB
38.35 KB

Status: Needs review » Needs work

The last submitted patch, drupal-hook-init-1911728-59.patch, failed testing.

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
1.1 KB
38.35 KB

yeah, that was stupid of me

Status: Needs review » Needs work
Issue tags: -API clean-up, -kernel-followup

The last submitted patch, drupal-hook-init-1911728-61.patch, failed testing.

ParisLiakos’s picture

Status: Needs work » Needs review
Issue tags: +API clean-up, +kernel-followup

#61: drupal-hook-init-1911728-61.patch queued for re-testing.

David_Rothstein’s picture

Should probably get rid of the $args variable itself also, since it's not used...

Status: Needs review » Needs work

The last submitted patch, drupal-hook-init-1911728-61.patch, failed testing.

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
1.03 KB
38.29 KB

true that, removed
seems bot is having issues

Crell’s picture

Status: Needs review » Reviewed & tested by the community

I think this addresses all of David's concerns, so let's try this again.

David_Rothstein’s picture

Only skimmed it over, but yes, this version looks good to me too.

alexpott’s picture

Title: Remove hook_init() » Change notice: Remove hook_init()
Priority: Normal » Critical
Status: Reviewed & tested by the community » Active
Issue tags: -kernel-followup +Needs change record

Committed 42daa83 and pushed to 8.x. Thanks!

ParisLiakos’s picture

Title: Change notice: Remove hook_init() » Remove hook_init()
Priority: Critical » Normal
Status: Active » Fixed
Issue tags: -Needs change record +kernel-followup

change notice here https://drupal.org/node/2013014
feel free to improve!

Automatically closed -- issue fixed for 2 weeks with no activity.