Path field should fall back to language neutral aliases (also makes this happen for the form widget!)

On monolingual sites, add aliases with 'en' langcode.

OR

When looking for aliases for the node form, fall back to language neutral if there is not an alias in the language

Given that 'en' is used as the langcode of the entity itself, I think there is no reason that the path alias should not inherit that automatically.

Given that the same UI behaviour should happen for the path alias as well, always at least have one langcode available.
This could also ease the transaction into path aliases as entities in the future.

That respects the langcode and falls back to unspecified.

Forgot to add some assertions for the overview page.

1. index 239588f..b14e9b7 100644
   --- a/core/modules/path/src/Plugin/Field/FieldType/PathItem.php
   
   --- a/core/modules/path/src/Plugin/Field/FieldType/PathItem.php
   +++ b/core/modules/path/src/Plugin/Field/FieldType/PathItem.php
   
   +++ b/core/modules/path/src/Plugin/Field/FieldType/PathItem.php
   @@ -30,6 +30,8 @@ public static function propertyDefinitions(FieldStorageDefinitionInterface $fiel
   
   @@ -30,6 +30,8 @@ public static function propertyDefinitions(FieldStorageDefinitionInterface $fiel
          ->setLabel(t('Path alias'));
        $properties['pid'] = DataDefinition::create('integer')
          ->setLabel(t('Path id'));
   +    $properties['langcode'] = DataDefinition::create('string')
   +      ->setLabel(t('Path langcode'));
        return $properties;
   

   I'm wondering whether we should provide an update path and set the langcode on there

2. +++ b/core/modules/path/src/Tests/PathLanguageUiTest.php
   @@ -78,4 +80,28 @@ function testNonDefaultUrl() {
   +  function testNotSpecifiedNode() {
   

   nitpick: we could add a "public" here.

1. set /where? This is a computed field type, this information not persisted anywhere anymore. It also wouldn't strictly be required, the patch works without it. But we will eventually also support loading an existing alias I hope, and then it might become useful.

2. Yeah :) I blame the existing methods which don't have this for forgetting it.

1. set /where? This is a computed field type, this information not persisted anywhere anymore. It also wouldn't strictly be required, the patch works without it. But we will eventually also support loading an existing alias I hope, and then it might become useful.

Oh right, nevermind.

+++ b/core/modules/path/src/Plugin/Field/FieldType/PathItem.php
@@ -54,7 +56,7 @@ public function postSave($update) {
-        if ($path = \Drupal::service('path.alias_storage')->save('/' . $entity->urlInfo()->getInternalPath(), $this->alias, $this->getLangcode())) {
+        if ($path = \Drupal::service('path.alias_storage')->save('/' . $entity->urlInfo()->getInternalPath(), $this->alias, $this->langcode ?: $this->getLangcode())) {

@@ -67,7 +69,7 @@ public function postSave($update) {
-        \Drupal::service('path.alias_storage')->save('/' . $entity->urlInfo()->getInternalPath(), $this->alias, $this->getLangcode(), $this->pid);
+        \Drupal::service('path.alias_storage')->save('/' . $entity->urlInfo()->getInternalPath(), $this->alias, $this->langcode ?: $this->getLangcode(), $this->pid);

This change looks odd. Should \Drupal\path\Plugin\Field\FieldType\PathItem() override \Drupal\Core\Field\FieldItemBase::getLangcode() and contain this logic? Maybe getLangcode() needs to always return the parent langcode - not sure.

getLangcode() is the langcode of the entity. It is a public method, overriding that feels wrong.

getLangcode() follows specific rules. You get the active translation, but if the field is not translatable then it is the language of the original translation. It is never not specified unless the entity actually has not specified as the langcode.

There will be additional changes with #2689459: If you don't want to translate your URL alias, the original URL alias won't work with your translations, basically, if the field is not translatable then we will use language not specified and not the language of the original entity as the default in the widget, so that the alias is available in all languages.

It made sense to me to split that up, if you think it makes more sense to combine the two issues, then we can also do both things together. The other issue would basically do a isTranslatable() check and if not, us not specified. I'm not sure yet about changing it when saving or respecting the existing langcode, a this now does.

And I kept the getLangcode() as a BC fallback.

Yet another related issue is loading lazy loading existing aliases when accessing the data, which will mean support for REST, search api, rules and other systems which use field api/typed data in a generic way.

Back to you for feedback, feel free to set back. @catch might also have thoughts on this, as he opened the issue.

  /**
   * Gets the langcode of the field values held in the object.
   *
   * @return string
   *   The langcode.
   */
  public function getLangcode();

Above are the docs on \Drupal\Core\Field\FieldItemInterface::getLangcode() - so whilst this is normally the same as the entity's langcode in this case it does not appear to be because PathItem has its own langcode. Tricky.

Tried to document this behavior a bit but failed to come up with a concise description why we didn't override getLangcode() (which might prove that it would maybe make sense? I'm not sure..)

While thinking about this, this issue, in combination with #2689459: If you don't want to translate your URL alias, the original URL alias won't work with your translations could result in some interesting problems. Imagine a single language site, the path field is not translatable. With that other issue, we save the alias initially as unspecified. Then we add another language later and enable translations, making the field now translatable. Now we will find the existing unspecified alias when adding a translation and will update that instead of saving a new one in the translation language. We can figure out a solution in the other issue, possibly make the langcode visible to the user, so he can change it.

1. +++ b/core/modules/path/src/Plugin/Field/FieldType/PathItem.php
   @@ -30,6 +30,8 @@ public static function propertyDefinitions(FieldStorageDefinitionInterface $fiel
   +    $properties['langcode'] = DataDefinition::create('string')
   +      ->setLabel(t('Path langcode'));
        return $properties;
   

   Sometimes it would be nice to document when to use this new langcode and when not.

2. +++ b/core/modules/path/src/Plugin/Field/FieldWidget/PathWidget.php
   @@ -34,7 +34,14 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
          $path = \Drupal::service('path.alias_storage')->load($conditions);
   ...
   +          $path = \Drupal::service('path.alias_storage')->load($conditions);
   

   In case someone wants to, we could store \Drupal::service('path.alias_storage') as local variable.

3. +++ b/core/modules/path/src/Tests/PathLanguageUiTest.php
   @@ -78,4 +80,28 @@ function testNonDefaultUrl() {
   +
   +    $this->drupalGet($node->toUrl('edit-form'));
   +    $this->assertFieldByName('path[0][alias]', $edit['alias']);
   +    $this->drupalPostForm(NULL, [], t('Save'));
   +
   +    $this->drupalGet('admin/config/search/path');
   +    $this->assertText(t('None'));
   +    $this->assertNoText(t('English'));
   

   I'm wondering whether we should test also the aliases directly, like going to the path aliases itself.

Needs a reroll as some of the tests have been moved to phpunit.

It's a bit more complicated than that :)

"Rebased", which is more like a rewrite as this now can and has to be in PathItem instead of the widget.

The thing now is that the duplicated load always happens on accessing the value, which is likely more often than the edit widget.

Thinking about extending load to support array values in the condition and then have the same language sorting logic as the lookup methods.

Not addressed the review yet.

+++ b/core/modules/path/src/Plugin/Field/FieldType/PathItem.php
@@ -164,12 +170,17 @@ protected function ensureLoaded() {
+            $conditions['langcode'] = LanguageInterface::LANGCODE_NOT_SPECIFIED;

This loads the first alias found of the 'und' language, since $conditions isn't previously set.

This patch resolves that, but we need further test coverage since this was previously green.

Here's an updated test that demonstrates the issue with #22. The 'test only' patch is actually that patch with only the updated test, and the full patch contains the updated test and the change in #24.

Just a note: at the moment, this patch conflicts with the latest patch in 2689459. It would be great to either get this committed so 2689459 can be re-rolled or to combine the patches so that they can be used together.

The patch is also not compatible with the recent commit in #2916300: Use ComputedFieldItemListTrait for the path field type so needs a refactor.

I'm going to work on the refactoring of the fix.

Here's the refactored fix (no interdiff since the previous patch no longer applied).

This fixes coding standards. The fail in #30 looks to be unrelated/random:

Exception: Warning: apcu_store(): Unable to allocate memory for pool.

This has two failures on the 8.6.x branch.

Reroll of the #33.

+++ b/core/modules/path/src/Plugin/Field/FieldType/PathFieldItemList.php
@@ -26,12 +27,18 @@ protected function computeValue() {
+      if ($alias === FALSE) {
+        // Fall back to non-specific language.
+        if ($this->getLangcode() !== LanguageInterface::LANGCODE_NOT_SPECIFIED) {
+          $conditions['langcode'] = LanguageInterface::LANGCODE_NOT_SPECIFIED;
+          $alias = \Drupal::service('path.alias_storage')->load($conditions);
+        }

I believe this would also address the unexpected/confusing behavior reported against JSON:API in #3012019: Include the path / url alias and #3015566: Missing path attribute?

This approach is new as of #20, and means the issue title should probably be updated.

+++ b/core/modules/path/src/Plugin/Field/FieldType/PathItem.php
@@ -63,10 +63,15 @@ public function preSave() {
+    // If specified, rely on the langcode property for the language, so that the
+    // existing language of an alias can be kept. That could for example be
+    // unspecified even if the field/entity has a specific langcode.
+    $alias_langcode = $this->langcode ?: $this->getLangcode();

It seems like we set the the language property of the PathItem to Drupal's default in PathFieldItemList::computeValue() when we create the form, so the $this->language is always populated and it doesn't respect in which language the entity is created, hence the Test error.

This patch fixes #41 by checking if path alias has pid value before using the langcode property.

One problem still remains - when url alias field is configured as non-translatable, whenever you add an entity translation it will create a new url alias in the source language of the entity. Ex. if I create node in english, set the path alias, then decide to add a polish translation and save a form I get two rows in the DB:

16	/node/15	/test	en
17	/node/15	/test	en

I believe it should be fixed in #2689459: If you don't want to translate your URL alias, the original URL alias won't work with your translations as it only applies to the non-translatable aliases.

Not sure why CI decided to fail on this - all tests are green now.

This looks good to me, would be great to get the ball rolling and improve url alias handling.

+++ b/core/modules/path/src/Plugin/Field/FieldType/PathFieldItemList.php
@@ -26,12 +27,18 @@ protected function computeValue() {
     $entity = $this->getEntity();
     if (!$entity->isNew()) {
-      // @todo Support loading language neutral aliases in
-      //   https://www.drupal.org/node/2511968.
-      $alias = \Drupal::service('path.alias_storage')->load([
+      $conditions = [
         'source' => '/' . $entity->toUrl()->getInternalPath(),
         'langcode' => $this->getLangcode(),
-      ]);
+      ];
+      $alias = \Drupal::service('path.alias_storage')->load($conditions);
+      if ($alias === FALSE) {
+        // Fall back to non-specific language.
+        if ($this->getLangcode() !== LanguageInterface::LANGCODE_NOT_SPECIFIED) {
+          $conditions['langcode'] = LanguageInterface::LANGCODE_NOT_SPECIFIED;
+          $alias = \Drupal::service('path.alias_storage')->load($conditions);
+        }
+      }
 

There is still the performance aspect to consider here. I'm wondering if this could be done like the main alias lookup. Look for both language specific and neutral alias in a single load, if there is more than one alias, prefer the one with the matching language.

I'm wondering if this could be done like the main alias lookup. Look for both language specific and neutral alias in a single load, if there is more than one alias, prefer the one with the matching language.

This would indeed be much better for performance.

That would be very easy to do after #2336597: Convert path aliases to full featured entities since we'll be able to use an entity query with an OR condition on the langcode field :)

There is still the performance aspect to consider here. I'm wondering if this could be done like the main alias lookup. Look for both language specific and neutral alias in a single load, if there is more than one alias, prefer the one with the matching language.

Do you mean an additional method in AliasStorageInterface that combines load and lookupPathAlias together? That sounds like a BC break so I guess we would need another service for that?

#49 would be great but probably won't go into core before the next minor release?

I'm also not sure about the performance impact - the code doesn't seem to run in the critical path (does it?). All API additions we may wanna add are soon to be deprecated by #2336597: Convert path aliases to full featured entities. All that makes me wonder - wouldn't be enough to just leave a comment and create the followup to refactor this part once #2336597 gets in?

Agreed with #51, I think the current patch is fine for 8.6.x given that we know that it can be improved in 8.7.x.

Done.

That was fast ;)

> I'm also not sure about the performance impact - the code doesn't seem to run in the critical path (does it?

I'm not 100% sure about that. I did just check that at least by default, it doesn't do that, but because this is an implicit load of the field, all it takes is some code somehow accessing it to trigger that.

But fine :)

I did comment in the follow-up that I disagree with @amateescu about the entity query, though :)