Add a way to trigger_error() for deprecated hooks

+++ b/core/lib/Drupal/Core/Extension/ModuleHandler.php
index 6b2738c779..c2cbc29afe 100644
--- a/core/lib/Drupal/Core/Extension/ModuleHandlerInterface.php

--- a/core/lib/Drupal/Core/Extension/ModuleHandlerInterface.php
+++ b/core/lib/Drupal/Core/Extension/ModuleHandlerInterface.php

+++ b/core/lib/Drupal/Core/Extension/ModuleHandlerInterface.php
@@ -239,6 +239,49 @@ public function invoke($module, $hook, array $args = []);
+   */
+  public function invokeDeprecated($module, $hook, array $args = []);
+

We could avoid changing the interface by having a module_handler.deprecated which is like a regular module handler but marks every function call as deprecated. Any thoughts about that?

This looks good, though I am wondering whether there is a legitimate way in the documentation standards to say 'see moduleInvokeAll() for the parameters' rather than repeat them here. I think this would improve clarity that the method signature is identical, and that it's a drop-in replacement.

Agreed ... we should ensure we add test coverage for that, and well, for that we first need actual capabilities for that.

Here is an alternative implementation using hook_hook_info().

It seems like we'd only want to trigger the error if there are implementations.

I think you are absolutely right here :)

1. +++ b/core/lib/Drupal/Core/Extension/ModuleHandler.php
   index 6b2738c779..eadeebfad7 100644
   --- a/core/lib/Drupal/Core/Extension/ModuleHandlerInterface.php
   
   --- a/core/lib/Drupal/Core/Extension/ModuleHandlerInterface.php
   +++ b/core/lib/Drupal/Core/Extension/ModuleHandlerInterface.php
   

   For better BC maybe add a new interface for that? To be honest I don't expect thought that anyone extends this particular class to be honest.

2. +++ b/core/tests/Drupal/KernelTests/Core/Extension/ModuleHandlerTest.php
   @@ -0,0 +1,56 @@
   +   * @expectedDeprecation The deprecated alter hook hook_deprecate_alter() is implemented: deprecation_test_deprecate_alter
   ...
   +   * @expectedDeprecation The following modules implement the deprecated hook hook_deprecate(): deprecation_test
   

   It is a bit weird that the first message is about the concrete function name and the second is just containing the module name. I think using the full function makes it easier to find the particular implementation.

module_handler is a service, so it needs one interface that all consumers can use. Since we can invoke() etc on ModuleHandlerInterface, the deprecated invoke has to be on the same interface.

Fair, we can add new methods in minor releases ...

1. +++ b/core/lib/Drupal/Core/Extension/ModuleHandler.php
   @@ -414,6 +414,43 @@ public function invokeAll($hook, array $args = []) {
   +    $result = $this->invokeAll($hook, $args);
   +    $this->triggerDeprecationError($hook);
   
   @@ -503,6 +540,28 @@ public function alter($type, &$data, &$context1 = NULL, &$context2 = NULL) {
   +    // Invoke the alter hook. This has the side effect of populating
   +    // $this->alterFunctions.
   +    $this->alter($type, $data, $context1, $context2);
   ...
   +      @trigger_error($message, E_USER_DEPRECATED);
   

   Ubernitpick: Once the deprecation is before and once after the actual hook calls.

2. +++ b/core/tests/Drupal/Tests/Core/Extension/ModuleHandlerDeprecatedTest.php
   @@ -0,0 +1,84 @@
   +class ModuleHandlerDeprecatedTest extends UnitTestCase {
   ...
   +    $this->moduleHandler->load('module_handler_test_deprecated');
   

   Given this loads code we should IMHO run the test in isolation/in its own separate process.

@runTestsInSeparateProcesses should work for that I think.

Yeah totally, this is totally possible.

From my perspective this looks great!

+++ b/core/lib/Drupal/Core/Extension/ModuleHandlerInterface.php
@@ -239,6 +239,49 @@ public function invoke($module, $hook, array $args = []);
+   * @return mixed
+   *   The return value of the hook implementation.
+   */
+  public function invokeDeprecated($module, $hook, array $args = []);
...
+   *   done using array_merge_recursive().
+   */
+  public function invokeAllDeprecated($hook, array $args = []);

@@ -290,6 +333,60 @@ public function invokeAll($hook, array $args = []);
+   *   associative array as described above.
+   */
+  public function alterDeprecated($type, &$data, &$context1 = NULL, &$context2 = NULL);

I'm wondering whether we should put @see statements to the parallel methods && and then maybe get rid of the 1to1 copied documentation

Thank you! I think we have a solid plan now.

@Mile23
Do you think we need an actual module providing it to have some sort of complete test coverage?

Well, we could write a test which ensures that the hooks are actually triggered ...

1. +++ b/core/lib/Drupal/Core/Extension/ModuleHandler.php
   @@ -411,6 +411,43 @@ public function invokeAll($hook, array $args = []) {
   +      @trigger_error($message . implode(', ', $hooks), E_USER_DEPRECATED);
   
   @@ -502,6 +539,28 @@ public function alter($type, &$data, &$context1 = NULL, &$context2 = NULL) {
   +      $message = 'The deprecated alter hook hook_' . $type . '_alter() is implemented in these functions: ' . implode(', ', $this->alterFunctions[$cid]);
   +      @trigger_error($message, E_USER_DEPRECATED);
   

   It would be kind of nice to say: Read more on hook_$hook() what you should do instead. As alternative maybe invokeDeprecated etc. could take an additional message parameter?

2. +++ b/core/tests/Drupal/Tests/Core/Extension/ModuleHandlerDeprecatedTest.php
   @@ -0,0 +1,97 @@
   + * We have to build elaborate mocks for these tests because
   + * symfony/phpunit-bridge doesn't deal well with kernel tests or
   + * process-isolated tests.
   + * @see https://www.drupal.org/node/2884530
   ...
   +class ModuleHandlerDeprecatedTest extends UnitTestCase {
   

   We could remove this comment now. Maybe though testing this using a unit test isn't a good idea in the first place. Any opinion about that?

+++ b/core/lib/Drupal/Core/Extension/ModuleHandler.php
@@ -411,6 +411,45 @@ public function invokeAll($hook, array $args = []) {
+      $hooks = [];
+      foreach ($modules as $module) {
+        $hooks[] = $module . '_' . $hook . '()';
+      }

nit: i'd use an array_map here, but that's just a personal preference

This looks ready to me, would be great to ship 8.5.0 with this.

Unable to allocate pool apcu issue again

thanks

Nice!

Committed 5485720 and pushed to 8.5.x. Thanks!