Path alias changes for draft revisions immediately leak into live site

Pretty sure menu link will be same problem

I discussed this issue with @xjm, @cottser, @catch, @alexpott and @lauriii. We agreed this should be critical for the reasons @catch gave in #2.

I think it would be great if #2336597: Convert path aliases to full featured entities solved this so you could create multiple revisions of a path alias. Realistically though, that seems like it's going in the direction of a whole other experimental module, which will need to be written and will have its own timeline to get to stable status. Given this is a critical data integrity issue, it risks the stable status of content_moderation.

Would everyone be comfortable with adding the validation to prevent alias changes on forward revisions as a means to downgrade this to a Major perhaps, with the view to fix it once the linked issue is resolved?

Another off the cuff suggestion would be to create aliases for the latest-version route for all forward revisions where paths change, eg "/node/1/latest" => "/new-path" then switch them over once the latest revision is published, but I think implementing and maintaining something like that would look a lot worse than some simple validation, especially since it's a stop-gap measure that we want to solve properly.

I was thinking about this problem yesterday and actually started to write a path_alias module for #2336597: Convert path aliases to full featured entities, but then I realized that node/1/revision/2/view is a perfectly valid system path, so we could solve this with much less effort than rewriting the path alias system by using the 'revision' entity url for draft revisions.

It would look something like the patch attached. The final patch is going to take a bit more effort to complete so I'd like to get some opinions first if this is an approach worth pursuing :)

Sounds like a good option. One issue might be that the revision path doesn't exist for all entity types, hence the suggestion of using the "latest-version" route, which content_moderation adds.

Other than that, if this is easy to do, all we'd need to include in addition to roughly the approach in #7 is switch the revision alias over to the canonical one once that revision is published.

Sounds doable. +1 from me.

This obviously depends on the entity having a "revision" URL. In core that's only Node (because only Node and BlockContent are revisionable, and BlockContent isn't rendered as a full page anyway), but in contrib or custom? So I guess the final patch will need a check that the URL exists, but what do we do if it doesn't?

Other than that, it sounds like a good plan.

This only works if the path alias is changing when creating the future draft revision - otherwise the old alias is updated to point at a revision that's not the default revision.

@alexpott, yes, I said that the patch is incomplete, it was just a PoC to show everyone what the idea is :)

@Sam152:

One issue might be that the revision path doesn't exist for all entity types, hence the suggestion of using the "latest-version" route, which content_moderation adds.

The bug that needs to be fixed here can be reproduced without Content Moderation, it's a general problem with core's forward revision support so we can not rely on a route added by CM.

@timmillwood:

So I guess the final patch will need a check that the URL exists, but what do we do if it doesn't?

We most likely need to throw a "incomplete entity definition" exception if the entity type is revisionable, has a canonical "view" route but lacks a "view revision" one.

Another point: This will only work also if changing which revision is the current/published revision triggers the revision and its fields to be saved (and hence the postSave() method to be called) on both the revision that used to be current and the one that is now becoming current. Does that happen?

Patch in #7 still applies. Re-uploading it here so CI can happen.

Needs a test, since there are instructions for reproducing the error in the IS.

@Mile23, I specifically used the 'do no test' option for the patch in #7 so we don't waste CI resources on a patch that was simply meant as a PoC and a discussion starting point.

I like the plan thus far. My feeling is that it's worth pursuing a more fleshed out implementation + tests.

We most likely need to throw a "incomplete entity definition" exception if the entity type is revisionable, has a canonical "view" route but lacks a "view revision" one.

I think we'd need to silently fall-back to the leaky behaviour for BC reasons, but the rest of the plan seems like the best that has been suggested yet.

I've been testing the patch from #7 this morning and there are two big issues.

1. $entity->isDefaultRevision() returns TRUE when it should be FALSE.
2. $entity->toUrl('revision')->getInternalPath() is returning the canonical path not the revision path.

I think this is due to \Drupal\path\Plugin\Field\FieldType\PathItem::postSave being called before the entity has been saved.

Maybe this code needs to move to hook_entity_create and hook_entity_update?

it seems like hook_entity_create and hook_entity_update are too early too, and hook_entity_insert is too late because by then $entity->path->alias is empty.

hook_entity_insert is too late because by then $entity->path->alias is empty.

We have to work with the assumption that #2846554: Make the PathItem field type actually computed and auto-load stored aliases will be committed sometime in the near future, at which point $entity->path->alias will have a value.

I think this is due to \Drupal\path\Plugin\Field\FieldType\PathItem::postSave being called before the entity has been saved.

I'm pretty sure that can not happen :)

Looks like a contrib module I had installed was causing the issues I mentioned in #17 and #18.

So here's an updated patch with a test.

Not sure I'm 100% happy with the logic, and think this is the best solution when no alias is added, but will think on it.

I was talking to @timmillwood about this issue and a flaw in my proposal came up: there is nothing that could link multiple sources from the {url_alias} table to a single entity.

One possible solution for that would be to add another column to the table, entity_id and maybe even revision_id. How do others feel about that?

This new patch fixes the failing tests in #20.

I also think this addresses the critical issue, but it doesn't fix the general issue with URL aliases.

Currently the path field is only added by the Path module to Node and Taxonomy term entity types, as this issue only relates to forward revisions and Taxonomy terms are not revisionable, this issue only affects nodes.

If someone creates a custom entity which manually implements the path field then they would benefit from this patch (as long as they have a revision path). However if they create url aliases without using the path module then they'll still get this issue.

@timmillwood, are you sure the changes to PathItem::postSave() from #20 and #23 are actually needed? Maybe the new test coverage exposed some problems with the code from #7?

1. +++ b/core/modules/path/src/Plugin/Field/FieldWidget/PathWidget.php
   @@ -28,7 +28,12 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
   -      $conditions = ['source' => '/' . $entity->urlInfo()->getInternalPath()];
   +      if ($entity->getEntityType()->isRevisionable() && !$entity->isDefaultRevision()) {
   +        $conditions = ['source' => '/' . $entity->toUrl('revision')->getInternalPath()];
   +      }
   +      else {
   +        $conditions = ['source' => '/' . $entity->toUrl()->getInternalPath()];
   +      }
   

   This implies that *every* non-default revision should have an alias, but that's not the case.

   We need an OR condition which falls back to the $entity->toUrl()->getInternalPath() source here :)

2. +++ b/core/modules/path/tests/src/Functional/PathContentModerationTest.php
   @@ -0,0 +1,105 @@
   +class PathContentModerationTest extends BrowserTestBase {
   

   The test should have code comments to explain exactly what it's testing.

   And we should also test deleting revisions and whole entities as well.

@amateescu - As we discussed the changed between #7 and #23 were needed to the forward alias doesn't overwrite the default alias.

#24.1 - I have tried to add a workaround for this, but not too happy with it.

#24.2 - added comments.

Also updated the test / patch to fix a new issue I thought about. What if you create a default revision, then don't change the alias in the forward revision, you'll get two aliases the same, but with different sources. I am now loading the alias for the canonical path, and not creating a new one if one already exists.

When discussing this with @amateescu he pointed me to #2846554: Make the PathItem field type actually computed and auto-load stored aliases, so I wonder if it's worth postponing on that?

Test didn't pass because of the widget.

This issue was discussed during the Hard Problems meeting at DrupalCon in Baltimore. See notes and decisions here: https://docs.google.com/document/d/1-5B-Gndhklns20NZpGQul10525rFWuYB2NbT...

From the Workflow Initiative meeting last week (see #28) the action items included "Quick solution should move forward", which is this issue.

This patch fixes #24.1 properly.
It also tests the widget returns the correct default value each time we load the edit form.
Then extends the test to test deleting an whole entity as well as deleting a single revision.

Deleting a whole entity resulted in the following change

-        $query->condition($field, $this->connection->escapeLike($value), 'LIKE');
+        $query->condition($field, $value, 'LIKE');

so we could delete all revision aliases with a source such as '/node/1/revisions/%'. I'm not really happy with this change.

Also deleting a single revision isn't working because \Drupal\path\Plugin\Field\FieldType\PathFieldItemList::delete is only called when deleting a whole entity. I'm wondering if this could be considered a core bug.

This duplicates some code, but fixes the issue of aliases still existing after deleting a revision.

Looking at how #2846554: Make the PathItem field type actually computed and auto-load stored aliases might've changed things, and re-running tests for that reason too.

Re-roll plus updates based on #2846554: Make the PathItem field type actually computed and auto-load stored aliases.

I did a manual testing and working as expected. We have revision with it's own path. Not sure if this is exactly same as proposed solution in issue summary. Here is some code review comments(mostly optional improvements).

1. +++ b/core/lib/Drupal/Core/Path/AliasStorage.php
   @@ -160,7 +165,7 @@ public function delete($conditions) {
   -        $query->condition($field, $this->connection->escapeLike($value), 'LIKE');
   +        $query->condition($field, $value, 'LIKE');
   

   using "LIKE" without excapeLike?

2. +++ b/core/modules/path/path.module
   @@ -86,8 +87,23 @@ function path_entity_translation_create(ContentEntityInterface $translation) {
   +function path_entity_revision_delete(EntityInterface $entity) {
   
   +++ b/core/modules/path/src/Plugin/Field/FieldType/PathFieldItemList.php
   @@ -32,6 +32,15 @@ public function delete() {
   +    $entity_type_id = $entity->getEntityTypeId();
   +    $source = str_replace('{' . $entity_type_id . '}', $entity->id(), $entity->getEntityType()->getLinkTemplate('revision'));
   +    $source = str_replace('{' . $entity_type_id . '_revision}', '%', $source);
   +    $revisionable_conditions = [
   +      'source' => $source,
   +      'langcode' => $entity->language()->getId(),
   ...
   +    \Drupal::service('path.alias_storage')->delete($revisionable_conditions);
   

   Looks like same code repeated. Is it worth having a helper?

3. +++ b/core/modules/path/src/Plugin/Field/FieldType/PathItem.php
   @@ -114,23 +114,33 @@ public function set($property_name, $value, $notify = TRUE) {
   +    if ($this->alias && (!$update || $source != $this->source)) {
   

   Can we have a comment here on what are we checking?

4. +++ b/core/modules/path/src/Plugin/Field/FieldType/PathItem.php
   @@ -164,12 +174,19 @@ protected function ensureLoaded() {
   +        if ($entity->getEntityType()->isRevisionable() && !$entity->isDefaultRevision()) {
   

   This is the typical condition to check if we are in non-default revision on a revisionable entity. Worth having it as a API in entity level(i.e. allows to check any additional checks in future in one location)?

5. +++ b/core/modules/path/tests/src/Functional/PathContentModerationTest.php
   @@ -0,0 +1,152 @@
   +  public function testNodePathAlias() {
   

   Missing docblock.

#36.1 - yes, LIKE without escapseLike() is correct. escapleLike removes all '%' and '_' characters, and we need them now. See \Drupal\Core\Database\Connection::escapeLike

#36.2 - I had considered this and wasn't sure a service for this would add much, and a helper function is a definite no. I'll give it more thought.

#36.3 - Added

#36.4 - Maybe we can do that in a followup? But not really convinced of the gain.

#36.5 - Added

When thinking about #36.4 I decided that we need to depend on an entity type having a revision link template, not just being revisionable.

For #36.2 I wondered if we could extend AliasStorage, to create something like EntityAliasStorage, however I don't think this is feasible. We could add a another service, but I'm not sure it's needed at this stage.

Path module only works with Node, Taxonomy Term, and Media. Taxonomy Term is not revisionable yet, but Media is and doesn't have a revision link template. So will regress back to the functionality before this patch until it gets one. #2885486: Media entity is revisionable but doesn't have a revision link template is a follow up for this. Not making it a requirement as Media is experimental.

#40.1 - I think an OR query could work. All we're trying to do here is simplify the code. We could so something like:

$alias = \Drupal::service('path.alias_storage')->load($conditions);
if (empty($alias)) {
  $alias = \Drupal::service('path.alias_storage')->load($revision_conditions);
}

#40.2 - This allows us to do real LIKE conditions with values such as '/node/1/revision/%'.

I hope this addresses the concerns in #40.

Here's an interdiff for #44.

+++ b/core/modules/path/tests/src/Functional/PathContentModerationTest.php
@@ -0,0 +1,155 @@
+    // Add another forward revision with no alias.
+    $this->drupalGet('node/' . $node->id() . '/edit');
+    $this->assertSession()->fieldValueEquals('path[0][alias]', '/revision-three');
+    $this->drupalPostForm('node/' . $node->id() . '/edit', [
+      'title[0][value]' => 'revision four',
+      'path[0][alias]' => '/revision-four',
+    ], t('Save and Create New Draft'));

The patch in #44 is incorrect because this test is testing what happens when no path aliases is added and the field is left empty.

Fixing #42 and #44.

@catch - I think this is a better fix than the validation, and isn't too disruptive. At least it gives a workable solution, where as the validation and the solution in #2766957: Forward revisions + translation UI can result in forked draft revisions prevent people from doing what they want.

Also, this patch is written, writing the validation quick fix would take more time, which would could be spending on the final solution or other issues.

Would this make paths fully revisionable? Does the patch here also support revert-ing a revision and preserving the audit trail?

It looks like it does but just want to verify.

I wouldn't go as far as saying it makes paths fully revisionable, but it allows you to have aliases on non-default revisions. I've not tested reverting so not 100% what happens.

Either way, this is much better than what we currently have, and better than preventing users from creating "draft" aliases.

Thanks @timmillwood

2 questions:
- Is the proposed data structure agreed upon? As in, will the revision path be the storage of the alias? I know there has been back and forth on the approach in the issue but would be good to have consensus as @timmillwood mentioned.
- If that is agreed upon, would it be okay to back port and post a patch for 8.3.x? This will help those looking to solve this issue in the short term until 8.4 is released. However, this is a little more complicated as ensureLoaded doesn't exist in 8.3 and I believe #2846554 is the source of that. Also, I do not want to create noise for this issue with, just want to give people a bandaid

I can't see this being back ported to 8.3, although you could backport it yourself and just patch 8.3 sites you run, this is done in many issues, where people share the 8.3 version of a patch.

@timmillwood, forsure!

With that though, for the sake of data parity/upgradability, etc, how certain do we feel that the revision template will be will be the proper identifier? Not trying to force too much of a decision, I just know that there was a PoC, other discussion and that developed into this patch. So just want to verify and update the issue description to reflect path forward.

@catch, I don't believe validation will work as a stop gap because of things like scheduled updates, or if content moderation is forced / always revisioned, or for companies that have a strict publishing policy must be a draft before being published, etc.

@catch, fully understand.

How would validation work if revisions are forced and the defaultRevision can't be edited? Or would the stop gap be revisions have to be toggable for the leaks to stop?

I have just been testing out reverting and this patch doesn't change the current state of reverting content with path aliases in HEAD, which I think it (maybe unrelatedly) broken.

Steps to reproduce (with or without Content Moderation enabled):

1. Create a new node with the title "foo" and path alias "/foo".
2. The path /foo will show the content.
3. Edit the content, update the title to "foo2" and path alias "/foo2".
4. The path /foo2 will show the content, /foo with throw a 404 error.
5. Revert to the first revision.
6. The path /foo2 will show the content (with title "foo"), /foo with throw a 404 error.

Taking this patch in the same direction as #2858434: Menu changes from node form leak into live site when creating draft revision and #2858431: Book storage and UI is not revision aware, changes to drafts leak into live site by throwing an error message on forward revisions, and preventing the URL alias from being updated.

Sorry if I'm not reading the patch right, but I don't think this will work in some cases and I think those cases are enough to justify thinking through this more.

If you have a workflow of:

- Draft/Published -> Draft
- Draft ->Published
- Published -> Archived
- Archived -> Published

You cannot edit the default revision.

Further, how would something like pathauto work in this? Can the title never change if the default revision isn't being edited and the path created off of node:title?

So just tried the patch on simplytest.me, looks as though you can save the node and are presented with an error.

This could be confusing but it doesn't stop the editing. Sorry for the confusion

I'm not sure I understand #63, but as you say in #64, it does work. To stop it being confusing it's vital we get the wording correct.

Pathauto could be an interesting one, but here we wouldn't be altering what Pathauto does or how it works, so it would carry on as it is.

I thought a screenshot might be useful here.

From a usability perspective: What if the content author does not remember the original alias?

The original alias doesn't get lost / changed.

The only "lost" input is the alias they just entered before saving.

Ok, good.

Reviewed this with Roy, Gabor, Adam Hoenich and Andrei.

Thanks for raising this, this is quite tricky - but actually solvable. While the ideal solution involves actually raising a validation error and recovery upon submission, we thought given the engineering, and timespan it will effect the user (hopefully resolved with workspaces) and number of users it will affect that this is not worth it.

Therefor the message is for us, a good intermediate solution to continue with.

What the message should contain:

• How to resolve the problem that occurred.
• Show which fields we couldn't save/change.
• What the brilliant values where, that you entered enabling users to recover the data that was lost

I@m highly confused about this code change. I would have expected something which works on the field widget level, not being part of the storage API.

@dawehner - I think doing this in the widget might be too early, see #2883868: Content Moderation decides to set a new revision as the default one way too late in the entity update process.

@Bojhan, I implemented the proposed changes from the call in this issue: #2858431-51: Book storage and UI is not revision aware, changes to drafts leak into live site if you want to take a look :)

This is marked as MUST-HAVE in the roadmap.

"Might be", this is quite a vague reason :)

If I would work on this patch I would try to write a constrain validator, as this place allows you to also provide a human readable error message.

#2883868: Content Moderation decides to set a new revision as the default one way too late in the entity update process was merged, this can now be properly "fixed" with a validation error.

Here's the patch with a new constraint validator. And this is how it looks like, just like a regular entity form validation error:

I try to understand what this means in the context of pathauto, which changes path aliases automatically. Given that pathauto should change its values just for published entities as well, right?

1. +++ b/core/modules/path/src/Plugin/Validation/Constraint/PathAliasConstraint.php
   @@ -0,0 +1,19 @@
   + *   id = "PathAlias",
   
   +++ b/core/modules/path/src/Plugin/Validation/Constraint/PathAliasConstraintValidator.php
   @@ -0,0 +1,57 @@
   +class PathAliasConstraintValidator extends ConstraintValidator implements ContainerInjectionInterface {
   

   I wonder whether we can find a better name, but I cannot come up with a good short name to be honest.

2. +++ b/core/modules/path/src/Plugin/Validation/Constraint/PathAliasConstraintValidator.php
   @@ -0,0 +1,57 @@
   +      $original = $this->entityTypeManager->getStorage($entity->getEntityTypeId())->loadUnchanged($entity->id());
   

   Isn't the right thing to do to use $entity->getLoadedRevisionId() to load the original?

Looks great and works as advertised, attaching a test-only patch.

+++ b/core/modules/path/src/Plugin/Validation/Constraint/PathAliasConstraintValidator.php
@@ -0,0 +1,57 @@
+      $original = $this->entityTypeManager->getStorage($entity->getEntityTypeId())->loadUnchanged($entity->id());

I'm wondering whether it would make sense to use $entity->original if it is available.

Isn't the right thing to do to use $entity->getLoadedRevisionId() to load the original?

I think we need the "actual" default revision here: we don't want the user to change the alias wrt what's actually published, I think, even if it's different from the value in the revision being the default one when the form was instantiated.

I think we need the "actual" default revision here: we don't want the user to change the alias wrt what's actually published, I think, even if it's different from the value in the revision being the default one when the form was instantiated.

Oh you are totally right here!

Failed as expected, patch to review is #77 (green!)

Oops, #78 was empty :(

1. +++ b/core/modules/path/src/Plugin/Validation/Constraint/PathAliasConstraintValidator.php
   @@ -0,0 +1,57 @@
   +      $original = $this->entityTypeManager->getStorage($entity->getEntityTypeId())->loadUnchanged($entity->id());
   

   I think $entity->original should work, but loadUnchanged() might give a more predictable result.

2. +++ b/core/modules/path/tests/src/Functional/PathContentModerationTest.php
   @@ -0,0 +1,101 @@
   +      'title[0][value]' => 'forward revision',
   

   Forward revision? draft revision? pending version? :D

Failed as expected for reals now, patch to review is still #77.

I wonder whether we can find a better name, but I cannot come up with a good short name to be honest.

I also tried to figure out a better name but nothing came up..

I think $entity->original should work, but loadUnchanged() might give a more predictable result.

Well, it doesn't, I tried :) $entity->original is not set at that point.

Let's get this in :)

Patch to commit is #77

In light of #2875154: Clarify and document wording around drafts, revisions, published & friends, should we change all uses of "revision" to "version" in the user facing text? The screenshot in #77 uses both in a single message :)

@yoroy, that's an excellent point! Now that the patch actually stops the form from being submitted I think we can also simplify the message a bit and drop the first sentence.

+++ b/core/modules/path/src/Plugin/Validation/Constraint/PathAliasConstraintValidator.php
@@ -0,0 +1,57 @@
+      if ($value->alias != $original->path->alias) {

This is the only line in the constraint validator that is PathItem-specific.

I'm fine with deferring this to a follow-up, but I think we should be able to make this completely generic by using FieldItemListInterface::equals(), i.e. with the following:

$original_value = $original->get($value->getName());
if (!$value->equals($original_value)) {
  ...
}

Not sure if that would actually be useful to the related book or menu patches.

That said - again - I don't want to unnecessarily hold this up, as it would be important to get this one in, so would be fine with tackling that in a follow-up. But thought it would be useful to bring up since we have all the smart folks here in the issue ;-)

@tstoeckler, that's how I approached the problem initially, but book and menu_ui do not provide fields at all, they're doing old-style form alters so there's nothing really reusable here.