Make the PathItem field type actually computed and auto-load stored aliases

Yay! dawehner++

+++ b/core/modules/path/src/PathAliasComputed.php
@@ -0,0 +1,47 @@
+      $langcode = $langcode ?: $entity->language()->getId();
+      $alias = $this->getAliasStorage()->lookupPathAlias('/' . $entity->toUrl()->getInternalPath(), $langcode);

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

One of the two issues will need to be updated once this lands.

What about actually relying on it and removing the custom loading in the widget? That should give us a lot of integration test coverage, we probably still want a unit/kernel test on top if it then..

+++ b/core/modules/path/src/Plugin/Field/FieldWidget/PathWidget.php
@@ -28,14 +28,7 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
+      $path = $items->first()->get('alias') ?: [];

This should get the item based on the delta maybe?

Yeah, we talked about this. +++ If we caan solve this properly and remove any need for special normalizers.

Ok after chatting with @Berdir and @dawehner it seems that all the properties need to be "computed" not just "alias" but langcode and pid to.
Because in #10 after a small fix the PathWidget alias was being loaded but not pid. So the alias would save but always create a new row in url_alias table.
So instead of creating 3 properties that are computed which would require 3 queries and get out off sync it might be better to handle this overriding __get and getValue.
So that is what this patch does.

But basically for both those overrides it just calls a new protect method ensureLoaded() which will load if needed then call the parent.

Included the chat below but important part to not lose track of is

berdir: tedbow: in fact, such a pattern could be supported by default for computed fields. but I think it makes sense to just do it for path (we already have plenty of dependant and related issues..) and when it works for us, we can propose it in the "figure out what the hell a computed field means exactly"-issue ;)

Full discussion

6:26 tedbow: dawehner_: was looking at PathWidget and trying to get it save the alias. can get it to work but then always saves new alias not update the last which is left in the table
16:29 berdir: tedbow: dawehner_: yeah, we have to load and set pid too, good point
16:29 berdir: as well as langcode, as soon as the other issue about support for language fallback and saving with a certain langcode lands
16:30 tedbow: dawehner_: so would that mean that all of these properties have be computed?
16:31 tedbow: berdir: ^
16:32 berdir: tedbow: yeah.. but we have to load those things together of course as we don't want to do 3 different queries, which might get inconsistent. And that's exactly where computed fields/properties get complicated.. as there's not *really* an API for that at the moment
16:32 tedbow: berdir: ah, yeah was looking at that. thanks for the clarification
16:32 dawehner_: berdir: tedbow so does that mean we should store and get the result from the field item and just use it from within the computed fields?
16:33 berdir: tedbow: the problem is that there are really two methods that are used to access data, getValue() and __get(). what you would have to do is override both, add an isLoaded flag, and if FALSE, try to load data and set it on the different properties
16:33 berdir: then we don't even need custom computed property classes anymore
16:34 tedbow: berdir: because it would handled on the fieldITem level not the property level?
16:35 berdir: tedbow: yeah
16:36 tedbow: berdir: fun stuff!
16:37 berdir: tedbow: in fact, such a pattern could be supported by default for computed fields. but I think it makes sense to just do it for path (we already have plenty of dependant and related issues..) and when it works for us, we can propose it in the "figure out what the hell a computed field means exactly"-issue ;)
16:38 tedbow: berdir: so for now that would mean in \Drupal\path\Plugin\Field\FieldType\PathItem::propertyDefinitions ->setComputed(TRUE) on all properties but no ->setClass(*) ?
16:40 berdir: tedbow: yeah, although technically, it wouldn't even be required anymore to do that

At least some of the tests failed because of ensureLoaded() was trying to get call \Drupal\Core\Entity\Entity::toUrl() on new entities. At that point they don't have id so we can't get a URL.

#7 is still pending.

Re #17 yes I am sure this needs a lot of work ;)

This patch include some ideas for REST issues. Related to #2649646: Normalize path fields as part of the entity: allow REST clients to know the path alias
This issue is interesting because if we could get this to work we would not need special normalizer for path fields.

This patch contains a bunch of todo's because I just testing some stuff out here.

1. +++ b/core/modules/path/src/Plugin/Field/FieldType/PathFieldItemList.php
   @@ -34,4 +34,22 @@ public function delete() {
   +  public function getValue($include_computed = FALSE) {
   +    /**
   +     * @todo Is there better way to do this???
   +     * This is needed because:
   +     * 1. for this class to work with \Drupal\serialization\Normalizer\ListNormalizer::normalize
   +     *    I think because \Drupal\Core\Entity\ContentEntityBase::__sleep call $field->getValue()
   +     *    otherwise this will always return empty.
   +     * 2. For $node->path->getValue() to work.
   +     *      getValue() will work if you just saved node with the path
   +     *      But if you try node_load($node->id(), TRUE); getValue() will NOT work.
   +     */
   +    $this->first()->getValue();
   +    return parent::getValue($include_computed);
   +  }
   

   Is this just a hack? Is there better way?

2. +++ b/core/modules/path/src/Plugin/Field/FieldType/PathItem.php
   @@ -34,10 +34,20 @@ class PathItem extends FieldItemBase {
   +      /**
   +       * @todo If we set to computed then:
   +       *   1. In order for the value to work with \Drupal\serialization\Normalizer\ComplexDataNormalizer::normalize
   +       *       We have to override getIterator as commented out below to set $include_computed to TRUE.
   +       *   2. preSave() and postSave() methods in this class will not be called and therefore the you cannot
   +       *       create an alias with $entity->set('path',$values) or Node::create('path'=> $values)
   +       */
   +      //->setComputed(TRUE)
   

   Using setCompute(TRUE) would cause these problems. I think it goes back to @Berdir point above IRC about what exactly is "computed"

3. +++ b/core/modules/rest/tests/src/Functional/EntityResource/Node/NodeJsonAnonPathyTest.php
   @@ -0,0 +1,97 @@
   +class NodeJsonAnonPathyTest extends NodeJsonAnonTest {
   

   This is test is just to try setting path and have the REST GET request retur it. Without the changes in this patch this test should fail. It would also fail in #16

1. +++ b/core/modules/path/src/Plugin/Field/FieldType/PathFieldItemList.php
   @@ -34,4 +34,22 @@ public function delete() {
   +    /**
   +     * @todo Is there better way to do this???
   +     * This is needed because:
   +     * 1. for this class to work with \Drupal\serialization\Normalizer\ListNormalizer::normalize
   +     *    I think because \Drupal\Core\Entity\ContentEntityBase::__sleep call $field->getValue()
   

   not sure If I understand this yet, but I'll try to have a look at what's going on myself.

2. +++ b/core/modules/path/src/Plugin/Field/FieldType/PathItem.php
   @@ -34,10 +34,20 @@ class PathItem extends FieldItemBase {
   +       * @todo If we set to computed then:
   +       *   1. In order for the value to work with \Drupal\serialization\Normalizer\ComplexDataNormalizer::normalize
   +       *       We have to override getIterator as commented out below to set $include_computed to TRUE.
   +       *   2. preSave() and postSave() methods in this class will not be called and therefore the you cannot
   +       *       create an alias with $entity->set('path',$values) or Node::create('path'=> $values)
   +       */
   

   I get the first, but not sure about the second, why would that not be called?

I am not sure if part of this issue is to path field be able to update and inserted like other fields.
Below is some code I was playing around with #18 the debug statements output the path array.
In #16 or 8.3.x they output and empty array.

$node = Node::create([
      'title' => 'tester',
      'type' => 'article',
    ]);
    $node->set('path', ['alias' => '/lewis-carrol' . random_int(1,100000), 'langcode' => 'en']);
    $node->save();
    $nodeLoaded = node_load($node->id(), TRUE);
    debug($nodeLoaded->path->getValue(), 'path save during insert');

    // try to update the path
    $nodeLoaded->set('path', ['alias' => '/george-eliot' . random_int(1,100000), 'langcode' => 'en']);
    $node->save();
    $nodeLoaded = node_load($nodeLoaded->id(), TRUE);
    debug($nodeLoaded->path->getValue(), 'path save during update');

Re @berdir in #20

I get the first, but not sure about the second, why would that not be called?

Yeah I am not sure but if uncomment the setComputed(TRUE) lines the code above breaks. Meaning you can't do
$node->set('path', ['alias' => '/lewis-carrol' . random_int(1,100000), 'langcode' => 'en']);

Also if set a break point in those functions it is never hit.

Though for me it kind of makes sense that preSave() and postSave() would not be called for "computed" fieldItems. I mean you "save" computed fields do you?

Yeah. This is the can of worms I was referring to before :) You need to start passing the parameter to include computed fields and that messes with normalisation quite a bit. We would have to opt in globally? Unless we introduce new field list normalizers that handle it? Could we not just stick with custom storage and just implement more logic in the actual list item class to handle the loading etc maybe? Or at least do what's needed to instnatiate the pathItem correctly, as isEmpty would need to be handled properly too.

I started a kernel test to test various different things around setting, saving and loading the computed path field.

I think it makes sense to have the same special cases that we have in ItemList::get() for a computed field to also have in getValue(), isEmpty() and possibly other methods. I added that to PathFieldItemList for now. I also added the ensureLoaded() call to isEmpty on PathItem.

My kernel test passes now with this, but I might not be covering all cases yet and I also didn't clean up all those comments/questions/commented code. Enough for tonight :)

I suggest you add more assertions to the test if things still aren't fully working for the normalizers, based on what those need.

Ideally, this would be fixed before #2649646: Normalize path fields as part of the entity: allow REST clients to know the path alias. But in doing so, this issue must ensure to NOT show the path in serializations of entities, to prevent that #2649646: Normalize path fields as part of the entity: allow REST clients to know the path alias would change it again. We should start exposing paths in our serializations only once they're ready, otherwise we break BC.

@Wim: I think fixing this would basically make the other issue a duplicate or maybe we can just keep it to add better tests, it should "just work" then, for both hal and json and every other format. It will behave as if a value was actually stored in the field.

This fixes the language test, but it is a bit shaky. The problem is that adding a new translation calls toArray() on the existing entity to use as values for the new translation, that loads it including ID and langcode, which we then update and as a result update the existing alias instead of saving a new one.

Might be better to check that in setValue(). Will also get a bit more complicated with #2511968: Path field should fall back to language neutral aliases (also makes this happen for the form widget!), when we'll actually support a fallback to undefined.

Also did some more cleanup in the PathItem and PathWidget classes.

@Wim: Would be awesome if you could provide tests for the path/rest stuff, I guess it would be best if we add it there then we can just close the other. I have no overview of the new test structure yet, so you're probably much faster.. just forget that path is computed, write a test as if path would be a normal field by just writing and accessing its value as everything else. @tedbow already started something here but some @todo's and workarounds there should no longer be necessary now.

Note that this structure not just includes the alias like the other issue but also the ID and langcode. And the ID is required to be able to update an existing alias, but it could of course also cause conflicts/overwrites if an ID is already used. But since aliases do not have UUID's, I don't see how we else we could do that.

+++ b/core/modules/path/src/Plugin/Field/FieldWidget/PathWidget.php
@@ -50,22 +32,22 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
+ '#value' => $items->langcode,

When creating a new translation
$items->langcode will equal to the source translation.

$items->getLangcode()
can be used instead because it will always equal to langcode of fielditem/entity

+++ b/core/modules/path/src/Plugin/Field/FieldWidget/PathWidget.php
@@ -50,22 +32,22 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
+      '#value' => $items->pid,

If langcode was fixed then this would start to cause test failure. When creating a translation the pid should be set to null.

Here is patch that should fix this.

#re @Wim Leers' comment in #27

We should start exposing paths in our serializations only once they're ready, otherwise we break BC.

I can confirm what @Berdir said the path field will just start to work normalization with these changes.
I created NodeJsonAnonPathyTest to test this out. Basically just enable the path module on Node Rest test.

Sorry this is just temporary solution. I can improve/remove it tomorrow.
I think we have to do some explicit stuff in the normalizer to make path NOT show up.

OK, so this works and the field items are included in normalized output already because the path field (top level) is set to computed, but the items within the field are not? Otherwise, the ComplexDataNormalizer would not include these properties. Right?

+++ b/core/modules/path/src/Plugin/Field/FieldType/PathFieldItemList.php
@@ -34,4 +34,27 @@ public function delete() {
+    // Automatically create the first item for computed fields.
+    // @todo: Move this to the base class.
...
+    // Automatically create the first item for computed fields.

Let's create a follow-up issue and can we please add to second comment as well?

re #32 @damiankloip yes that is what look like to me from stepping through.

#33
Ok create follow up #2848418: Automatically create first items for computed fields in FieldItemList and add to comments

Also deleted [#NodeJsonAnonPathyTest]
This was just rough test to see if this would automatically work with normalization without any changes.

@damiankloip suggested in IRC repurposing #2649646: Normalize path fields as part of the entity: allow REST clients to know the path alias to actually add normalization testing.

We already have #2392845: Add a trait to standardize handling of computed item lists, we could use that as the follow-up issue to make this more generic.

Random test failure because of #2848177: More \Drupal\Tests\outside_in\FunctionalJavascript\OutsideInBlockFormTest random fails
And yes I wrote that randomly failing test :(

Generally this is looking great! Nice work all.

1. +++ b/core/modules/path/src/Plugin/Field/FieldType/PathFieldItemList.php
   @@ -34,4 +34,28 @@ public function delete() {
   +    // @todo: Move this to the base class in https://www.drupal.org/node/2848418.
   

   Based on berdirs comment, maybe this issue URL needs to be updated. If we agree it makes sense to use that issue. Seems legit to me.

2. +++ b/core/modules/path/src/Plugin/Field/FieldType/PathFieldItemList.php
   @@ -34,4 +34,28 @@ public function delete() {
   +    if (!isset($this->list[0]) && $this->definition->isComputed()) {
   

   We can put this into a method that is reused by both isEmpty() and getValue()? ensureLoaded()? Similar to the PathItem class maybe.

3. +++ b/core/modules/path/src/Plugin/Field/FieldType/PathItem.php
   @@ -88,4 +130,21 @@ public static function mainPropertyName() {
   +    if (!$this->isLoaded && !$entity->isNew()) {
   

   Let's check isLoaded before anything else and just return early otherwise. Then do the next conditional that checks the entity.

4. +++ b/core/modules/path/src/Plugin/Field/FieldType/PathItem.php
   @@ -88,4 +130,21 @@ public static function mainPropertyName() {
   +        'langcode' => $this->getLangcode(),
   

   Not sure if it makes more sense (semantically at least) if this comes from the entity too? or is there a specific reason that cannot happen? When translations are added etc.. (related to the above comment about langcodes)? If so, maybe a quick comment to clarify this for our future selves.

5. +++ b/core/modules/path/src/Plugin/Field/FieldWidget/PathWidget.php
   @@ -50,22 +32,28 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
   +    if (isset($content_translation['target'])) {
   

   The fix in the PathItem for the language is needed as well as this?

6. +++ b/core/modules/path/tests/src/Kernel/PathItemTest.php
   @@ -0,0 +1,109 @@
   +  public static $modules = array('path', 'node', 'user', 'system');
   

   Nit: new array syntax. The rest of the file seems to be using it.

7. +++ b/core/modules/path/tests/src/Kernel/PathItemTest.php
   @@ -0,0 +1,109 @@
   +  protected function setUp() {
   

   Do we put a docblock here for these setUp methods or not? I forget now.

8. +++ b/core/modules/path/tests/src/Kernel/PathItemTest.php
   @@ -0,0 +1,109 @@
   +    \Drupal::service('router.builder')->rebuild();
   

   Is this needed in here?

@damiankloip thanks for the review!
1. Fixed point to existing issue and closed #2848418: Automatically create first items for computed fields in FieldItemList

2. Created the new method

3. Fixed

4. Getting the langcode from the entity makes sense. Add a comment about why this is necessary.

5. If you are referring to this

if ($this->pid && !$this->alias) {
        \Drupal::service('path.alias_storage')->delete(array('pid' => $this->pid));
      }

No actually I just checked and this is not necessary anymore.
Removed it.
The only reason to leave it I think would be assist a new translation being made programmatically.
6. fixed
7. Yes, add inheritdoc docblock
8. Not @Berdir added that. Don't see why it is needed.
UPDATE: I just checked and removing this line does not cause PathItemTest to fail. I didn't check before made the last patch :(

Removing \Drupal::service('router.builder')->rebuild(); call in test

But also

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

I there a reason we aren't adding "source" here as a property?

I thought about that, but I'm not sure if it makes sense/is necessary as it is really just the internal path of the entity itself.

Here's a review from somebody who knows little about Entity Field API internals. Hopefully it's useful and not full of stupid questions.

1. +++ b/core/modules/path/src/Plugin/Field/FieldType/PathFieldItemList.php
   @@ -34,4 +34,31 @@ public function delete() {
   +    $this->ensureLoaded();
   ...
   +    $this->ensureLoaded();
   ...
   +  /**
   +   * Automatically create the first item for computed fields.
   +   *
   +   * @todo: Move this to the base class in https://www.drupal.org/node/2392845.
   +   */
   +  protected function ensureLoaded() {
   +    if (!isset($this->list[0]) && $this->definition->isComputed()) {
   +      $this->list[0] = $this->createItem(0);
   +    }
   +  }
   
   +++ b/core/modules/path/src/Plugin/Field/FieldType/PathItem.php
   @@ -30,12 +37,22 @@ public static function propertyDefinitions(FieldStorageDefinitionInterface $fiel
   +    $this->ensureLoaded();
   
   @@ -43,6 +60,22 @@ public static function schema(FieldStorageDefinitionInterface $field_definition)
   +    $this->ensureLoaded();
   ...
   +    $this->ensureLoaded();
   
   @@ -88,4 +121,23 @@ public static function mainPropertyName() {
   +  /**
   +   * Ensures the alias properties are loaded if available.
   +   */
   +  protected function ensureLoaded() {
   +    if (!$this->isLoaded) {
   +      $entity = $this->getEntity();
   +      if (!$entity->isNew()) {
   +        $alias = \Drupal::service('path.alias_storage')->load([
   +          'source' => '/' . $entity->toUrl()->getInternalPath(),
   +          'langcode' => $this->getLangcode(),
   +        ]);
   +        if ($alias) {
   +          $this->setValue($alias);
   +        }
   +      }
   +      $this->isLoaded = TRUE;
   +    }
   +  }
   

   We do this to match the behavior of other (non-computed) field types, correct? If so, can we add a comment to the ensureLoaded() method about that?

   And why does this todo only exist for PathFieldItemList::ensureLoaded() and not also for PathItem::ensureLoaded()?

2. +++ b/core/modules/path/src/Plugin/Field/FieldType/PathItem.php
   @@ -30,12 +37,22 @@ public static function propertyDefinitions(FieldStorageDefinitionInterface $fiel
   +    $properties['langcode'] = DataDefinition::create('string')
   +      ->setLabel(t('Language Code'));
   

   See \Drupal\Core\Entity\ContentEntityBase::baseFieldDefinitions(), the field type and label are different there. Is there a reason we should not match what's there?

   That has:

   $fields[$entity_type->getKey('langcode')] = BaseFieldDefinition::create('language')
           ->setLabel(new TranslatableMarkup('Language'));
   

3. +++ b/core/modules/path/src/Plugin/Field/FieldWidget/PathWidget.php
   @@ -50,22 +32,30 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
   +      '#default_value' => $items->alias,
   

   Interesting; I'd have expected that this needs to be read from the first item, not from $items?

4. +++ b/core/modules/path/src/Plugin/Field/FieldWidget/PathWidget.php
   @@ -50,22 +32,30 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
   +    $pid = $items->pid;
   

   Same observation here.

5. +++ b/core/modules/path/src/Plugin/Field/FieldWidget/PathWidget.php
   @@ -50,22 +32,30 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
   +      // The langcode for items may not be correct at this point if adding a new
   

   Why?

2. This is not a field type, it is a data type. A language field is a complex structure that has a langcode string + language reference see \Drupal\Core\Field\Plugin\Field\FieldType\LanguageItem::propertyDefinitions(). This is just a langcode. And this is actually basically consistent with the langcode defined in LanguageItem.

3/4, see \Drupal\Core\Field\FieldItemList::__get(), this is what makes $entity->field->property possible as well, even though field is actually always a list. $items->property is a shortcut for $items[0]->property/$items->first()->property/$items->get(0)->property.

5. Because it was cloned from the source translation. I'm not 100% sure about the language stuff yet, I certainly like this approach more than mine, but we rely on the widget and creating a translation in the API will not work and the related issues about fallback and supporting untranslatable aliases will also make it more complex again.

Note on 3/4: I'm perfectly happy to change that to the first item and working with that, as widgets usually do. I just wanted to explain why it works.

re #43

And why does this todo only exist for PathFieldItemList::ensureLoaded() and not also for PathItem::ensureLoaded()?

Add commnent above behaving like non-computed fields.

Do we want to move the PathItem to the base fieldItem class?

I added the todo but I am not sure if this will be the same for other computed fields. For instance in the __get() call to ensureLoaded() we don't need to pass the property name because we know that all the properties are loaded at once from the alias storage service, so it is not any more expensive to load all the properties at once. For other computed fields this might not be the case. But I guess we can determine if there a good generic way to handle this, such as always passing the property name, when we get to that issue.

What about this, a bit strange still but we don't rely on the widget for it to work correctly and includes API test coverage. And it should also pass PathAliasTest.

And this will make the two related issues for which I now also added explicit @todo's easier and might help explain why we do it like this.

For the record, for those of us who would like this functionality today: This patch applies to 8.2.6 and jsonapi started showing something else than null after applying it:

"path": {
"alias": "/articles/some-article",
"pid": "12350",
"langcode": "en"
},

Thanks guys.

#44

2. Yes, of course, data type, not field type. Sorry, and thanks for pointing that out.

   If it's consistent with the langcode defined in LanguageItem: is this important that it stays consistent? If so, we should add an @see to prevent regressions.

3. I know this. But what if you have multiple values, and they have different langcodes? Ah, I guess that is impossible: correct me if I'm wrong, but to access field values for a different translation, you need to retrieve the different translation of the entire entity. That's why this code is actually correct. Correct?

   But if widgets access the first item (as you say in #45), then probably best to stay in sync with that.

5. Hm okay… but that sounds like this needs additional test coverage then. Just having this comment kind of scares me.

#46: So we make path aliases a computed field item, and then we want to make them behave like a non-computed field item? This is confusing.

#47: I like that interdiff a lot!

1. +++ b/core/modules/path/src/Plugin/Field/FieldType/PathItem.php
   @@ -128,12 +143,14 @@ public static function mainPropertyName() {
   +        // @todo Support loading languge neutral aliases in
   

   So this is currently not supported at all?

   If its' not supported anywhere, then it's probably okay to have this TODO.

2. +++ b/core/modules/path/src/Plugin/Field/FieldType/PathItem.php
   @@ -141,6 +158,12 @@ protected function ensureLoaded() {
   +          // Ensure that at the langcode is set correctly.
   

   Incomplete sentence. Not sure what this means.

3. +++ b/core/modules/path/src/Plugin/Field/FieldType/PathItem.php
   --- a/core/modules/path/src/Plugin/Field/FieldWidget/PathWidget.php
   +++ b/core/modules/path/src/Plugin/Field/FieldWidget/PathWidget.php
   

   The changes here sure make it far more straightforward! :) Awesome :)

4. +++ b/core/modules/path/tests/src/Kernel/PathItemTest.php
   @@ -71,6 +76,28 @@ public function testPathItem() {
   +    // Assert the alias on the english node, the german translation and the
   

   s/english/English/
   s/german/German/

5. +++ b/core/modules/path/tests/src/Kernel/PathItemTest.php
   @@ -71,6 +76,28 @@ public function testPathItem() {
   +    $loaded_node = $node_storage->load($node->id());
   +    $this->assertEquals('/foo', $loaded_node->path->alias);
   +    $translation = $loaded_node->getTranslation('de');
   +    $this->assertEquals('/furchtbar', $translation->path->alias);
   +
   +    $stored_alias = $alias_storage->lookupPathAlias('/' . $node->toUrl()->getInternalPath(), $node->language()->getId());
   +    $this->assertEquals('/foo', $stored_alias);
   +    $stored_alias = $alias_storage->lookupPathAlias('/' . $node->toUrl()->getInternalPath(), $translation->language()->getId());
   +    $this->assertEquals('/furchtbar', $stored_alias);
   

   Woot! :)

About making it behave like a non-computed field, that might not be explained very well, but we compute/load-on-demand to make it work like any other field *from the outside*, yes.

1. No, it is not and never was, just like the other thing. It made sense for me to add those @todo's in case someone wonders about exactly that while reviewing this.

2. Actually it is complete, there are even more words there than necessary. at need to go. But I guess Ican expand what correctly actually means (.. set to the entity language.)

3. Yes, the test coverage you requested now actually exists in the kernel test.

About making it behave like a non-computed field, that might not be explained very well, but we compute/load-on-demand to make it work like any other field *from the outside*, yes.

Right, so documenting that better is probably enough :)

1. Yep, this is fine :)
2. Hah, well "clarify" then.

Getting close!

Wrote some REST tests, was a bit frustrating for two reasons:

* Impossible to debug REST requests, opened #2851746: Support xdebug header in ResourceTestBase and move htttpClient property to right place.
* Weird behavior of RessourceResponse that serializes *after* being cached, which is slow and results in weird behavior with the language on cached requests. Had to move setValue() to translation_create() hook, that seems to work.

More fun: The tests only work when your base url does *NOT* end with a trailing slash, which works fine for everything else.

This fixes some test fails, removed hardcoded assumption about urls *not* using an alias.

Fixed namespace of that class.

#52:

• Reviewed that issue.
• An issue that comes to mind is #2807501: ResourceResponse::$responseData is serialized, but is never used: unserializing it on every Page Cache hit is a huge performance penalty. Serializing does not happen in this response, but in a subscriber. And that subscriber must run early enough, or we risk caching the entire object that's being sent in the response. Although this sounds like it could be a slightly different problem.

  Happy to change REST if you can explain the problem in more detail. I'd rather not have the REST module's architecture force PathItem::setValue() to be removed in favor of path_entity_translation_create().

#54:

+++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
@@ -688,7 +688,8 @@ public function testPost() {
-    $this->assertSame([str_replace($this->entity->id(), static::$firstCreatedEntityId, $this->entity->toUrl('canonical')->setAbsolute(TRUE)->toString())], $response->getHeader('Location'));
+    $unserialized = $this->serializer->deserialize((string) $response->getBody(), get_class($this->entity), static::$format);
+    $this->assertSame([$unserialized->toUrl('canonical')->setAbsolute(TRUE)->toString()], $response->getHeader('Location'));

@@ -708,7 +709,8 @@ public function testPost() {
-    $this->assertSame([str_replace($this->entity->id(), static::$secondCreatedEntityId, $this->entity->toUrl('canonical')->setAbsolute(TRUE)->toString())], $response->getHeader('Location'));
+    $unserialized = $this->serializer->deserialize((string) $response->getBody(), get_class($this->entity), static::$format);
+    $this->assertSame([$unserialized->toUrl('canonical')->setAbsolute(TRUE)->toString()], $response->getHeader('Location'));

This code was just changed (simplified) yesterday in #2834848: REST test fails depending on local testing URL. Retesting #56 for that (which is otherwise green currently, yay!).

Rebased, yes, the new implementation should be fine.

Hm, lost my other comment?

* Yes #2807501: ResourceResponse::$responseData is serialized, but is never used: unserializing it on every Page Cache hit is a huge performance penalty looks related, but I was seeing different responses on a cache miss and cache hit, that shouldn't happen? As far as I see, the cached response object should only contain the normalized string but somehow that wasn't the case, at lest not for dynamic page cache. Is it possible that issue fixed it for internal page cache but not dynamic? I very much doubt we can or even want to support returning different responses or placeholders for DPC, so that seems weird?

path_entity_translation_create() is more correct anyway, so don't worry about that. I just didn't go with that initially as it is a bit more complicated as you have to loop over all fields. A corresponding method for fields would be nice.

but I was seeing different responses on a cache miss and cache hit, that shouldn't happen?

No, definitely not :(

Is it possible that issue fixed it for internal page cache but not dynamic? I

Yes, see #2827797: ResourceResponse(Subscriber) + Dynamic Page Cache: making authenticated ResourceResponses significantly faster.

path_entity_translation_create() is more correct anyway, so don't worry about that.

Okay, great :)

1. +++ b/core/modules/path/path.module
   @@ -76,3 +76,17 @@ function path_entity_base_field_info(EntityTypeInterface $entity_type) {
   +    if ($field_definition->getType() == 'path' && $translation->get($field_name)->pid) {
   

   ===

2. +++ b/core/modules/path/path.module
   @@ -76,3 +76,17 @@ function path_entity_base_field_info(EntityTypeInterface $entity_type) {
   +      // to save this as a new alisa.
   

   s/alisa/alias/

3. +++ b/core/modules/path/tests/src/Functional/NodeJsonPathTest.php
   @@ -0,0 +1,52 @@
   +class NodeJsonPathTest extends NodeJsonCookieTest {
   

   1. This is a great start, but we want it at least for every format.

      This should therefore be renamed to NodePathJsonCookieTest.

   2. Also, I really don't like that we now have NodeJsonCookieTest and NodePathJsonCookieTest.

      path_entity_base_field_info() does this for Term and Node. So we should also test it for taxonomy terms.

      So, let's create NodePathResourceTestBase and TermPathResourceTestBase, and then create the necessary subclasses for every format and every authentication method.

Once that's done, I think this is RTBC :)

I'm not sure about having at least 6 additional test (+2 new base classes) classes for this, I don't really see what we gain from that. json and hal maybe yes, but the point is that we now rely to 100% on entity-type/format/authentication agnostic code. If anything, then we could do a serialization (kernel) test to test the different formats.

Unfortunately, I've seen time and time again that there's unexpected per-format and even per-authentication mechanism problems. That's why I'd rather be prudent, and prefer to have more rather than less test coverage.

However, if you prefer, I'm also fine with adding the path stuff to the existing NodeResourceTestBase and TermResourceTestBase. Since aliases are probably used 99% of the time when using those entity types. Zero new test classes then.

1. I've actually seen more bugs *because* of type safe checks than not, especially around entity type and other database related things, but sure ;)
2. Fixed.
3. Fine, this adds it to the two base classes, works for me. Explicitly went with a different approach in regards to permission, so that we have permission checking properly covered there, works for you?

Ah, didn't update the hal+json tests.

#64: looks great :)

#67: yep, that's why I insisted on testing all formats. As you can see, the support for aliases does cause format-specific changes!

#69:

+++ b/core/modules/rest/tests/src/Functional/EntityResource/Node/NodeResourceTestBase.php
@@ -30,6 +30,7 @@
+    'path',

Hm… why exactly is this PATCH-protected? I don't see this field mentioned in \Drupal\node\NodeAccessControlHandler::checkFieldAccess() at all.

That's my only remaining concern.

#67: We could argue on whether that's a format-specific or simply a content change but lets not go down that path.. as long as we're both OK with the patch now ;)

#69 Yes, not in NodeAccessControlHandler, why would that know anything about a field defined by path module. You're looking for \Drupal\path\Plugin\Field\FieldType\PathFieldItemList::defaultAccess(). For terms, I did grant that permission, so have both having and not having access covered for that field.

New title, we're no longer trying.

Ah, I didn't find \Drupal\path\Plugin\Field\FieldType\PathFieldItemList::defaultAccess(), my bad!

For terms, I did grant that permission, so have both having and not having access covered for that field.

That makes sense! Let's document that in \Drupal\Tests\rest\Functional\EntityResource\Term\TermResourceTestBase::setUpAuthorization() + \Drupal\Tests\rest\Functional\EntityResource\Node\NodeResourceTestBase::setUpAuthorization with an @see to each other, so we don't change that in the future.

Once that tiny thing documentation addition is made, I will RTBC this :)

Like this? Always find writing comments to explain something that does *not* exist hard. Also only referenced the class as a whole, they're already > 80 characters and it's not just the method that is doing the path stuff.

Yep!

That's the best we can do here.

We closed #2649646: Normalize path fields as part of the entity: allow REST clients to know the path alias as a duplicate of this. It also had the Default content tag. So setting that on this one too.

looks like a change record would be good here. Something that says "In Drupal 8.4.x you can access an alias by... Prior to Drupal 8.4.x to access an alias you did this..." with code examples. Because as the issue summary notes there is are API changes here. Has anything extended PathItem in contrib?

> Has anything extended PathItem in contrib?

Yes, pathauto doesn't just extend it, it actually replaces the class for the path field type. And I hope nobody else does that, or pathauto won't work anymore. Pretty sure the changes are fine with pathauto but I'll run the tests to be sure.

What we will be able to do is simplify some code I think and move more logic into the class instead of the entity hooks where it currently is.

I'll try to create a change record soon, thanks for the feedback.

Change Record: https://www.drupal.org/node/2856220

There is no actual API or new API, the existing code just behaves differently, it basically just works as you'd expect.

Tested with Pathauto and I did find one bug that is fixed with this patch. Basically, we also need to load values when setting something new, as otherwise we replace those values again on save.

@catch wanted to take a close look at this, so assigning to him. Thanks!

OK so it's good that this loads values on demand, there are earlier issues (possibly duplicates) that wanted to load it all the time.

However the current approach here looks like it could conflict with #2336597: Convert path aliases to full featured entities, which is a pre-requisite for versioning of path aliases and integration with workspaces/previewing. If you load a revision and access the path alias, would you not expect the alias to match the path from when that revision was published etc?

Right now path aliases are completely orthogonal to the entity system and have no real connection to any specific entity, we just hack some form fields into entity forms to make adding them more convenient. This issue further integrates aliases with entities but it's making the inadequacy/disconnect of the current system more obvious by doing so.

So does #84 mean this is simply hard-postponed on, or even a duplicate of #2336597: Convert path aliases to full featured entities?

#2336597 last received a comment on October 21, 2016. Before that, its last comment was on June 23, 2015. Is that actually going to happen? Will it be solved as part of the Workflow Initiative?

Yes, as far as I see, that other issue is pretty far from being anywhere near completey, hasn't been updated in years and so on.

And unless we'd change path fields to be an entity reference field, which would IMHO be a pretty big and breaking API change, we still need a custom field type that keeps the structure and then it doesn't really matter if we support reading or not, it will need to (internally at least) change a lot anyway.

Also, if you look at menu links, even though those are entities, the integration is actually even more custom and even weirder than this even though they are entities.

Pathauto currently "solves" the workflow problem by only generating an updated alias when the default revision is stored and ignores anything else.

And even if we would support storing aliases for non-default revisions, I have no idea how that could even work (would we then point that alias to the custom revision URL? If not, then you'd save a new alias but it wouldn't be visible? Even with workspaces, how would we know which path revision to query for? .. so much fun)

IMHO, I'd vote to not block/wait on that.

Right now with content_moderation the following happens:

1. Create article with alias /hello
2. Create a new draft of that article with filling in /hello2 as the url alias
3. The article is immediately accessible at /hello2 and not at /hello

So for content_moderation to be stable, that bug needs to be fixed one way or the other.

For alias revisions I'd expect us to add a status field to aliases, then they can be published/unpublished along with revisions - but yes it's very far from what we do now which is why I'm asking questions here.

Right, but that bug has IMHO nothing to do with this issue, it neither makes it more complicated nor easier to fix that. We'd have to support loading the correct alias if we'd support something like that, but that's a pretty small part of the whole picture.

If you have a status then you still don't know to which revision a certain alias belongs?

I could even imagine that to support that we'd switch to duplicating the alias information in a normal field with revisions and sync with the url_alias table if it is the default revision. Then this logic wouldn't be needed anymore, but it would continue to behave the same to the outside. But that would likely be a problem for Pathauto, which needs this to be a computed field but all possible solutions are likely going to cause some trouble with Pathauto.

So I think that puts us back in an RTBC state?

This needed a reroll. Here is the new patch.

Sorry, uploaded the partial patch. Here is the complete patch

The rebase in #92 is correct.

Something is causing the patch to no longer work, to cause 'path' => [] to be the normalization… :( Trying to figure out why.

Stepped through it with a debugger.

\Drupal\serialization\Normalizer\ListNormalizer is given the PathFieldItemList object to normalize. This is its code:

  public function normalize($object, $format = NULL, array $context = array()) {
    $attributes = array();
    foreach ($object as $fieldItem) {
      $attributes[] = $this->serializer->normalize($fieldItem, $format, $context);
    }
    return $attributes;
  }

we never make it to the inner array: there are no $fieldItems to iterate over.

If I break before that loop and call $object->getValue()… then it does find $fieldItems. In other words: as of right now, \Drupal\path\Plugin\Field\FieldType\PathFieldItemList::ensureLoaded() is never called.

I have no idea at all why. Nothing changed in the serialization module that would cause this. Did a recent Entity API change cause this?

This fixes those tests for me. Caused by that rest issue that makes the output respect the type.

I haven't had a chance to figure out why but reverting #2751325: All serialized values are strings, should be integers/booleans when appropriate makes this patch pass at least NodeHalJsonAnonTest

oh I didn't see @Berdir's fix. so yeah looks like related to cast issue

Rerolled.

Fixed that.

#97: :O :O :O But … but … but … then how the hell was I getting 'path' => [] locally? WTF.

If I look at #92 I don't see that 'path' => [] problem either. So it must've been something on my local setup. Weird. I agree that the fixes in #97 make sense (and in #101, to adjust for the PHPCS change that was deployed over the weekend).

Back to RTBC per #89.

testEditorXssFilterOverride
fail: [Completion check] Line 418 of core/modules/editor/src/Tests/EditorSecurityTest.php:
The test did not complete due to a fatal error.

That doesn't seem related, retesting.

Random infra fail. Note that https://www.drupal.org/pift-ci-job/634987 does not even list any failing test! :/

Love this patch! It saved me *hours* of work! Big thanks to everybody!

The PathItem field is currently the only computed base field in core, and I was using it as inspiration for my own computed field. I was just about to embark on a long journey of discovery why my field wasn't working when I stumbled on this. Having this in will have great educational value!

For now I will update the documentation page Dynamic/Virtual field values using computed field property classes with this knowledge since I saw that several people have commented there wondering the same thing. I will also refer to #2392845: Add a trait to standardize handling of computed item lists which is going to fix this for all computed fields.

This issue just came up as something that could be useful for #2856363: Path alias changes for draft revisions immediately leak into live site.

For some reason the rtbc re-test was not testing this. Looking at the PHP7 test results on #101 this issue needs work.

We're using this patch in a project (8.3), and everything has seemed to work nicely.

But when we added two new REST endpoints, one that can create new nodes and another that edits existing nodes, we see that the path alias is empty (after $node->save();) for new nodes, and we get the old path for updated nodes.

for “old” nodes, getValue() on the node’s PathFieldItemList works fine.

For now I have resorted to $alias = \Drupal::service('path.alias_manager')->getAliasByPath('/node/' . $node->id()); and I wonder if the issue we have getting old path data could caused be an issue with the patch?

Slightly annoying that hal and serialization use quite different implementations to normalize fields, also based on my debugging, looks like the generic complex data and list normalizers were used and not the field/field item specific ones. So there might be some bugs hiding there but not really our problem.

We can fix this by also ensuring we load the data in getIterator().

I guess that also explains #102, Wim, you probably had that patch applied already that was changing json seriaization...

Slightly annoying that hal and serialization use quite different implementations to normalize fields, also based on my debugging, looks like the generic complex data and list normalizers were used and not the field/field item specific ones. So there might be some bugs hiding there but not really our problem.

Yep to all of that. Related: #2834734: Add a FieldableEntityNormalizer (rather than having support for fields in EntityNormalizer, which we can't change anymore due to BC).

It looks like we need to address #112. @badrange the comment sounds a bit vague - perhaps you can supply a little bit more detail?

Also added credit for @Wim Leers, @damiankloip, @catch and @badrange for code reviews and/or testing of the patch on live sites.

Yes, would be great if you could provide more info, @badrange. And thanks for testing it on an actual production, that's always most appreciated :)

Sorry for being a bit vague, I'm not sure if I understand the situation completely myself. I'll try again:

For read only endpoints, everything works fine. But when we created an endpoint that allows nodes to be changed, which return the same data (basically a crazy recursive function that extracts all field data from the entities in the system), I got complaints from the front end guys that the endpoint returned old urls.

The url is updated by pathauto upon node->save() as it should, but our crazy recursive function; which depends on using getValue(), gets old data after node->save. Here are some extracts of some of the lines of codes in question:

if ($field instanceof PathFieldItemList) {
  $field = $entity->get($field_name);
  $value = $this->normalizeFieldItemListValue($field->getValue());
}

Right now we are in a crazy hurry to catch a deadline, so I can't start writing tests myself, but I think that a test should do something like this:

1. For an existing node, get the path (using the fixes from this patch, naturally)
2. Change the node path, with a node->save()
3. Get the path again, and fail the test if the path you see is the old outdated path

I would be super happy if this patch gets committed, one more D8 GOTCHA down before I find the time to blog about them :D

Changing status back.

For step 3, did you load the node? If so, did you use ::load() or ::loadUnchanged()? i.e. \Drupal\Core\Entity\EntityStorageInterface::loadUnchanged(). You need the latter, otherwise it'll use the node that's statically cached for that request.

Thank you Wim Leers - using ::loadUnchanged() indeed returns the updated path.

Today I learned something new from you. At the moment I'm wondering whether I should be embarrassed not to know about this.

Long story short: All is good with this patch, on my behalf.

#121: no need to be embarrassed — Drupal does so much object caching that it requires you to know about these things, which is unfortunate. The problem is this rough edge in Drupal, not you. :)

#122: yay!

I think the behavior described in #118 is a bug because we shouldn't need to use loadUnchanged() after an entity save process, so we need to ensure that we clear the "static cache" after saving.

Note: I tested both with node load and loadUnchanged and they both gave updated URLs. I don't know why it didn't happen when I tried previously, I might have messed up something else.

Will try amateescu's patch later - don't have time now.

so we need to ensure that we clear the "static cache" after saving.

I was wondering why this was not the case already. But I've run into weirdness with this myself many times in tests, so I assumed it was something like that. My bad!

The test results for #124 weirdly show no failures, but testbot's actual output shows this:

00:29:17.695 Drupal\system\Tests\System\HtaccessTest                      113 passes   2 fails                            

That seems like a random fail. Retesting.

I've tested amateescu's patch in #124, and the results are the same both with #118 and #124: Both $this->entityStorage->load() and $this->entityStorage->loadUnchanged() returns the updated path alias.

Note: #124 doesn't apply to 8.3.2 in core/modules/hal/tests/src/Functional/EntityResource/Node/NodeHalJsonAnonTest.php and core/modules/rest/tests/src/Functional/EntityResource/Node/NodeResourceTestBase.php.

Ok then, if #118 is a non-issue after all, let's go back to the patch in #113. Sorry for the noise :/

#130: no worries. Although I wonder that if even you can be confused by this, whether we want explicit test coverage for the "read, update, read again in same request" case, which may behave correctly or not depending on whether static caches are being invalidated as they should or not.

@Wim Leers, before posting #130 I ran the tests added by this patch after commenting out all the resetCache() calls and everything passed as expected, so we have at least one "manual" confirmation that "read, update, read again in same request" is not a problem here.

Let's see if committers would like to see such an explicit test as well.

#131/#132 given issues like #2802403: Combination of language negotiation and path aliasing can cause a corrupted route cache, 404s I think it'd be worth having the explicit test coverage. My first thought reading the tests was "why are we even resetting the caches" but of course the point is to test different method calls with an empty cache. We could possibly just add more assertions to the same test (and a couple of lines of comment to explain the difference wouldn't hurt).

I still have a general uneasy feeling that we're improving the integration of something that's conceptually wrong, but the patch itself is fine as far as the change goes. I did find a couple of nitpicks reviewing again:

+++ b/core/modules/path/path.module
@@ -76,3 +76,17 @@ function path_entity_base_field_info(EntityTypeInterface $entity_type) {
+      // If there are values and a PID, update the langcode and unset the pid

Mixed upper and lower case in one comment for PID/pid.

1. +++ b/core/modules/path/tests/src/Kernel/PathItemTest.php
   @@ -0,0 +1,154 @@
   +
   +    // Assert the alias on the English node, the German translation and the
   +    // stored aliases.
   

   Could use an oxford comma.

2. +++ b/core/modules/path/tests/src/Kernel/PathItemTest.php
   @@ -0,0 +1,154 @@
   +
   +    // Reload the node to make sure that it is possibly to set a value
   +    // immediately after loading.
   

   s/possibly/possible

Fixed the points from #133 and added some assertions for #131 / #132.

+++ b/core/modules/path/tests/src/Kernel/PathItemTest.php
@@ -11,6 +11,7 @@
+ * @group failing

We don't have this anywhere else in core, so I think we want to not introduce that here. I think it's a local development leftover :) Can be fixed on commit though.

Oops! Yep, that's how I run phpunit tests locally these days :/

Committed a2cc58d and pushed to 8.4.x. Thanks!

I kind of needed this on a D8.3 project, attaching a re-rolled patch which seems to be working. Could be useful to others.

Follow-up: #2905524: The count() method used on PathFieldItemList behaves differently if isEmpty() has been called..

Follow-up: #2908674: When using get() method directly on PathItem the alias is not loaded.

Follow-up: #2909183: Add path alias (PathItem) field PATCH test coverage.