I thought this issue already existed, but couldn't find it. If someone else knows where it is, please mark one or the other a duplicate and merge.

The path alias lookup system needs to turn into a self-encapsulated service object.

- It should be a single object that takes a database connection in its constructor.
- It should offer two public lookup methods: getSystemPath() and getAliasedPath() (or something). These map 1:1 to drupal_get_normal_path() and drupal_get_path_alias(), respectively.
- There should be basic crud methods on the object, too. These should mostly be a direct port from the current code in path.inc.
- There *must* be an interface defined for all of the above.
- The path object must be added to the DIC.
- The PathSubscriber object should be instantiated via the DIC, and have the path object injected into it via the DIC.
- PathSubscriber should be updated to use the injected object rather than the current hard-coded functions.

There should be unit (not system, unit) tests for all of the above classes.

Path alias lookups are highly optimized having been a cause of various performance issues in releases prior to D7, any patch should be properly profiled/benchmarked before commit.

Who wants it? :-)

while discussing at catch's patch at #1209226: Avoid slow query for path alias whitelists in IRC, i raised the idea of making the path lookup code into a pluggable class. rough interface idea being:

interface DrupalPathRegistryInterface {
  function save(array $path);
  function load($pid);
  function delete($pid);
  function getNormalPath($path, $path_language = NULL);
  function getPathAlias($path, $path_language = NULL);
} 
CommentFileSizeAuthor
#98 1269742_98.patch1.56 KBchx
#86 1269742.alias_manager.86.patch99.18 KBkatbailey
#86 interdiff.txt3.5 KBkatbailey
#83 1269742.alias_manager.83.patch99.38 KBkatbailey
#83 interdiff.txt17.33 KBkatbailey
#81 1269742.alias_manager.81.patch97.46 KBkatbailey
#81 interdiff.txt8.67 KBkatbailey
#75 1269742.alias_manager.75.patch89.88 KBg.oechsler
#75 interdiff.txt835 bytesg.oechsler
#71 1269742.alias_manager.71.patch89.06 KBkatbailey
#71 interdiff.txt45.81 KBkatbailey
#68 1269742-path-unit-test.patch9.38 KBg.oechsler
#66 1269742-path.crud-unitTest.patch5.76 KBg.oechsler
#62 1269742-62.alias-manager.patch81.53 KBkatbailey
#62 interdiff.txt19.17 KBkatbailey
#60 1269742-60.alias-manager.patch76.75 KBksenzee
#52 1269742.path_alias_manager.52.patch76.88 KBkatbailey
#47 1269742.path_aliasers.47.patch75.58 KBkatbailey
#45 1269742.path_aliasers.45.patch76.02 KBkatbailey
#40 1269742-40.patch73.91 KBpwolanin
#38 1269742-38.patch70.09 KBpwolanin
#37 1269742-37.patch51.58 KBpwolanin
#31 1269742.pathregistry.31.patch73.56 KBkatbailey
#29 1269742.pathregistry.29.patch73.6 KBkatbailey
#23 1269742.pathregistry.23.patch53.31 KBkatbailey
#20 1269742.pathregistry.20.patch46.15 KBkatbailey
#12 path.interface.patch37.89 KBAnonymous (not verified)
#11 path.interface.patch31.4 KBAnonymous (not verified)
#10 path.interface.patch31.4 KBAnonymous (not verified)
#8 path.interface.patch31.36 KBAnonymous (not verified)
#6 path.interface.patch31.36 KBAnonymous (not verified)
#5 path.interface.patch30.94 KBAnonymous (not verified)
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

catch’s picture

Title: make path lookup code into an pluggable class » Make path lookup code into an pluggable class
Issue tags: +Framework Initiative

If we do this it'll allow for the possibility of having pluggable storage for path aliases cleanly, which would be really nice to have.

I'd want to keep the whitelist handling as a separate class as per #1209226: Avoid slow query for path alias whitelists, since that's self-contained and potentially re-usable logic across different path alias implementations.

Possible implementations we could do with this:

4.5/6 "grab all aliases across the site" - for sites that only have dozens of aliases.

4.7-6.x "one query per path" (but probably using the whitelist)

What we have now in 7.x with a per-page cache of system paths + the whitelist.

Redis and/or MongoDB to take the paths out of SQL entirely.

Damien Tournoud’s picture

I would like to remove all this from the bootstrap code, and move that to a module. I see no reason at all to have a path.inc when we have hook_url_inbound_alter() and hook_url_outbound_alter().

catch’s picture

Anonymous’s picture

i've started on this, very early work is here:

http://drupal.org/sandbox/justinrandell/1272704

Anonymous’s picture

Status: Active » Needs review
FileSize
30.94 KB

ok, i tried helping over at #1209226: Avoid slow query for path alias whitelists, and it just made me want to smash things.

so i smashed up path.inc.

this is really just some brute force refactoring, that adds a DrupalPathRegistryInterface, and a default implementation, DrupalPathRegistry, with a path() factory method. path CRUD functions use the new DrupalPathRegistry class, as do drupal_get_normal_path() and drupal_get_path_alias().

i like the idea of making path.inc a module, but i've wanted to get going quickly, and do this in stages.

Anonymous’s picture

FileSize
31.36 KB

this time without the drupal_clear_path_cache() fatals.

Status: Needs review » Needs work

The last submitted patch, path.interface.patch, failed testing.

Anonymous’s picture

Status: Needs work » Needs review
FileSize
31.36 KB

without the syntax error in path.test.

Status: Needs review » Needs work

The last submitted patch, path.interface.patch, failed testing.

Anonymous’s picture

Status: Needs work » Needs review
FileSize
31.4 KB

fix typo in DrupalPathRegistry->delete(), restore function signature of drupal_get_path_alias().

Anonymous’s picture

FileSize
31.4 KB

ahem.

Anonymous’s picture

FileSize
37.89 KB

ok, more refactoring, prepare to get rid of drupal_lookup_path() by eliminating all calls to it that don't come from path()->getNormalPath() or path()->getPathAlias().

andypost’s picture

+++ b/includes/path.incundefined
@@ -9,15 +9,196 @@
+function path() {
+  static $path_registry;

How this class factory will affect a simpletest? Do we need a way to reset this static?

+++ b/includes/path.incundefined
@@ -400,72 +522,8 @@ function drupal_path_alias_whitelist_rebuild($source = NULL) {
+function path_load($pid) {
+  return path()->load($pid);

Any reason to left this in path.inc? Suppose this sub-system could be pluggable so menu loader should not live in pluggable include

Crell’s picture

beejeebus asked me to note here that path resolution is a core function; In the Context API (not yet committed but expected to be eventually), the path:system context key is provided by core so core must provide at least one implementation for path resolution, even if it's essentially a no-op implementation.

However, it should be possible for contrib modules to change the handler used for a given key. (The exact mechanism for that is still being debated and may be dependent on the CMI.) In that case, core could either provide a default SQL-based implementation object or a no-op object, and a contrib module could specify some other implementation to replace it. It would not be using the WSCCI plugins system (as that will likely depend on context), but the swap-out mechanism is still available.

Having an interface for that resolution logic for classes to implement is a good idea either way.

sun’s picture

Given that we don't need and don't want and can't support this functionality in early bootstrap and in the installer/updater anyway, moving it into path.module actually makes most sense to me (as proposed in #464164: Move URL alias functionality into a Path API module (still separate from the Path UI)).

It could still be pluggable even if it's in Path module, so leaving this issue open.

Anonymous’s picture

Assigned: » Unassigned

.

catch’s picture

Issue tags: +WSCCI, +kernel-followup

Marked #1591632: Make path system OO and injectable as a duplicate.

I copied Larry's issue summary from there, but I have serious reservations about the feasibility of doing some of them in a first pass on this, you never know though so I'll hold off trying to explain them for now.

RobLoach’s picture

If we have the DrupalPathRegistry instance held in drupal_container(), we could have a parameter registered to allow the Path system to be swappable.

drupal_container()->setParameter('path_class', 'DrupalPathRegistry');
drupal_container()->register('path', '%path_class%');
function path() {
  return drupal_container()->get('path');
}

It is easy to switch parameter definitions so that the correct path class is instantiated. Tagging so that this issue is tracked.

Crell’s picture

We don't need to split the class out like that. Different classes would likely need different setup anyway (different dependencies, say for one that stores the path index in MongoDB it would need a Mongo connection, not an SQL DB connection).

The first 4 bullet points in the summary can be done now with straight up unit testing (I've verified that you can do pure unit tests even with a DB connection as long as you write your code properly, it's quite easy) without even thinking about the DIC. We should do that. Then the rest of the bullet points are done after that class is written. At that point it's just wiring.

Build the component first, then wire it up.

katbailey’s picture

For the sake of getting this issue moving again I have rerolled beejeebus' patch from #12 and moved the class and interface definitions into their own files under core/lib/Drupal/Core/Path. Seems like it still constitutes a decent starting point. I guess the main task for now is to add a constructor that takes a db connection object and then make it use that directly in its methods rather than using the various db wrapper functions. Is that about right?

Status: Needs review » Needs work

The last submitted patch, 1269742.pathregistry.20.patch, failed testing.

Crell’s picture

Thanks, Kat! This gets us moving again. Yes, a connection parameter for the constructor is step 1. Review below:

+++ b/core/includes/common.inc
@@ -2498,7 +2498,7 @@ function drupal_deliver_html_page($page_callback_result) {
+        $path = path()->getNormalPath(variable_get('site_404', ''));

I wonder if this method shouldn't be renamed. I have no idea what "normal" means. (Insert obvious joke here.) getSystemPath()? (Since system_path is what the property on the request object is called.)

+++ b/core/includes/path.inc
@@ -9,6 +11,19 @@
+/**
+* Return a DrupalPathRegistryInterface class.
+*/
+function path() {
+  static $path_registry;
+  if ($path_registry === NULL) {
+    $path_registry = new DrupalPathRegistry();
+  }
+  return $path_registry;
+}

I'm inclined to skip this factory entirely and just go straight to the DIC. Let's not make more work for ourselves by having yet another static singleton to track when we already have a better system for that.

+++ b/core/includes/path.inc
@@ -9,6 +11,19 @@
@@ -16,18 +31,19 @@ function drupal_path_initialize() {

Are we even using this function still? I think it's probably vestigial and should be removed. This logic has all been ported to listeners already. Or if not, it should be.

+++ b/core/includes/path.inc
@@ -406,78 +363,8 @@ function drupal_path_alias_whitelist_rebuild($source = NULL) {
+function path_load($pid) {
+  return path()->load($pid);
 }

Just from the path, I don't actually understand what this function does. The docblock is getting lost somewhere. :-) Honestly I would rather avoid dumb-wrappers like this, as it only encourages us to keep using function singletons that break encapsulation and testability.

+++ b/core/lib/Drupal/Core/Path/DrupalPathRegistry.php
@@ -0,0 +1,102 @@
+use Drupal\Core\Path\DrupalPathRegistryInterface;
+
+class DrupalPathRegistry implements DrupalPathRegistryInterface {

Class/Interface names should not have "Drupal" in them.

+++ b/core/lib/Drupal/Core/Path/DrupalPathRegistry.php
@@ -0,0 +1,102 @@
+  public function save(array &$path) {

I know this is a straight-port, but I want to try and avoid anonymous arrays as data objects. If Path is not going to be its own data object, then the CRUD functions should take multiple useful parameters rather than an anonymous data array.

+++ b/core/lib/Drupal/Core/Path/DrupalPathRegistry.php
@@ -0,0 +1,102 @@
+        module_invoke_all('path_insert', $path);

I have no idea how we're going to handle calling hooks from inside an injected object. :-)

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocalePathTest.php
@@ -101,13 +101,13 @@ class LocalePathTest extends WebTestBase {
-    path_save($edit);
-    $lookup_path = drupal_lookup_path('alias', 'node/' . $node->nid, 'en');
+    path()->save($edit);
+    $lookup_path = path()->getPathAlias('node/' . $node->nid, 'en');

One of the advantages of this approach is that we can replace these functional tests with direct unit tests of the path object. Those are blazingly fast. Have a look at the routing branch in the WSCCI sandbox for how I'm doing that there, including with a database connection. It works surprisingly well.

-1 days to next Drupal core point release.

katbailey’s picture

OK, getting closer, but I still haven't fixed the tests. Putting this up anyway in case someone else wants to work on it as I probably won't be able to for the next couple of days...

Crell’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 1269742.pathregistry.23.patch, failed testing.

katbailey’s picture

Fyi the latest work on this is now in a branch in the wscci sandbox: http://drupalcode.org/sandbox/Crell/1260830.git/shortlog/refs/heads/path... (corresponds to the patch in #23).

From today's irc meeting:
[09:45am] katbailey: the path alias thing isn't really a use case for this because it's not a module
[09:45am] Crell: Perhaps we could move it to the path module, just to see if it can be done?
[09:46am] katbailey: sure, I can try that as a proof of concept
[09:46am] Crell: Ideally with this approach that functionality could live in a module without any downside.
[09:47am] Crell: Which brings me to... http://drupal.org/node/1269742
[09:47am] Druplicon: http://drupal.org/node/1269742 => Make path lookup code into an pluggable class => Drupal core, base system, normal, needs work, 25 comments, 7 IRC mentions
[09:48am] Crell: There's a very clear roadmap there for what needs to be done, and katbailey's already done a lot of it.
[09:48am] Crell: Who wants to finish it?
[09:48am] jmolivas joined the chat room.
[09:48am] katbailey: Crell: that confuses me - what about your comment here http://drupal.org/node/464164#comment-5922494
[09:48am] Druplicon: http://drupal.org/node/464164 => Move URL alias functionality into a Path API module (still separate from the Path UI) => Drupal core, path.module, normal, needs work, 29 comments, 14 IRC mentions
[09:49am] Crell: katbailey: That's before I realized that using an event listener for it, we could move it out to a module without any performance impact.
[09:49am] Crell: Well, without any serious impact.
[09:49am] Crell: Basically it's already been moved to a "hook" (event), so whether that lives in a hard-coded core list or a generated contrib list doesn't make much difference.
[09:52am] katbailey: ok, then if someone wants to take up the path aliasing stuff before I can get to it (which probably woudn't be for a few days) they could work on moving it out into a module, plus fixing up the tests of course

katbailey’s picture

I've recreated the path-alias-logic branch in the sandbox as it needed to be rebased against upstream/8.x. I also pushed a change to it that moves all the path alias lookup logic into path module. The core Path component now just has a stub implementation of PathRegistryInterface.

I'm not submitting a patch for this here yet because for this to work we need #1599108: Allow modules to register services and subscriber services (events). So I've added another branch to the wscci sandbox, path-bundle, that combines these two issues and shows how it would work: http://drupalcode.org/sandbox/Crell/1260830.git/shortlog/refs/heads/path...

Here's a gist of the entire diff: https://gist.github.com/3068825

I'm not sure if it makes sense to postpone this issue until the bundles patch gets in. Maybe for now we should just keep it all in the Drupal\Core\Path component..?

Crell’s picture

Priority: Normal » Critical

Per discussion with Dries at MWDS:

- #1599108: Allow modules to register services and subscriber services (events) breaks path alias caching, because of scope issues.
- The real fix for that is this issue, where we refactor the path alias library to be cleanly injected and not rely on globals that go out of scope on us at inconvenient times.
- Refactoring the path alias system as part of that issue would harm too many kittens.
- We're therefore OK with committing the bundle patch (above), but in exchange we mark this issue as a critical to restore path alias caching.

katbailey’s picture

Status: Needs work » Needs review
FileSize
73.6 KB

This still needs a lot of work but I want to get the patch up now anyway, seeing as I was able to reroll it against HEAD which now has bundles :-)
I'm noticing one particular set of tests exploding spectacularly (LanguageUILanguageNegotiationTest - I narrowed it down to when language_save() is called but am clueless beyond that) so testbot might not even give us an accurate idea of where we're at with tests, but I'd like to get feedback on the general approach before continuing with it...

Status: Needs review » Needs work

The last submitted patch, 1269742.pathregistry.29.patch, failed testing.

katbailey’s picture

Status: Needs work » Needs review
FileSize
73.56 KB

gah! Let's try this one...

Status: Needs review » Needs work

The last submitted patch, 1269742.pathregistry.31.patch, failed testing.

Berdir’s picture

+++ b/core/includes/common.incundefined
@@ -2178,7 +2178,7 @@ function url($path = NULL, array $options = array()) {
-    $alias = drupal_get_path_alias($original_path, $langcode);
+    $alias = drupal_container()->get('path_registry')->getPathAlias($original_path, $langcode);

Probably the completely wrong issue to discuss this and this might have already been done, haven't followed this part of core development closely.

This is really long, and you not only need to remember an arbitrary string identifier but also a method for which you don't have autocomplete.

What is this supposed to be look like finally? wouldn't it make sense to keep the old function as a wrapper for this, maybe temporarly?

+++ b/core/modules/path/lib/Drupal/path/PathRegistry.phpundefined
@@ -0,0 +1,317 @@
+  public function cacheClear($source = NULL) {
+    drupal_static_reset('drupal_lookup_path');
+    $this->path_alias_whitelist_rebuild($source);
+  }

Wouldn't clearCache() be a better method name?

Powered by Dreditor.

Crell’s picture

Ideally the "final" version will have $path_alias_manager objects injected into other objects by the DIC, so you'd be doing $this->paths->getPathAlias(). Also, most code should never be touching that object directly.

+++ b/core/modules/path/lib/Drupal/path/PathRegistry.php
@@ -0,0 +1,317 @@
+   */
+  private function lookup_path($action, $path = '', $langcode = NULL) {

No privates, and methods should be camelCase().

+++ b/core/modules/path/lib/Drupal/path/PathRegistry.php
@@ -0,0 +1,317 @@
+    // Use the advanced drupal_static() pattern, since this is called very often.
+    static $drupal_static_fast;
+    if (!isset($drupal_static_fast)) {
+      $drupal_static_fast['cache'] = &drupal_static('drupal_lookup_path');
+    }
+    $cache = &$drupal_static_fast['cache'];

Statics inside a class are basically useless. They're shared between ALL instances of the object, which is rarely what you want. Instead you want $this->cache as an array or something.

For tests, I don't look at them closely but tests of the path alias object itself should not use drupal_container() but simply test the object directly, which is unit-testable then. (Or, should be.)

I don't think path_registry is the right name for this service. It's not a registry. It's an aliasing tool, and one of many possible ways to mess with paths. Essentially, it's one particular implementation of hook_url_inbound_alter() (at least the way it's wired in now via a kernel.request event). There could be many others, including pattern-based aliasing rather than a big denormalized lookup table.

For the same reason, while I understand why there's a null implementation in core with an override in path.module that actually does something useful, I'm not sure that's the right approach. Of course, the right approach may not be possible until we have #1705488: Implement a generator for Drupal paths, and some sort of event/hook/thing there that we can latch on to in a performant way. (I don't know what that performant way is yet.) So, that makes me uncomfortable but I don't know what the better alternative is yet.

Also, dear god when did the aliasing system get so bloody complicated?

catch’s picture

+++ b/core/lib/Drupal/Core/Path/PathRegistryInterface.phpundefined
@@ -0,0 +1,88 @@
+  /**
+   * Delete a URL alias.
+   *
+   * @param $criteria
+   *   A number representing the pid or an array of criteria.
+   */
+  public function delete($pid);
+
+  /**
+   * Given a path alias, return the internal path it represents.
+   *
+   * @param $path
+   *   A Drupal path alias.
+   * @param $path_language
+   *   An optional language code to look up the path in.
+   *
+   * @return
+   *   The internal path represented by the alias, or the original alias if no
+   *   internal path was found.
+   */
+  public function getSystemPath($path, $path_language = NULL);

I'm not at all sure it makes sense to combine the alias/system path lookup methods in the same class as the path crud functions. The alias/system path lookup is going to be necessary for any implementation we have, but saving individual path aliases isn't necessarily and even if you did we might not use pid etc. Also they're performing very different jobs already, in the same way efq and entity_*() are different.

+++ b/core/modules/path/lib/Drupal/path/PathRegistry.phpundefined
@@ -0,0 +1,317 @@
+    // Retrieve the path alias whitelist.
+    if (!isset($cache['whitelist'])) {
+      $cache['whitelist'] = variable_get('path_alias_whitelist', NULL);
+      if (!isset($cache['whitelist'])) {
+        $cache['whitelist'] = $this->path_alias_whitelist_rebuild();
+      }

Note there's a patch at #1209226: Avoid slow query for path alias whitelists which moves the path whitelist to DrupalCacheArray. Looking at the patch it's not changing any of this logic so it doesn't really matter but just cross-linking.

pwolanin’s picture

Taking a pass at this while Kat is on the plane - sounds like we need to split this into 2 interfaces/classes for lookup vs crud?

Re-rolling for a start, since the patch doesn't apply to HEAD.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
51.58 KB

setting to needs review just for testbot

pwolanin’s picture

FileSize
70.09 KB

missed the new files

I just added this patch to a new WSCCI branch based on current 8.x:
http://drupalcode.org/sandbox/Crell/1260830.git/shortlog/refs/heads/path...

Status: Needs review » Needs work

The last submitted patch, 1269742-38.patch, failed testing.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
73.91 KB

This is now working in the UI, but the tests are hanging for me locally - not sure why - sometimes I get this after navigating back:

[26-Aug-2012 12:13:06] Uncaught PHP Exception Symfony\Component\DependencyInjection\Exception\ServiceCircularReferenceException: "Circular reference detected for service "language_manager", path: "language_manager"." at  core/vendor/symfony/dependency-injection/Symfony/Component/DependencyInjection/Container.php line 246

This patch corresponds to what I jsut pushed on the branch linked in #38.

Status: Needs review » Needs work

The last submitted patch, 1269742-40.patch, failed testing.

katbailey’s picture

Thanks, @pwolanin. I spent some time today trying to figure out what the architecture of this should be, i.e. taking into account Crell and catch's comments in #34 and #35 respectively. I ended up creating a new branch because I wanted to add the DIC compilation patch and work off that, so that I could use a compiler pass. So I created this branch http://drupalcode.org/sandbox/Crell/1260830.git/shortlog/refs/heads/path....

It's a bit of a monster, totally unfinished and experimental, but I figured I'd leave an update here about it anyway seeing as I'm done for the night and probably won't get back to even thinking about it until the weekend...

katbailey’s picture

After chatting with Crell on irc today I've abandoned the approach mentioned in #43 which required a compiler pass. I pushed a substantial commit to the branch @pwolanin had created (deleted the other branch), which splits apart the responsibilities of the aliasing system and hopefully gets us moving in more or less the right direction. Still too unfinished to put a patch up though I think (for one thing, a bunch of tests will need to get totally rewritten).

katbailey’s picture

Status: Needs work » Needs review
FileSize
76.02 KB

Actually, here's a patch, just for kicks...

Status: Needs review » Needs work

The last submitted patch, 1269742.path_aliasers.45.patch, failed testing.

katbailey’s picture

Status: Needs work » Needs review
FileSize
75.58 KB

Here's a slightly more serious attempt at a patch... (only slightly)
Also, I hate the name PathAliaser - suggestions anyone?

Status: Needs review » Needs work

The last submitted patch, 1269742.path_aliasers.47.patch, failed testing.

Crell’s picture

+++ b/core/includes/bootstrap.inc
@@ -2454,6 +2454,10 @@ function drupal_container(Container $reset = NULL) {
+    // A call to url() could happen before the kernel gets initialized, so we
+    // need to have the path_aliaser service in place.
+    $container->register('path_aliaser', 'Drupal\Core\Path\PathAliaser');

We really really need to eliminate that case by moving more things into the Kernel, after the DIC initializes. *sigh*

+++ b/core/lib/Drupal/Core/ExceptionController.php
+++ b/core/lib/Drupal/Core/ExceptionController.php
@@ -102,7 +102,7 @@ class ExceptionController extends ContainerAware {

@@ -102,7 +102,7 @@ class ExceptionController extends ContainerAware {
     $system_path = $request->attributes->get('system_path');
     watchdog('access denied', $system_path, array(), WATCHDOG_WARNING);
 
-    $path = drupal_get_normal_path(config('system.site')->get('page.403'));
+    $path = drupal_container()->get('path_aliaser')->getSystemPath(config('system.site')->get('page.403'));

ExceptionController should be made ContainerAware, and then it won't need drupal_container() but can access the container directly. I have code in the routing branch that handles ContainerAware controllers that I may split off to a separate patch. Just marking this for reference.

+++ b/core/lib/Drupal/Core/Path/PathAliaser.php
@@ -0,0 +1,31 @@
+class PathAliaser implements PathAliaserInterface {
+
+  /**
+   * Implements \Drupal\Core\Path\PathAliaserInterface::getSystemPath().
+   */
+  public function getSystemPath($path, $path_language = NULL) {
+    return $path;
+  }
+
+  /**
+   * Implements \Drupal\Core\Path\PathAliaserInterface::getPathAlias().
+   */
+  function getPathAlias($path = NULL, $path_language = NULL) {
+    return $path;
+  }
+
+  /**
+   * Implements \Drupal\Core\Path\PathAliaserInterface::cacheClear().
+   */
+  public function cacheClear($source = NULL) {
+  }
+}

I think at this stage we should go ahead and leave the Aliaser class in core directly; I don't think we need to have a null implementation in core for now.

+++ b/core/modules/path/lib/Drupal/path/EventSubscriber/PathSubscriber.php
@@ -0,0 +1,49 @@
+/**
+ * Access subscriber for controller requests.
+ */
+class PathSubscriber extends PathListenerBase implements EventSubscriberInterface {
+
+  protected $aliaser;
+
+  public function __construct(PathAliaserInterface $aliaser) {
+    $this->aliaser = $aliaser;
+  }
+
+  /**
+   * Resolve the system path.
+   *
+   * @param Symfony\Component\HttpKernel\Event\GetResponseEvent $event
+   *   The Event to process.
+   */
+  public function onKernelRequestPathResolve(GetResponseEvent $event) {
+    $request = $event->getRequest();
+    $path = $this->extractPath($request);
+    $path = $this->aliaser->getSystemPath($path);
+    $this->setPath($request, $path);
+  }
+

Are you removing the existing alias code from the other path listener class? There's another subscriber already where this is happening. I don't much care if we spin this off to another subscriber or reuse the same one; that's a performance question as far as I'm concerned, so that should dictate it, but if both are still firing that could be a source of some test fails.

+++ b/core/modules/path/lib/Drupal/path/Path.php
@@ -0,0 +1,81 @@
+class Path {
+
+  public function __construct(Connection $connection) {
+    $this->connection = $connection;
+  }

$connection needs to be a defined object property.

+++ b/core/modules/path/lib/Drupal/path/Tests/LookupTest.php
@@ -0,0 +1,96 @@
+class LookupTest extends WebTestBase {
+  public static function getInfo() {
+    return array(
+      'name' => t('Path lookup'),
+      'description' => t('Tests that DrupalPathRegistry returns correct paths.'),
+      'group' => t('Path API'),
+    );
+  }
+

With an injected DB connection, this can all be unit tests, not webtests. Yes, it works. :-) If you need a hand setting that up let me know and I can walk you through it.

22 days to next Drupal core point release.

webchick’s picture

We're over thresholds on critical tasks, so features in D8 are currently blocked. Any chance this could be downgraded to "major"? Is it actually a release blocker?

webchick’s picture

Ah, according to #28, no. :(

katbailey’s picture

OK, this should deal with most of Crell's comments in #49.

We really really need to eliminate that case by moving more things into the Kernel, after the DIC initializes.

Yep, and now I'm making it even worse by requiring the database service in the bootstrap container :-( But I have created an issue to deal with this here #1784312: Stop doing so much pre-kernel bootstrapping.

ExceptionController should be made ContainerAware

It already is - I just hadn't realized. So I've changed that line to use $this->container instead of drupal_container().

I've moved everything back into the Path subsystem (i.e. no longer trying to move it off into path module).

I still haven't devised a decent unit test for this stuff.

I have at least made a start on tackling the problem that makes this issue critical: caching of system paths with a reliable key. In the bundles patch (#1599108: Allow modules to register services and subscriber services (events)), we changed the test at core/modules/path/lib/Drupal/path/Tests/PathAliasTest.php to check that the system path was cached using the alias as the cid, instead of the source. We had to do this because we knew the alias would be used as the cid during cache set because this happened outside of request scope and so a call to current_path() would not be able to return the system path.

To tackle this, I've moved the whole caching of system paths bit inside the AliasManager class. It grabs the system path at the start of a request, then at request close it uses that as the cid when storing the paths to the cache. During lookup it also uses the system path (obviously) as the cid when trying to retrieve from teh cache.

One thing I'm not sure about is whether the AliasManager class itself should maybe implement the EventSubscriberInterface, instead of PathSubscriber handing off to it like this...

katbailey’s picture

Status: Needs work » Needs review

Might as well find out where we're at with tests...

Status: Needs review » Needs work

The last submitted patch, 1269742.path_alias_manager.52.patch, failed testing.

Crell’s picture

Definitely the subscriber should be separate from AliasManager. AliasManager is an API. PathSubscriber is a bridge to connect two systems. That should not be tightly coupled into a single class.

I'll try to have a look at this soon if I can, but no guarantees. :-( (I have to prep for my next class this weekend.)

katbailey’s picture

OK, well just to explain why I was thinking that way... if the alias manager is going to take care of caching system paths, it needs to know when to do this, i.e. at request close (and it also needs to be told "here, this is the system path for the current page, use that as your cache key"). So right now I have the PathSubscriber calling startRequest() and endRequest() methods on the AliasManager but it seems totally wrong to have those in the AliasManagerInterface.

Crell’s picture

+++ b/core/lib/Drupal/Core/ExceptionController.php
@@ -102,7 +102,7 @@ class ExceptionController extends ContainerAware {
@@ -168,7 +168,7 @@ class ExceptionController extends ContainerAware {

@@ -168,7 +168,7 @@ class ExceptionController extends ContainerAware {
       $_GET['destination'] = $system_path;
     }
 
-    $path = drupal_get_normal_path(config('system.site')->get('page.404'));
+    $path = drupal_container()->get('path.alias_manager')->getSystemPath(config('system.site')->get('page.404'));

ExceptionController should also get the path manager injected, rather than calling drupal_container().

It may also make the patch easier to reroll if we put the drupal_container()-> call inside the existing functions for now rather than replacing them in all places. Once the actual patch goes in we can then remove the now-just-dumb-wrapper functions in a follow-up.

katbailey’s picture

Tagging for PNW sprint

ksenzee’s picture

Assigned: Unassigned » ksenzee

I worked on this at the PNWDS sprint. There are some genuine test failures that I'm still working on, so I'll assign it to myself in the unlikely case that someone comes by and decides to pick it up in the meantime.

ksenzee’s picture

Assigned: ksenzee » Unassigned
FileSize
76.75 KB

Apparently people at BADCamp need work to do so I'll give this up. I have not done any rearchitecting yet; I was hoping to get tests to pass first. They don't yet. Notes:

- This applies to HEAD as of October 30 (881edb14c9dfbf6d986704d19a307ac1041edaac).
- At some point the save() method was changed so that it no longer does a load() to check for the pid. This is breaking tests and actual functionality, and I'm not sure I agree that every call to save() should know whether it's creating a new alias or updating an existing one. We should be able to pass in a source and an alias and let save() take it from there.
- I added hook_path_insert() and hook_path_update() back in. I don't know where they belong, but leaving them out isn't a great option either.

Crell’s picture

katbailey’s picture

Status: Needs work » Needs review
FileSize
19.17 KB
81.53 KB

OK, I wanted to make some progress on the whole CacheDecorator idea we discussed as a solution to the problem mentioned in #56, plus I needed a break from the bootstrap mess - so here's an initial stab at rearchitecting this to use a CacheDecorator for caching system paths. I am still a bit confused about how this will work when it comes to subrequests.

Status: Needs review » Needs work

The last submitted patch, 1269742-62.alias-manager.patch, failed testing.

Crell’s picture

Status: Needs work » Needs review
+++ b/core/lib/Drupal/Core/CacheDecorator/AliasManagerCacheDecorator.php
@@ -0,0 +1,116 @@
+    // Because this class is only used by the PathSubscriber, when this method
+    // is called we know it is being called to get the system path of the
+    // current request. Therefore the result of the path lookup is the cache
+    // key we'll need to use when writing the system path cache. Pop it onto
+    // our stack of request paths.

I don't know that we can count on this assumption. There's nothing inherent in this class that says it may only be called from the subscriber; if it breaks if called from elsewhere, then it's still tightly coupled.

Rather, instead we should add a setCacheKey() method and have the subscriber set the cache key after it retrieves it.

+++ b/core/lib/Drupal/Core/CacheDecorator/AliasManagerCacheDecorator.php
@@ -0,0 +1,116 @@
+  public function setSystemPaths(array $system_paths) {
+    $this->systemPaths = $system_paths;
+    $this->aliasManager->setSystemPaths($system_paths);
+  }

It's unclear why we'd want to save the system paths twice. Needs a comment, or rethinking.

+++ b/core/lib/Drupal/Core/CoreBundle.php
@@ -79,6 +80,20 @@ public function build(ContainerBuilder $container) {
+    $container->register('cache_decorator.alias_manager', 'Drupal\Core\CacheDecorator\AliasManagerCacheDecorator')

This should be called something like path.alias_manager.cached, since it's still really part of the path system.

+++ b/core/lib/Drupal/Core/EventSubscriber/PathSubscriber.php
@@ -16,26 +18,33 @@
+  protected $aliasManagerCacheDecorator;

I don't think we need this to be stealth-hungarian named. :-) Just $aliasManager.

+++ b/core/lib/Drupal/Core/Path/AliasManager.php
@@ -0,0 +1,299 @@
+   * @param $action
+   *   One of the following values:
+   *   - wipe: delete the alias cache.
+   *   - alias: return an alias for a given Drupal system path (if one exists).
+   *   - source: return the Drupal system URL for a path alias (if one exists).

This is a red flag. It's probably a red flag in the existing code, but shoving 3 operations into one method is asking for trouble. For instance, wipe doesn't even belong to this object anymore.

+++ b/core/lib/Drupal/Core/Path/AliasManager.php
@@ -0,0 +1,299 @@
+      $this->whitelist= state()->get('system.path_alias_whitelist', NULL);

State system should be an injected dependency.

Although, should the whitelisting get moved to the cache object? That way all of the performance-futzing is in one place. Hm. Not sure. I defer to catch.

(And we'll likely be refactoring that when we merge this in with the generator anyway, so maybe we shouldn't stress about it now.)

+++ b/core/lib/Drupal/Core/Path/AliasManagerInterface.php
@@ -0,0 +1,52 @@
+  /**
+   * Sets the array of system paths that have been cached for the current path.
+   */
+  public function setSystemPaths(array $cached_paths);

Needs more complete docblock.

Or, needs to be removed entirely and this logic moved to the caching wrapper.

+++ b/core/lib/Drupal/Core/Path/Path.php
@@ -0,0 +1,91 @@
+      $query = $this->connection->insert('url_alias', array('return' => Database::RETURN_INSERT_ID))

RETURN_INSERT_ID is the default, I think. No need to set it here.

+++ b/core/lib/Drupal/Core/Path/Path.php
@@ -0,0 +1,91 @@
+      // @todo: Find a correct place to invoke hook_path_insert().
+      $hook = 'path_insert';

There's a hook for that??? Why???

If we really need to keep that functionality, switch it to an event listener.

+++ b/core/modules/system/lib/Drupal/system/Tests/Path/LookupTest.php
@@ -2,27 +2,27 @@
 class LookupTest extends WebTestBase {
   public static function getInfo() {
     return array(
       'name' => t('Path lookup'),
-      'description' => t('Tests that drupal_lookup_path() returns correct paths.'),
+      'description' => t('Tests that the alias manager returns correct paths.'),
       'group' => t('Path API'),
     );
   }

I'm pretty sure this entire class could get converted to a UnitTest, and then eliminate the reference to the container.

sun’s picture

$hook = 'path_insert';

There's a hook for that??? Why???

I don't interface with the insert/update hooks, so those might indeed be questionable (since they do not really provide sufficient context), but I am actively integrating with hook_path_delete() in #229568-34: Pathauto in Core (which just moved into #1751210: Convert URL alias form element into a field and field widget), so as to delete a referenced alias in field data values when it is deleted. In general though, I think it is a good idea to keep the low-level API separated, which means that it indeed should invoke CUD hooks. I don't really see why we'd have to move to event listeners here.

g.oechsler’s picture

I started hacking a UnitTest exercising CRUD operations on Path objects, basically by cargo culting from PathMatcherTest.

This one applies on top of 1269742-62.alias-manager.patch. I submit it here standalone to avoid getting in the way of katbailey. So it will surely fail on the bot, because of missing Path, AliasManager a.s.o.

Status: Needs review » Needs work

The last submitted patch, 1269742-path.crud-unitTest.patch, failed testing.

g.oechsler’s picture

Status: Needs work » Needs review
FileSize
9.38 KB

Just for the fun of it: This one contains a port of LookupTest to a unit test. I just added a method to CrudTest which breaks with the description and naming of it. But hey, as long as I have no feedback whether I'm on the right - er... - path, I won't care too much about wording.

This patch will fail like the one from #66 for the same reason!

Status: Needs review » Needs work

The last submitted patch, 1269742-path-unit-test.patch, failed testing.

catch’s picture

The whitelisting, I'm not sure where it lives yet, but it's going to end up looking like #1209226: Avoid slow query for path alias whitelists, not what we have now with the scary query + state. That means the dependencies for the class and some of the structure is going to end up different once that's fixed.

katbailey’s picture

Status: Needs work » Needs review
FileSize
45.81 KB
89.06 KB

This addresses #64, including refactoring the crazy lookup method.
Also, yay g.oechsler :-D Added in those unit tests with some minor changes, and got rid of the existing tests that they replace. Not sure what to do about the url_inbound_alter test - should the url_alter_test module provide its own bundle and register a path subscriber?

Status: Needs review » Needs work

The last submitted patch, 1269742.alias_manager.71.patch, failed testing.

Crell’s picture

sun:

In general though, I think it is a good idea to keep the low-level API separated, which means that it indeed should invoke CUD hooks. I don't really see why we'd have to move to event listeners here.

If we're going to have CUD events, then yes it should be separated. My statement was one of surprise that such CUD hooks even existed right now in the first place. If we keep them, then they really ought to change to events because events are cleanly injectable and hooks are not. (The same is true of the various entity CUD hooks, too; I haven't raised that question yet because the entity system is still being reshuffled daily, and it is, I think, a December-safe change.)

I looked over the interdiff in #71, and it looks a lot better. I'll have to review the full patch when I'm better rested. :-) Also, the new tests look schweet! :-)

Crell’s picture

+++ b/core/lib/Drupal/Core/Path/AliasManagerInterface.php
@@ -0,0 +1,59 @@
+  /**
+   * Returns an array of source lookups.
+   */
+  public function getSourceLookups();

What's a source lookup?

+++ b/core/lib/Drupal/Core/Path/AliasManagerInterface.php
@@ -0,0 +1,59 @@
+  /**
+   * Sets the system paths to use for bulk alias lookups.
+   *
+   * The cache decorator calls this method after doing a cache lookup of system
+   * paths for the current request path. Aliases can then be looked up for all
+   * these paths in bulk and are then available for subsequent lookups.
+   *
+   * @param $cached_paths
+   *   An array of system paths
+   */
+  public function setSystemPaths(array $cached_paths);

If the intent here is to pre-lookup a set of aliases, then setSystemPaths() seems like the wrong name. Rather, it's a "load all the aliases!" operation. So it's more like getPathAliases(), or preloadAliases(), or something like that. It's not a set operation at all. That's just a quirk of the way the cache decorator is using it. It's debatable if the cache decorator should even be mentioned here at all.

+++ b/core/lib/Drupal/Core/Path/Path.php
@@ -0,0 +1,91 @@
+class Path {

This whole class still needs docblocks.

Other than that (and the remaining test fails), I like this. We can get rid of those hooks in favor of events later.

g.oechsler’s picture

FileSize
835 bytes
89.88 KB

I tried to track down the bugs that made the tests fail and had a quite interesting tour through many place in core I haven't seen before. After two days I decided to rebase my test environment to latest 8.x dev and ... most of the tests seem to pass now!? Let's check this on the testbot.

Also I found a tiny thing in views_ui.module, so here's a new patch.

g.oechsler’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 1269742.alias_manager.75.patch, failed testing.

g.oechsler’s picture

I think I figured out what's going wrong, but sadly I have no solution for it.

There are four failing tests, three of them (ForumTest, ImageThemeFunctionTest and JavaScriptTest) pass, if I run them from the web interface of my machine. When I run them via run-tests.sh they fail just like on the bot.

Looks like the problem is in current_path:

function current_path() {
  // @todo Remove the check for whether the request service exists and the
  // fallback code below, once the path alias logic has been figured out in
  // http://drupal.org/node/1269742.
  if (drupal_container()->isScopeActive('request')) {
    return drupal_container()->get('request')->attributes->get('system_path');
  }

  // If we are outside the request scope, fall back to using the path stored in
  // _current_path().
  return _current_path();
}

When running with run-tests.sh isScopeActive('request') is never true, so we always fall back to _current_path() which produces different/wrong output and therefore failing tests.

Berdir’s picture

I'd suggest to check if the patch at #1784312: Stop doing so much pre-kernel bootstrapping is fixing this somehow.

katbailey’s picture

I'll look into this - thanks for pinpointing the problem, g.oechsler!

katbailey’s picture

Status: Needs work » Needs review
FileSize
8.67 KB
97.46 KB
Berdir’s picture

+++ b/core/lib/Drupal/Core/CacheDecorator/AliasManagerCacheDecorator.phpundefined
@@ -0,0 +1,123 @@
+<?php
+
+/**
+ * @file
+ * Definition of Drupal\Core\CacheDecorator\AliasManagerCacheDecorator.

The current standard for these @file docblocks is "Contains..." instead of "Definition of..."

+++ b/core/lib/Drupal/Core/EventSubscriber/PathSubscriber.phpundefined
@@ -16,23 +18,31 @@
+  /**
+   * Ensures system paths for the request get cached.
+   */
+  public function onKernelTerminate(PostResponseEvent $event) {
+    $this->aliasManager->writeCache();
   }
 
   /**
@@ -116,6 +126,7 @@ static function getSubscribedEvents() {

@@ -116,6 +126,7 @@ static function getSubscribedEvents() {
     $events[KernelEvents::REQUEST][] = array('onKernelRequestLanguageResolve', 150);
     $events[KernelEvents::REQUEST][] = array('onKernelRequestFrontPageResolve', 101);
     $events[KernelEvents::REQUEST][] = array('onKernelRequestPathResolve', 100);
+    $events[KernelEvents::TERMINATE][] = array('onKernelTerminate', 200);

Ah, this is exactly what I suggested in #512026-109: Move $form_state storage from cache to new state/key-value system for KeyValueDatabaseExpirable::garbageCollection(). Would be great if you could comment in there on what would the best way to do this. Not sure where and how we should register such an event. Same for the lock service, which has something like this too (see also me questions about the reliability of such an event in that comment)

katbailey’s picture

Lots of cleanup in this latest patch, should cover #74. I've created a follow-up issue for the CUD hooks, #1844354: Convert path alias CUD hooks to events.

Now, I should also explain some of the changes I made in the previous patch to get the remaining tests to pass:

  • the ones mentioned by g.oechsler in #78 that were failing only with run-tests.sh were a result of the removal of drupal_path_initialize(). That function initializes the path retrievable through _current_path() to the system path, so for an empty request path _current_path() returns the system path set for the home page. We don't need this anymore because we use a kernel request listener to resolve the home page to a system path. However, _current_path() still gets used when we're not in request scope and it should never return an empty string - this was confusing certain tests so I added code to WebTestBase to ensure it gets set to 'run-tests' if it's empty.
  • The tests for hook_url_inbound_alter: I had to completely overhaul these, adding a bundle and a PathSubscriber class to the test module to do its arbitrary path resolution thing. And I actually deleted one of the tests because it was just checking the above behavior of _current_path() which we do not use anymore in the context of an actual request.
disasm’s picture

I like the overall patch, and think it's much better than what we had before, but since you asked for nitpicky, I have two simple comments:

+++ b/core/lib/Drupal/Core/CacheDecorator/AliasManagerCacheDecorator.php
@@ -0,0 +1,123 @@
+      $expire = REQUEST_TIME + (60 * 60 * 24);

When you're doing math for calculating times, it's nice to have a comment like expire time of 24 hours.

+++ b/core/lib/Drupal/Core/Path/AliasManager.php
@@ -0,0 +1,324 @@
+   * @var String

lowercase string

Crell’s picture

+++ b/core/lib/Drupal/Core/Path/AliasManager.php
@@ -0,0 +1,324 @@
+    if ($path === NULL) {
+      $path = _current_path();
+    }

This makes me sad. :-( Why do we have to call out to a legacy function here? More specifically... in what situation should $path be an optional parameter? I can't see how this use case even exists, and if it does, I still hope there's a cleaner way than this.

+++ b/core/lib/Drupal/Core/Path/AliasManagerInterface.php
@@ -0,0 +1,60 @@
+  /**
+   * Given an internal Drupal path, return the alias set by the administrator.
+   *
+   * If no path is provided, the function will return the alias of the current
+   * page.
+   *
+   * @param $path
+   *   An optional internal Drupal path.

As above, I don't think this is a legitimate use case. If you want the current path, there are other ways to get it and then you can call getPathAlias() with an actual path. In 7 years of Drupaling I cannot recall a time when I wanted "get me the current aliased path"... or at least not one that I didn't know at the time was an ugly hack of evil in the theme layer that I felt sick doing. :-)

I think that's all I could find left. If we can resolve that, we can finally get it RTBC. woo!

katbailey’s picture

Crell’s picture

Status: Needs review » Reviewed & tested by the community

I think... it is time.

Dries’s picture

How about a helper function like drupal_service():

$normal_path = drupal_container()->get('path.alias_manager')->getSystemPath($path);

Would become:

drupal_service('path.alias_manager')->getSystemPath('foo');

Obviously, that would be tackled in another issue but I think it would improve DX.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Thanks!

Dave Reid’s picture

Status: Fixed » Needs work
Issue tags: +Needs change record

I don't see anymore how hook_url_inbound_alter() is getting invoked since drupal_get_normal_path() was removed, but the hook is still defined by system.api.php?

We could also use change records for how to replace all the removed functions? And possibly hook_url_inbound_alter() if the above is true?

I've also noticed that this removes the flexibility to pass additional information in along with the path aliases. In Drupal 7, we can do something like this:

$alias['source'] = $source;
$alias['alias'] = $alias;
$alias['langcode'] = $langcode;
$alias['pathauto'] = TRUE;
path_save($alias);

But now all the arguments are hard-coded:

drupal_container()->get('path.crud')->save($source, $alias, $langcode);

This seems like a regression which I know will affect a few contrib modules. Can the crud methods accept an array like it did with Drupal 7, rather than making it more of a DX regression?

katbailey’s picture

Issue tags: -Needs change record

Yes, hook_url_inbound_alter needs to be removed - I'll try and get to this tonight and write a change notice, including the new way to alter the inbound path.

Not sure what to say about the path_save() issue though... I mean, I wasn't aware that there was code that took advantage of something like that and it seems to me like an odd way to do things. If I could understand the use case a bit better I might be able to help figure out how to make it work with the new set-up.

katbailey’s picture

Issue tags: +Needs change record

Oops

Dave Reid’s picture

For example, it's used by Pathauto to indicate that this alias is an automatically generated alias and not a manual user-created alias. We also add in $path['original'] to show what the original alias was if it is being changed or updated, so that the Redirect module can automatically create redirects so that the original alias redirects to the new alias.

http://drupalcode.org/project/pathauto.git/blob/refs/heads/7.x-1.x:/path...
http://drupalcode.org/project/redirect.git/blob/refs/heads/7.x-1.x:/redi...

Dave Reid’s picture

I'm just not sure why path_save($path) was converted to essentially path_save($source, $alias, $language) when it would have been an easier conversion to leave it as an array. I guess it seemed to originate from:

I know this is a straight-port, but I want to try and avoid anonymous arrays as data objects. If Path is not going to be its own data object, then the CRUD functions should take multiple useful parameters rather than an anonymous data array.

But I would definitely consider url aliases their own data objects since we have path_load(). I'm not sure this change was justified.

catch’s picture

Whether they're data objects or not is really a discussion for #1553358: Figure out a new implementation for url aliases but I'm also not sure why it was changed here.

sun’s picture

Category: task » bug

@Dave Reid's argumentation and reasonings make sense to me. And he's definitely one of the most knowledgeable people in this field, especially when it comes to what contrib needs and is doing.

Changing passed/accepted arguments sounds like a quick fix though? Let's quickly adjust that.

sun’s picture

I'm confronted with these changes in #1751210: Convert URL alias form element into a field and field widget, and indeed, the new arguments to save() and delete() are non-trivial to work with. I would have expected to get the $pid as only, scalar return value, or as @Dave Reid says, accept an array as input, take it by reference, and populate 'pid' accordingly, as in the old code.

However, the following is actually much more critical:

+++ b/core/includes/bootstrap.inc
@@ -2490,6 +2490,10 @@ function drupal_container(Container $new_container = NULL, $rebuild = FALSE) {
+    $container->register('path.alias_manager', 'Drupal\Core\Path\AliasManager')
+      ->addArgument(new Reference('database'))
+      ->addArgument(new Reference('keyvalue.database'));

This hard-codes the Database implementation of the key/value service, which is an absolute no-go, since that ignores any possible service overrides, and consequently breaks all DrupalUnitTestBase tests.

Given that this part is effectively holding up aforementioned issue, which is bound to feature freeze, I'd appreciate if we could roll this back for the time being.

chx’s picture

Status: Needs work » Needs review
FileSize
1.56 KB

There are a few problems here but none of them seem to be worth a rollback. Once #1784312: Stop doing so much pre-kernel bootstrapping is in we can change keyvalue.database to keyvalue easily. That will immediately fix one of the problems, I simply can't care enough to fix it outside of that patch especially as I mentioned in that issue I had severe problems using DrupalUnitTestBase without it so trying to fix DUTB before that one I consider a futile exercise. The current mess of drupal_container() is not something I want to touch since I have such a beautiful fix over there in TestBundle.

The other one that Dave Reid raised is a valid point. Addressing it, however, while keeping katbailey's idea of meaningful parameters for IDE integration, better docs and all that is not particularly hard, just allow for passing in arbitrary additional things if you need them. Also, as I do not see at first sight what, if anything is using the return value of save, I just changed it to return the $pid.

Status: Needs review » Needs work
Issue tags: -Framework Initiative, -WSCCI, -Needs change record, -Dependency Injection (DI), -kernel-followup

The last submitted patch, 1269742_98.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
Issue tags: +Framework Initiative, +WSCCI, +Needs change record, +Dependency Injection (DI), +kernel-followup

#98: 1269742_98.patch queued for re-testing.

Note: the fail was, Setup environment: failed to clear checkout directory. Bot fluke.

sun’s picture

+++ b/core/lib/Drupal/Core/Path/Path.php
@@ -55,15 +55,15 @@ public function __construct(Connection $connection, AliasManager $alias_manager)
+  public function save($source, $alias, $langcode = LANGUAGE_NOT_SPECIFIED, $pid = NULL, $context = array()) {

Well, the other thing that's wonky to work with is the $pid argument.

For an existing path alias, the $pid should always be passed, and technically it should even come first, because it's the most important argument for an existing path alias.

I think that path aliases are indeed mini data objects - borderline entities even, although I wouldn't necessarily convert them into a true entity system entity.

If typed arguments are the issue, then we should have converted the array into a PathAlias object.

So, yeah, like others, I also don't really understand why this was changed as part of this patch.

katbailey’s picture

Crell’s picture

Given the length of this thread and that it's a critical, can we close it and move "clean up Path CRUD API" to a new issue? There's probably plenty of cleanup to be done there, and we already have one follow-up for that to shift the hook calls to events.

catch’s picture

@Crell I think that's #1553358: Figure out a new implementation for url aliases, I've bumped it to major. I could be persuaded it's critical since it blocks deployment of path aliases and the (unintentional) API usage regression here.

And yeah let's please move that discussion there - that issue stalled some time ago and if we'd persisted there we'd not be having this conversation now.

sun’s picture

I'm not 100% sure yet (as it's not exactly easy to follow the new code flow), but I suspect that this patch additionally resulted in the path alias cache getting cleared/refreshed too late — i.e.,

When entering a URL alias in the node edit form and saving it, you are redirected to the non-aliased path (e.g., node/1). Only on that second request, when hovering over the default "View" tab of the shown node, the new alias is known, and clicking the tab gets you to the aliased node path.

Previously, the new URL alias took effect immediately; i.e., adjusting the redirect URL of the form submission already.

Crell’s picture

Title: Make path lookup code into an pluggable class » Change Notice: Make path lookup code into an pluggable class

sun: Please file that as a new issue with a patch and let's tackle that there.

This issue still needs a change notice, so not closing yet but anything that's not a change notice please file as a follow-up issue.

marcingy’s picture

Category: bug » task
sun’s picture

Please file that as a new issue with a patch

Thanks, but that's not exactly the way I understand collaborative improvements — if my changes break stuff or have unintended consequences, then I understand it as my responsibility to ensure that the system stays functional for others and fix it accordingly, so I'll leave that to you folks. If that differs from your understanding, then I can't help. It took me long enough to debug this change and work around it for other issues already.

bforchhammer’s picture

Entity translation code still has a call to path_delete() which breaks the ability to delete translations: #1852394: Fatal error: Call to undefined function path_delete().

A change notice with instructions on how to fix it would be appreciated :)

katbailey’s picture

Title: Change Notice: Make path lookup code into an pluggable class » Make path lookup code into a pluggable class
Issue tags: -Needs change record

Added a change notice: [#1853148]. Not sure what the status of this issue should be now :-/

aspilicious’s picture

Status: Needs review » Fixed

The change notice is prety clear and easy to understand. As all the remaining problems are moved to other issues I'm going to mark this fixed. Please reopen if I misunderstood some comments.

Thnx :)

Dave Reid’s picture

I do not consider the issue of path aliases are no longer passed as arrays solved by #1553358: Figure out a new implementation for url aliases which is a much larger architectural decision. Filed #1854284: Path aliases are no longer arrays and cannot pass additional data to path hooks as a major bug report.

Status: Fixed » Closed (fixed)

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

effulgentsia’s picture

How about a helper function like drupal_service():

#1875086: Improve DX of drupal_container()->get()

tim.plunkett’s picture

This issue is still referenced by current_path(). What is the new issue for that?

amateescu’s picture

Wondering about the same thing as #115.

amateescu’s picture

Issue summary: View changes

Updated issue summary.

yched’s picture

Bump on #115

function current_path() {
  // @todo Remove the check for whether the request service exists and the
  // fallback code below, once the path alias logic has been figured out in
  // http://drupal.org/node/1269742       <--- THIS ISSUE HERE
  if (\Drupal::getContainer()->isScopeActive('request')) {

:-)

sun’s picture