PathItem::delete() never runs because the path field type is a computed field in disguise

I looked into this a bit and I think that PathItem is completely broken :/ There are multiple problems with it:

• its values can not be accessed through the API (e.g. $node->path->alias)
• it enforces the "has custom storage" status by returning an empty array in the schema() method
• by not being honest about it being a computed field, the delete() method is never called because it sits on the wrong level (i.e. it should be on the field item list class, not the field item class)

Here's some tests to prove all of the above.

Also, we really need to finish #2392845: Add a trait to standardize handling of computed item lists so we can use the computed field base class added there :)

Path items already have a field item list class, so just moving the delete() method in there is basically just kicking the can down the road, imo :/

If you remove that debug line and open a new issue for fixing the rest of the path field brokenness, I'm fine with going forward with #8 in this issue :)

Have we considered what happens with translations now that this actually works? :)

There is path_entity_translation_delete(), which has a bug when called on an entity type that has no canonical link template: #2539222: Exception when deleting a translation when there is no canonical link template.

If this is now called as expected for removed translations then we might be able to drop that hook and close the related issue as a duplicate?

But we do need to make sure to only delete the alias for the current translation, no idea if we have test coverage for that.

As mentioned in IRC, yes I think we should do exactly that. And drop the mentioned translation hook and see if tests are still passing :)

Here is something I posted on a duplicate issue, #2638974: [regression] Deleting node doesn't delete path alias, with a different test approach. It was suggested that there be an upgrade to remove unwanted url aliases. This would need integration with prior patches.

Hm, upgrade path seems like a tricky problem, not sure if we can reliable delete those aliases and if we should? Maybe just fix this asap so we don't have too many of those stale aliases?

Also, I guess this doesn't happen if you're using pathauto, due to pathauto_entity_delete().

Note that pathauto also actually makes this field computed. Maybe we should just be honest and do so in core too, because it really is. @yched didn't like it but it used to be and I think it still is. @yched's opinion is AFAIK mostly based on the fact that computed fields are not clearly defined, but the functionality that it does offer is what we need here.

Would be nice to get it fixed with something like #16 but if that gets too complicated, we can move forward with #18 for now, although I think I prefer extending the existing test coverage instead of adding a new test class.

Do not bother with the update hook. There be dragons. There is absolutely no way you can do this correctly. What if node/12345 is a view page for some demented reason?

... I think I prefer extending the existing test coverage instead of adding a new test class.

Here you go.

Is this why when a Node is loaded and normalized through a serializer the path alias value isn't in the resulting array? It seems like the PathItem field is implemented differently to normal fields.

Yeah, also if you tried to get $node->path from code, or using drush php REPL command, you will always get NULL...

I can confirm the patch in #24 works for me.

I agree with #21: the proper fix seems to be marking path fields as computed. However, since we have a working patch here and that's possibly a controversial solution, I'd suggest to go on with #24 and open a new issue to discuss the "proper fix". We can revert this change there if needed.

This issue was discussed with the core committers and Entity / Field maintainers and we agreed that it is major and that it would be better to go with the proper fix here: make the path item a computed field. This will also allow Pathauto to not change the storage of path fields on its own, and therefore fix a very annoying user-facing issue for them: #2673628: Ignore field definition removals/additions from fields with custom storage.

Ok, the attached patch does that. Also added an update function.

The new test seems to be passing. The test-only patch is unchanged, so no new patch for that.

Ah, the new method is now also called on translation deletions and will should have test fails.

This removes path_entity_translation_delete() and makes delete() language specific. This might require some more cases, for example, when deleting the last translation (and deleting the entity completely), we might want to delete UND aliases too. Not yet sure how to figure that out exactly. Doubt we have tests for that, see also #2484411: Manual path aliases are not the same as aliases on the node form and #2511968: Path field should fall back to language neutral aliases (also makes this happen for the form widget!).

Discussed with @amateescu about #2 (loading the existing aliases when accessing it). But looking into it a bit, that's actually not so trivial:

a) computed fields don't really have a proper API to "compute", only properties do. So to do this, we need to either add custom classes or hack around that somehow.
b) Adding computed properties also has some minor performance implications as we currently have to create those classes when instantiating a field item object.
c) It has implications on the behavior of postSave(). Specifically, we do not want to run in a case the value wasn't changed which would happen if $node->path->alias would autoload with the current code. So we might need to track changes and keep an original value.

Some or all of those things might mean that this is not applicable for 8.1.x. So I think it might make sense to move that part to a possibly 8.2.x only follow-up issue, despite what I said to @amateescu a few minutes ago in IRC :)

The path_entity_translation_delete() removal is probably also a problem for 8.1.x, doing it for now to see if it's really working. But we might need to leave it for 8.1.x, according to the discussion in #2348203: hook_node_access() no longer fires for the 'create' operation.

+++ b/core/modules/path/path.install
@@ -0,0 +1,46 @@
+  foreach (['node'. 'taxonomy_term'] as $entity_type_id) {

This could be why it's failing: Should be a comma where you have a period.

Thanks! Could really use this patch.

So having it computed means it vanished from the language settings page. Test still passes, so it doesn't seem to be break it when just removing that?

If the path alias vanishes from the content translation settings page does it means that path alias will always be translatable if the entity is? That we can't choose not to translate it?

In this other issue, #2689459: If you don't want to translate your URL alias, the original URL alias won't work with your translations, we are trying to solve a problem when you don't want to translate the path alias and use the same one for all translations. To do that, it was discussed to used 'und' langcode for untranslated path aliases.

IMHO, I think we should keep the option of not translating it, but I'm not familiar with how computed fields work, so maybe there's a way to do it even so.

Computed fields have no storage definition, at least not one that is accessible through getFieldStorageDefinitions().

So yes, apparantely, that results in content_translation no longer offering to make it translatable, as it only shows fields with a storage definition.

This is an alternative fix for that test that keeps the ability to configure it being translatable by allowing all computed fields to be translatable. not sure if that's correct.

Lol, I'm pretty sure I fought with @fago at some point about path being computed or not. It really is unfortunate that we do not have a more well-defined API around computed fields...

Let's prevent the error in the case, on an existing site, somebody did this manually or by applying #2673628: Ignore field definition removals/additions from fields with custom storage. I have such a case on a production site when I used first #2673628: Ignore field definition removals/additions from fields with custom storage. Now, if I'm applying #44, I'm getting

$ drush updb
The following updates are pending:

path module :
  8001 -   Change the path field to computed for node and taxonomy_term.

Do you wish to run all pending updates? (y/n): y
Argument 1 passed to                                                                               [error]
Drupal\Core\Entity\EntityDefinitionUpdateManager::uninstallFieldStorageDefinition() must implement
interface Drupal\Core\Field\FieldStorageDefinitionInterface, null given, called in
core/modules/path/path.install on line 26 and defined
EntityDefinitionUpdateManager.php:192
Argument 1 passed to Drupal\Core\Entity\EntityManager::onFieldStorageDefinitionDelete() must       [error]
implement interface Drupal\Core\Field\FieldStorageDefinitionInterface, null given, called in
core/lib/Drupal/Core/Entity/EntityDefinitionUpdateManager.php
on line 194 and defined EntityManager.php:426
Argument 1 passed to                                                                               [error]
Drupal\Core\Field\FieldStorageDefinitionListener::onFieldStorageDefinitionDelete() must implement
interface Drupal\Core\Field\FieldStorageDefinitionInterface, null given, called in
core/lib/Drupal/Core/Entity/EntityManager.php on line 427
and defined FieldStorageDefinitionListener.php:105
PHP Fatal error:  Call to a member function getTargetEntityTypeId() on null in core/lib/Drupal/Core/Field/FieldStorageDefinitionListener.php on line 106

Fatal error: Call to a member function getTargetEntityTypeId() on null in core/lib/Drupal/Core/Field/FieldStorageDefinitionListener.php on line 106
Drush command terminated abnormally due to an unrecoverable error.                                 [error]
Error: Call to a member function getTargetEntityTypeId() on null in core/lib/Drupal/Core/Field/FieldStorageDefinitionListener.php,
line 106
The external command could not be executed due to an application error.                            [error]

I think this is ready to go. It's a bit sad that we couldn't manage to also fix the second part of the problem, accessing the computed value, but fixing the bug is more important :)

Do we need an upgrade path test here?

And test what exactly? Without the update function, there are dozens of test fails about entity updates. This fixes them. The change doesn't actually change anything, as commented. So there is nothing functional that we could test, we only do this to get rid of the bogus entity updates status error.

I don't fancy this for 8.1.x.

I'd consider committing to 8.2.x, but we need to change the update to be 8200 not 8001 as well as the group (8001 is also wrong for 8.1.x, should be 8100).

Also this needs a change record to tell contrib what to do if they're using the path field type on other entity types, upgrade path only deals with node and taxonomy term.

Also @alexpott pointed out if we do #2673628: Ignore field definition removals/additions from fields with custom storage we don't need an upgrade path here. While that's true, having a redundant update seems fine.

Updated the update function, change record is next.

Here we go: https://www.drupal.org/node/2783903

Still looking good :)

Don't we need an upgrade path for the path aliases for deleted nodes and terms?

See https://www.drupal.org/node/2539634#comment-10716352

We can't say with 100% certainty that node/500 is not valid if there is no node with ID 500. As chx said, there be dragons.

+++ b/core/modules/content_translation/content_translation.admin.inc
@@ -113,7 +113,7 @@ function _content_translation_form_language_content_settings_form_alter(array &$
-          if (!empty($storage_definitions[$field_name]) && _content_translation_is_field_translatability_configurable($entity_type, $storage_definitions[$field_name])) {
+          if ($definition->isComputed() || (!empty($storage_definitions[$field_name]) && _content_translation_is_field_translatability_configurable($entity_type, $storage_definitions[$field_name]))) {

This change looks like we have an API break. Is this because we remove the setCustomStorage(TRUE)? Hmm interesting. This looks like quite a tricky change. Reading the comments on the issue I'm not sure there's been enough discussion about the potential set effects of this change

Yes agreed on the upgrade path, there's no hard association between an entity path and an alias, and people have all kind of legacy aliases on real sites. i.e. if you had an old landing page that was a node that you replaced with a panel, then instead of a redirect added an alias for node/n pointing to the panel.

@alexpott what exactly do you think the API change is?

@catch - suddenly computed fields are going to show up in a list they didn't and they have to deal with translation ... how do we know they can do that?

See #42-#44. Not showing them will make it impossible to make that other issue work, which I think would be unfortunate, as non-language specific aliases is a common feature request in pathauto as well.

But you are right, this change means that other computed fields will now show up in that form. Question is if the current behavior is a bug or not. I'm really not sure how a computed field behaves right now in regards to translations, the test apparently still worked, which I think might imply that they are translatable by default? Because then it is not about allowing to enable but allowing to disable translations.

Because then it is not about allowing to enable but allowing to disable translations.

Yep I'm not sure what it means to disable translations on a computed field. This is really tricky.

Does anyone else feel dirty about changing the path field from hasCustomStorage to isComputed? Because if you look at it: While one might imply the other, there is a clear difference.

Custom storage: Instead of storing the data for this field in the field storage, it is stored elsewhere. The only "computed" thing about this field is that it retrieves and stores its data manually from/in another storage instead of a standard field storage.

Computed: All bets are off. This field doesn't care about storage or anything. If it wants to load its values from an outside storage, it can. But the main use for this type of field is to compute items on the fly based on other fields that are available on the same host/parent/entity.

So if you look at it, the path field isn't computing anything. It's just using a custom storage without taking any other data into account. This seems like a hasCustomStorage situation without a doubt.

Compare this to a backreference field, for instance, and we have a whole different story. That field would require the host entity's ID field and entity type ID to be able to find out which entities are referring to it. So that would definitely be computed. The fact that it would call other storages does not make it hasCustomStorage because the field itself isn't storing anything. As opposed to the path field, which does store data.

I'm voicing this concern because the proposed fix looks like a hard thing to undo should we regret the change somewhere down the line. Especially because all computed fields will now show up in the translation UI as mentioned in #57

The fact that Pathauto turns it into a computed field also seems incorrect. While semantically it looks more like a computed field because Pathauto allows it to draw data from other fields on the host entity, it's still just a hasCustomStorage field.

At the end of the day it's a text field that stores a string in a storage and retrieves said string from that storage when rendered as a text input on a form. The relation between the path field and its storage is almost 1:1, regardless if where it gets its value from.

A computed field could come from a storage, but it may very well not need a storage at all. If I were to create a "random number" field with just a display (no form), that would be a true computed field which doesn't even need to call a single storage.

You're not wrong. Core is currently very unclear around computed/custom storage and how that is supposed to work.

hasCustomStorage() alone doesn't do anything other than not creating the storage. There is on official way to actually load your value and it works otherwise just like a normal field. You would need a storage_load() hook to populate it, kind of like comment statistics does it.

The main difference with computed is that their field item methods are also called when there is no value, as we don't know if there is one or not. I think you can argue that loading from custom storage is also kind of computing. We also need to know the entity URL to be able to load the path, so that's not really different to backreference, isn't it?

So it looks like the case I'm making is that this issue should not block #2673628: Ignore field definition removals/additions from fields with custom storage. That issue actually fixes the concerns I'm raising above.

It should also fix #2661178: Error: Mismatched entity and/or field definitions in the Pathauto issue queue because changing a field from hasCustomStorage to isComputed will no longer trigger a definition mismatch. Even though I'm not convinced Pathauto should turn the path field into a computed field to begin with.

This issue could instead try to focus on getting PathItem::delete() to run, as the title mentions, without blaming it on the fact that the path field is recognized as a computed field in disguise.

Core is currently very unclear around computed/custom storage and how that is supposed to work.

It looks like that is the very reason we are seeing quite a few issues like this one pop up.

I think you can argue that loading from custom storage is also kind of computing. We also need to know the entity URL to be able to load the path, so that's not really different to backreference, isn't it?

Very true. Although we would still be storing the end result whereas a true computed field may not.

#62++, great analysis!
#64++, great nuancing.

We have imperfect APIs. This is a fact, and applies to every piece of software.

I think the sole remaining question that must block a commit is: does this have additional (long-term) consequences, that we have not yet discussed here, and which core's test suite would not catch?

I've noticed at least one gap in test coverage:

1. +++ b/core/modules/path/src/Plugin/Field/FieldType/PathFieldItemList.php
   @@ -21,4 +21,17 @@ public function defaultAccess($operation = 'view', AccountInterface $account = N
   +    // Delete all aliases associated with this entity in the current language.
   +    $entity = $this->getEntity();
   +    $conditions = [
   +      'source' => '/' . $entity->toUrl()->getInternalPath(),
   +      'langcode' => $entity->language()->getId(),
   +    ];
   

   Good.

2. +++ b/core/modules/path/src/Tests/PathAliasTest.php
   @@ -314,6 +314,18 @@ function testNodeAlias() {
   +
   +    // Create fifth test node.
   +    $node5 = $this->drupalCreateNode();
   +
   +    // Set a path alias.
   +    $edit = array('path[0][alias]' => '/' . $this->randomMachineName(8));
   +    $this->drupalPostForm('node/' . $node5->id() . '/edit', $edit, t('Save'));
   +
   +    // Delete the node and check that the path alias is also deleted.
   +    $node5->delete();
   +    $path_alias = \Drupal::service('path.alias_storage')->lookupPathAlias('/node/' . $node5->id(), $node5->language()->getId());
   +    $this->assertFalse($path_alias, 'Alias was successfully deleted when the referenced node was deleted.');
   

   This is not yet testing the fact that we're only deleting only path aliases associated with the deleted translation.

We have existing test coverage for deleting aliases of translations, since that actually works, due to the existence of the hook in content_translation that we are removing here. But looking at that, it doesn't look like we have existing coverage to only delete the correct translation. Added that now.

About your question, I'm not aware of anything breaking with this patch. What I also don't know is if and how this can still get into 8.2.x. As a major bug, it would be really unfortunate if we have to wait another 6 month, but the removal of the hook for example could be tricky (we can keep an empty function). Same for some behavior changes around translating computed fields.

That looks great to me. And that is an excellent answer, thank you.

Note that to increase the chances of a core committer considering this acceptable for 8.2.x, you can improve the issue summary to better explain the consequences of this not being fixed.

Seeing as we only wrote an update hook for the node and taxonomy_term entity type, I'm guessing we need a change record informing module maintainers of the change to the path field and how they can update their instances of it.

updated issue summary.

We already have a change record, I also added the update function as an example to it now.

Ugh, sorry for the confusion. Didn't notice we already had a CR. Thanks for adding the update function to it, though!

The changes here look OK for a patch release to me - it's just overriding a previously-not-overwritten method and removing a hook implementation.

Not sure yet whether it's better for this to land during rc or shortly after 8.2.0 is tagged (i.e. for 8.2.1) though.

Since this changes some internal overrides and has an update function, and is a major bugfix, @catch and I agreed to make this an rc target. Since we are not creating any additional 8.1.x bugfix releases, we don't need to worry about backporting the update hook, so now is a great time to get this fix in.

Committed/pushed to 8.3.x and cherry-picked to 8.2.x. Thanks!

Yay, thanks!

Published the change record.

Opened #3549608: Language-neutral path aliases are not deleted with the parent entity to fix a regression introduced by this issue.