Decouple \Drupal\Core\Path\AliasManager and \Drupal\Core\Path\Path

1. +++ b/core/lib/Drupal/Core/CacheDecorator/AliasManagerCacheDecorator.php
   @@ -117,4 +117,12 @@ public function getPathLookups() {
   +   * Implements \Drupal\Core\Path\AliasManagerInterface::cacheClear().
   

   nitpick: should be {@inheritdoc}

2. +++ b/core/lib/Drupal/Core/Path/Path.php
   @@ -78,8 +82,7 @@ public function save($source, $alias, $langcode = Language::LANGCODE_NOT_SPECIFI
   +      $hook = 'insert';
   
   @@ -87,13 +90,10 @@ public function save($source, $alias, $langcode = Language::LANGCODE_NOT_SPECIFI
   +      $hook = 'update';
   

   We should name it a little bit different than "hook".

3. +++ b/core/lib/Drupal/Core/Path/Path.php
   @@ -87,13 +90,10 @@ public function save($source, $alias, $langcode = Language::LANGCODE_NOT_SPECIFI
   +      $this->event_dispatcher->dispatch('path.' . $hook, new PathInsertUpdateEvent($fields));
   

   Most other implementations do use a constant so you can really easy find the dispatcher and subscriber of an event. See KernelEvents::REQUEST

4. +++ b/core/lib/Drupal/Core/Path/PathCRUDSubscriber.php
   @@ -0,0 +1,80 @@
   +  public function hookPathUpdate(PathInsertUpdateEvent $event) {
   +    $this->moduleHandler->invokeAll('path_update', $event->getFields());
   +  }
   

   To be honest it is odd to have both events and hooks for the same thing.

5. +++ b/core/modules/system/lib/Drupal/system/Tests/Routing/MockAliasManager.php
   @@ -89,4 +89,11 @@ public function getPathLookups() {
   +   * Implements \Drupal\Core\Path\AliasManagerInterface::cacheClear().
   

   Should be {@inheritdoc}

1. +++ b/core/core.services.yml
   @@ -303,7 +303,12 @@ services:
   +    arguments: ['@path.alias_manager.cached', '@module_handler']
   
   +++ b/core/lib/Drupal/Core/Path/PathCRUDSubscriber.php
   @@ -0,0 +1,56 @@
   +   *
   +   * @var \Drupal\Core\Extension\ModuleHandlerInterface
   +   */
   ...
   +  public function __construct(AliasManagerInterface $alias_manager, ModuleHandlerInterface $module_handler) {
   ...
   +    $this->moduleHandler = $module_handler;
   

   We no longer need the module handler here + docs for the alias manager is missing.

2. +++ b/core/lib/Drupal/Core/Path/Path.php
   @@ -17,6 +18,27 @@
   +  const INSERT = 'path.insert';
   
   +++ b/core/lib/Drupal/Core/Path/PathDeleteEvent.php
   @@ -0,0 +1,40 @@
   +class PathDeleteEvent extends Event {
   

   Is it just me that event constants don't really belong in some class though more on the separate event classes?

I really like how it works now, though it is an API change to remove the hook support. Let's see what maintainers think.

I can't see any justification for replacing the hook invocations with events here. The use of events in core is extremely inconsistent, and given these are CRUD hooks this is adding to that inconsistency (compared to say entity CRUD) rather than improving it. Just seems unnecessary to me.

+++ b/core/lib/Drupal/Core/Path/AliasManager.php
@@ -131,13 +131,13 @@ public function getPathAlias($path, $path_language = NULL) {
+  public function cacheClear() {

There was a good reason for this argument when it was added, if it's been broken by refactoring we should fix it rather than removing.

13: 2126421_12.patch queued for re-testing.

Let's put it back for committer feedback.

13: 2126421_12.patch queued for re-testing.

Assigning to Alex for a second opinion.

I'm not sure on this one. The events save the circular dependency with the cache clear. But it's another, inconsistent, ad-hoc replacement of a hook with an event - I still really dislike the DX for events.

And yet, routes are using events for example. Why not this one?

Using events is fine by me - especially in the routing system and where we want a service to respond to something.

+++ b/core/core.services.yml
@@ -303,7 +303,12 @@ services:
+  path.crud_subscriber:
+    class: Drupal\Core\Path\PathCRUDSubscriber
+    tags:
+      - { name: event_subscriber }
+    arguments: ['@path.alias_manager.cached']

Just make the path.alias_manager_cache service an event listener. No need for the additional service and less code to maintain.

I don't think path aliases are the routing system, they're more a configuration/content entity that hasn't been converted yet.

While the documentation on the PathEvent class does not make any sense and is quite confusing, Symfony which is declared to superior to Drupal, uses the same terminology so this is apparently ready.

1. +++ b/core/lib/Drupal/Core/CacheDecorator/AliasManagerCacheDecorator.php
   @@ -98,23 +100,44 @@ public function getSystemPath($path, $path_language = NULL) {
   +  /**
   +   * Registers the methods in this class that should be listeners.
   +   *
   +   * @return array
   +   *   An array of event listener definitions.
   +   */
   

   We can also just @inheritdoc this piece here.

2. +++ b/core/lib/Drupal/Core/Path/Path.php
   @@ -24,21 +24,24 @@ class Path {
   +  protected $event_dispatcher;
   

   we use camelcase for variables.

3. +++ b/core/lib/Drupal/Core/Path/Path.php
   @@ -24,21 +24,24 @@ class Path {
   +   * @var \Symfony\Component\EventDispatcher\EventDispatcher
   ...
   +   * @param \Symfony\Component\EventDispatcher\EventDispatcher $event_dispatcher
   ...
   +  public function __construct(Connection $connection, EventDispatcher $event_dispatcher) {
   

   Let's typehint the interface instead.

setting back to needs work due to @dawehner's review

I don't think path aliases are the routing system, they're more a configuration/content entity that hasn't been converted yet.

FWIW, I agree with this and with the fact that we shouldn't use events here.

@amateescu & @catch - so how is the AliasManagerCacheDecorator supposed to react to the hooks?

@alexpott, the hooks can be implemented by a module (path I would say) and invoke the appropiate methods on AliasManagerCacheDecorator? @chx noted in IRC that path.module is not required, but then I wonder why we even have a Drupal\Core\Path namespace instead of having everything nicely contained in the path module..

path is really path_ui . that said, hooks can live in system. system is often the hook container for such things, anyways.

Unlike module_invoke_all of old, you dont pass a variable number of arguments to invokeAll, it's invokeAll($hook, array())

If we don't like events then \Drupal::service() it is.

  /**
   * Rebuild the path alias white list.
   *
   * @param $source
   *   An optional system path for which an alias is being inserted.
   *
   * @return
   *   An array containing a white list of path aliases.
   */
  protected function pathAliasWhitelistRebuild($source = NULL) {
    // When paths are inserted, only rebuild the whitelist if the system path
    // has a top level component which is not already in the whitelist.
    if (!empty($source)) {
      if ($this->whitelist->get(strtok($source, '/'))) {
        return;
     }
    }
    $this->whitelist->clear();
  }

Is there any point to the source parameter? Now that this patch removes the source parameter from AliasManagerInterface::cacheClear()? Maybe there is a reason in contrib for this but it does appear from the comment that we are no longer only rebuilding the whitelist if the system path has a top level component which is not already in the whitelist.

So the idea with that parameter was this:

- when a path is inserted/updated, it's often for paths like node/123 or user/456 via pathauto.

- if node/user is already in the path alias whitelist, there's really no need to clear the cache.

@slashrsm, the regression won't be on save, but on the cost of rebuilding the path alias whitelist on the next request.

It's quite possible this is already broken in HEAD, but it might be a regression compared to 7.x to kill it.

+++ b/core/lib/Drupal/Core/Path/Path.php
@@ -87,13 +89,10 @@ public function save($source, $alias, $langcode = Language::LANGCODE_NOT_SPECIFI
-      // @todo Switch to using an event for this instead of a hook.

@@ -137,9 +136,7 @@ public function delete($conditions) {
-    // @todo Switch to using an event for this instead of a hook.

Are we sure we don't want to keep this @todo?

@tim.plunkett, @slashrsm, this very issue decided to *not* use an event there, so why should we keep the @todos? :)

1. +++ b/core/modules/system/lib/Drupal/system/Tests/Path/AliasTest.php
   @@ -86,9 +84,8 @@ function testLookupPath() {
   +    $aliasManager = $this->container->get('path.alias_manager');
   
   @@ -108,6 +105,8 @@ function testLookupPath() {
   +    $aliasManager->cacheClear();
   ...
   +    // Hook that clears cache is not executed with unit tests.
   

   I wonder whether we should use the cached version as well to ensure that it also reflected in the cached one. Maybe we could/should also use \Drupal instead.

2. +++ b/core/modules/system/system.module
   @@ -3144,3 +3144,24 @@ function system_admin_paths() {
   +/**
   + * Implements hook_path_update().
   + */
   +function system_path_update() {
   +  \Drupal::service('path.alias_manager')->cacheClear();
   +}
   +
   +/**
   + * Implements hook_path_insert().
   + */
   +function system_path_insert() {
   +  \Drupal::service('path.alias_manager')->cacheClear();
   +}
   +
   +/**
   + * Implements hook_path_delete().
   + */
   +function system_path_delete($path) {
   +  \Drupal::service('path.alias_manager')->cacheClear();
   +}
   

   It is certainly sad to see that we make system.module bigger again, but yeah for now we cannot really avoid that sadly. It seems to be that we should use the ' path.alias_manager.cached' service instead ...

No longer needs a change record as nothing is new here.

Patch looks good.

Committed/pushed to 8.x, thanks!