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
- (as an administrator user, suggested install profile:
standard
) - Enable
node
,content_moderation
,content_translation
andpath
modules. - Add an another language.
- Create a node type, add a workflow to it and make it translatable. Use the defaults settings of the content translation form.
- Create a node with in the default language with path alias. Publish it.
- 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
- Try to add a new draft for the translation while not changing the path alias.
- Expected: new draft is successfully saved while alias of the published translations remain the same.
- What happens:
PathAliasConstraintValidator
returns a validation error You can only change the URL alias for the published version of this content.
Scenario 2
- 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).
- Expected: Validation error.
- 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.
Comments
Comment #2
huzookaComment #3
huzookaLet's check whether the bug exists in 8.7.x. Till I confirm it locally, I change the target version to 8.6.x.
Comment #4
huzookaVerified that 8.7.x is also affected.
Test-only and "complete" test for Scenario 1.
Comment #6
huzookaComment #7
huzookaTest only patch...
Comment #8
huzookaComment #10
huzookaComplete test.
Comment #11
huzookaComment #12
huzookaComment #13
BerdirThis 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.
Comment #14
johnchqueUpdating the addition of the violation, will work on tests later on today. :)
Comment #16
johnchqueMade some changes in the new added tests, think that is the expected behavior now (?).
Comment #18
Berdiryou 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.
Comment #19
johnchqueUpdated the condition and removed deprecated methods. :)
Comment #20
johnchqueFixing double space, thanks @Berdir!
Comment #21
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedIs
@droup
a new thing? :DI would still like to have this method in the existing
PathContentModerationTest
class.Comment #22
johnchqueThinking 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? :)
Comment #23
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedI 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 :)Comment #26
johnchqueCool, there we go. :) As suggested, removed the t()'s and moved the test to an existing one. :)
Comment #28
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedGreat! Let's do this :)
Comment #29
idflood CreditAttribution: idflood at Stimul.ch commentedI tested the patch on a production website where we hit this issue and I confirm this works nicely.
Comment #30
phjouI also confirm that it is working great. Thank you.
Comment #31
catchCommitted 0d1e6dd and pushed to 8.7.x. Thanks!
Comment #33
catchAnd cherry-picked to 8.6.x
Comment #35
julienjoye CreditAttribution: julienjoye at Ekino commentedThank you for the patch yongt9412, it works well here.
I rolled it back for 8.5.8 version, if someone else needs this...
Cheers.
Comment #36
maximpodorov CreditAttribution: maximpodorov commentedWill this be committed to 8.5?
Comment #38
vuil#35 works for us, on Drupal 8.5.14
Comment #39
samsylve CreditAttribution: samsylve commentedWe have this error in Drupal 9 when we try to sabe a Media in Draft even if it has a translation.