Report based on #2930599: Unable to save a translation if the path alias changes (which is 'works as designed' imho).

Problem/Motivation

User can't add new draft for a content translation of a node if the path alias of the latest published entity (original language) is different from the path alias of the new translation draft's form.

Steps to reproduce

Setup

  1. (as an administrator user, suggested install profile: standard)
  2. Enable node, content_moderation, content_translation and path modules.
  3. Add an another language.
  4. Create a node type, add a workflow to it and make it translatable. Use the defaults settings of the content translation form.
  5. Create a node with in the default language with path alias. Publish it.
  6. Create a translation for the node with a translated path alias which is different from the one the default node has. Publish this translation as well.

Scenario 1

  1. Try to add a new draft for the translation while not changing the path alias.
  2. Expected: new draft is successfully saved while alias of the published translations remain the same.
  3. What happens: PathAliasConstraintValidator returns a validation error You can only change the URL alias for the published version of this content.

Scenario 2

  1. Now try to add a new draft for the translation while changing the path alias to the one what the original published entity revision has (original means the original language).
  2. Expected: Validation error.
  3. What happens: New translation draft successfully saved, and the alias of the published (default) translation revision changes to the one we just filled in.

Proposed resolution

Extend logic of PathAliasConstraintValidator to check for the default revision of the current entity language, and not the original entity.

Remaining tasks

Copy my patch from #2930599: Unable to save a translation if the path alias changes here.
Extend it with a test for Scenario 2
Get a review.

User interface changes

Nothing.

API changes

Nothing.

Data model changes

Nothing.

CommentFileSizeAuthor
#39 image-2021-11-03-17-17-25-037.png59.3 KBsamsylve
#35 path-new_translation_draft_alias-3001124-35-8-5-x-backward-compatibility.patch8.42 KBjulienjoye
#26 interdiff-3001124-20-26.txt13.84 KBjohnchque
#26 path-new_translation_draft_alias-3001124-26.patch8.47 KBjohnchque
#26 path-new_translation_draft_alias-3001124-26-test-only.patch7.25 KBjohnchque
#20 interdiff-3001124-19-20.txt805 bytesjohnchque
#20 path-new_translation_draft_alias-3001124-20.patch8.17 KBjohnchque
#20 path-new_translation_draft_alias-3001124-20-test-only.patch6.95 KBjohnchque
#19 interdiff-3001124-16-19.txt6.27 KBjohnchque
#19 path-new_translation_draft_alias-3001124-19.patch8.17 KBjohnchque
#19 path-new_translation_draft_alias-3001124-19-test-only.patch6.95 KBjohnchque
#16 interdiff-3001124-14-16.txt2.16 KBjohnchque
#16 path-new_translation_draft_alias-3001124-16.patch8.76 KBjohnchque
#16 path-new_translation_draft_alias-3001124-16-test-only.patch7.55 KBjohnchque
#14 path-new_translation_draft_alias-3001124-14.patch7.96 KBjohnchque
#10 path-new_translation_draft_alias-3001124-10-complete.patch7.84 KBhuzooka
#7 path-new_translation_draft_alias-3001124-7-test_only.patch6.75 KBhuzooka
#4 path-new_translation_draft_alias-3001124-4-scenario1-complete.patch6.6 KBhuzooka
#4 path-new_translation_draft_alias-3001124-4-scenario1-test_only.patch5.52 KBhuzooka
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

huzooka created an issue. See original summary.

huzooka’s picture

Issue summary: View changes
huzooka’s picture

Version: 8.7.x-dev » 8.6.x-dev
Status: Active » Needs work

Let's check whether the bug exists in 8.7.x. Till I confirm it locally, I change the target version to 8.6.x.

huzooka’s picture

Verified that 8.7.x is also affected.

Test-only and "complete" test for Scenario 1.

huzooka’s picture

Assigned: Unassigned » huzooka
Status: Needs review » Needs work
huzooka’s picture

huzooka’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
huzooka’s picture

Assigned: huzooka » Unassigned
Status: Needs work » Needs review
FileSize
7.84 KB

Complete test.

huzooka’s picture

Issue summary: View changes
huzooka’s picture

Berdir’s picture

This is an improvement, but there is also the case when the translation does *not* exist.

This happens when you add add a new translation as draft, or when editing an existing translation that has not yet been published, ever.

The weird thing is that we are then comparing against the default translation. That means when adding a new translation, then the the alias must be set to the same value as the default revision of the default translation and it actually creates an alias then for the draft translation, but looking at that will then load the default revision and fall back to the original language.

And you then also can't change it later.

Since we are explicitly and currently always storing aliases by language, I think we should only compare if the language matches? So when adding an initial draft, you can enter whatever you want and you can too if you also change it as long as it hasn't been published yet.

The result would be something like this flow:

1. Create EN as published: anything allowed, setting to /foo
2. Add DE translation as draft: anything allowed, setting to /foo-de
3. Update DE draft: anything allowed, setting to /foo-de-changed
4. Edit again and then publish DE draft: anything allowed, setting to /foo-de-published
5. Creating a new EN draft: No changes allowed
7. Creating a new DE draft: No changes allowed

This gets more broken when using pathauto, which does not have that special initial case, it simply never generates an alias for a non-default revision right now. Which means that the DE draft in step 2 does not get an alias at all, and then editing that again shows an empty alias *and* trying to save results in a validation error.

johnchque’s picture

Updating the addition of the violation, will work on tests later on today. :)

Status: Needs review » Needs work

The last submitted patch, 14: path-new_translation_draft_alias-3001124-14.patch, failed testing. View results

johnchque’s picture

Made some changes in the new added tests, think that is the expected behavior now (?).

Berdir’s picture

Status: Needs review » Needs work
+++ b/core/modules/path/src/Plugin/Validation/Constraint/PathAliasConstraintValidator.php
@@ -48,8 +48,13 @@ public function validate($value, Constraint $constraint) {
+      // Only add the violation if the translation exists on $original.
+      if ($original->language()->getId() !== $entity_langcode && $original->hasTranslation($entity_langcode)) {
+        if ($value->alias != $original->path->alias) {
+          $this->context->addViolation($constraint->message);
+        }

you are checking if the translation exists now, but you're still comparing with the default language of $original, you lost the getTranslation() call from the previous patch.

We also need to merge the new test into the existing one for the parts that are there not tested yet.

johnchque’s picture

Updated the condition and removed deprecated methods. :)

johnchque’s picture

amateescu’s picture

  1. +++ b/core/modules/path/tests/src/Functional/PathContentTranslationModerationTest.php
    @@ -0,0 +1,164 @@
    + * @droup path
    

    Is @droup a new thing? :D

  2. +++ b/core/modules/path/tests/src/Functional/PathContentTranslationModerationTest.php
    @@ -0,0 +1,164 @@
    +  public function testTranslatedModeratedNodeAlias() {
    

    I would still like to have this method in the existing PathContentModerationTest class.

johnchque’s picture

Thinking about having it in the same class, it would mean of extending the setup a lot, even enabling a module that is not needed in the existing test. Would that be OK? :)

amateescu’s picture

I think it's ok to enable an additional module in the existing test. And all the languge-related code could be moved to the new test method.

Also, don't forget to remove all the t() calls from the test :)

johnchque’s picture

Cool, there we go. :) As suggested, removed the t()'s and moved the test to an existing one. :)

The last submitted patch, 26: path-new_translation_draft_alias-3001124-26-test-only.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

amateescu’s picture

Status: Needs review » Reviewed & tested by the community

Great! Let's do this :)

idflood’s picture

I tested the patch on a production website where we hit this issue and I confirm this works nicely.

phjou’s picture

I also confirm that it is working great. Thank you.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed 0d1e6dd and pushed to 8.7.x. Thanks!

  • catch committed 0d1e6dd on 8.7.x
    Issue #3001124 by yongt9412, huzooka, amateescu, Berdir: Unable to...
catch’s picture

Version: 8.7.x-dev » 8.6.x-dev

And cherry-picked to 8.6.x

  • catch committed 8814ec8 on 8.6.x
    Issue #3001124 by yongt9412, huzooka, amateescu, Berdir: Unable to...
julienjoye’s picture

Thank you for the patch yongt9412, it works well here.
I rolled it back for 8.5.8 version, if someone else needs this...
Cheers.

maximpodorov’s picture

Will this be committed to 8.5?

Status: Fixed » Closed (fixed)

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

vuil’s picture

#35 works for us, on Drupal 8.5.14

samsylve’s picture

We have this error in Drupal 9 when we try to sabe a Media in Draft even if it has a translation.
You can only change the URL alias for the published version of this content