While working on #2136503: Make \Drupal\Core\Path\AliasManager storage independent I found out that we don't use interface with \Drupal\Core\Path\Path. This will make implementation and usage of alternative storage implementations much harder.

Let's introduce PathInterface and use it in all places that currently expect Path class.

Patch follows.

Comments

slashrsm’s picture

Status: Active » Needs review
StatusFileSize
new4.9 KB
slashrsm’s picture

Issue tags: -Add PathInterface
slashrsm’s picture

StatusFileSize
new7.97 KB
new3.63 KB

Reroll.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

This looks perfect. People would argue that we need a change record.

sun’s picture

Issue tags: +API clean-up

+1

Next best step would be to rename both the class + interface to some more appropriate and less confusing name (in a separate issue).

PathHandler, PathManager, PathRepository, or similar. — Anything that doesn't sound like the domain object itself (a "Path") :-)

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. The AliasManager needs to be updated to typehint on PathInterface
  2. path.api.php should point to \Drupal\Core\Path\PathInterface::method() instead of \Drupal\Core\Path\Path::method()
+++ b/core/lib/Drupal/Core/Path/PathInterface.php
@@ -0,0 +1,101 @@
+  public function save($source, $alias, $langcode = Language::LANGCODE_NOT_SPECIFIED, $pid = NULL);

There is no use Drupal\Core\Language\Language.

slashrsm’s picture

Status: Needs work » Needs review
StatusFileSize
new10.28 KB
new2.62 KB
slashrsm’s picture

slashrsm’s picture

7: 2199381_7.patch queued for re-testing.

fgm’s picture

slashrsm’s picture

7: 2199381_7.patch queued for re-testing.

xano’s picture

StatusFileSize
new12.98 KB
new6.35 KB

Documentation clean-up.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Great!

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/migrate/lib/Drupal/migrate/Plugin/migrate/destination/UrlAlias.php
@@ -38,7 +38,7 @@ class UrlAlias extends DestinationBase implements ContainerFactoryPluginInterfac
-   * @param \Drupal\Core\Path\Path $path
+   * @param \Drupal\Core\Path\PathInterface $path
    *   The path crud service.
    */
   public function __construct(array $configuration, $plugin_id, array $plugin_definition, MigrationInterface $migration, Path $path) {

Need to update the tyephint too

xano’s picture

Status: Needs work » Needs review
StatusFileSize
new13.8 KB
new1.31 KB
dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 1a51606 and pushed to 8.x. Thanks!

  • Commit 1a51606 on 8.x by alexpott:
    Issue #2199381 by slashrsm, Xano: Add \Drupal\Core\Path\PathInterface.
    
roderik’s picture

Status: Fixed » Needs review
StatusFileSize
new728 bytes

Can I request small followup in the same issue?

I have not looked at the code in this patch closely, but while rerolling something against this, I spotted the following comment...

    *
    * This function may be used build the data for a menu tree only, for example
    * to further massage the data manually before further processing happens.
-   * menu_tree_check_access() needs to be invoked afterwards.
+   * $this->checkAccess() needs to be invoked afterwards.
    *
    * @param string $menu_name
    *   The name of the menu.
xano’s picture

Status: Needs review » Fixed

Well-spotted! However, that code is not related to this issue, so please open another one under the documentation component, so jhodgdon, the documentation maintainer, will find it and commit it to core.

slashrsm’s picture

Yay! :)

Status: Fixed » Closed (fixed)

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