Problem/Motivation
Since #2428795: Translatable entity 'changed' timestamps are not working at all got in core there is a mixed behaviour. Previously when saving an entity through the API and the UI would have resulted always in an updated changed timestamp. Now the ChangedItem introduces some behavioural logic so that it checks if the entity changed and only then updates its value.
This means accessing Entity Edit through the UI and directly saving it without changes will not update the changed timestamp anymore.... unless the module Content Translation is enabled, which will in an Entity Builder call setChangedTime(REQUEST_TIME) for the current translation. So here we should decide if we want that the changed timestamp is always updated on UI Save and then move the logic for that out of the Content Translation module so that it is not dependent on an optional module.
Some tests in core with no Content Translation module enabled were excepting that calling Entity Edit on the UI and saving withouth changes will result in a change and because this behaviour is missing now there was a neccessary change in #2480921: Make the node entity's revision_log untranslatable in a test to force the entity to be updated.
Proposed resolution
If the entity implements the EntityChangedInterface, update its changed timestamp after the entity is validated and before the entity is saved in ContentEntityForm::submitForm.
#2534512: EntityChangedConstraint needs to compare changed time of all translations showed that the EntityChangedConstraintValidator is malfunctioning and does not allow old translations to be saved, which we've seen in the current issue after we've changed the behaviour that the changed timestamp is updated after the entity is validated. So the right functioning EntityChangedConstraintValidator is needed here, where we have also test coverage for it. That is why we are integrating it in the current patch for this issue and setting the other one as duplicate.
Remaining tasks
none
User interface changes
none
API changes
The EntityChangedInterface now defines also the method setChangedTime.
The EntityChangedTrait implements also the setChangedTime method.
Comment | File | Size | Author |
---|---|---|---|
#64 | 62-64-interdiff.txt | 4.28 KB | hchonov |
#64 | 2506213-64.patch | 17.21 KB | hchonov |
#62 | 60-62-interdiff.txt | 605 bytes | hchonov |
#62 | 2506213-62.patch | 15.62 KB | hchonov |
#60 | 49-60-interdiff.txt | 405 bytes | hchonov |
Comments
Comment #1
plachComment #2
hchonovComment #3
hchonovTagging as blocker as #2480921: Make the node entity's revision_log untranslatable is postponed on this.
Comment #4
alexpottI think pressing save in the UI should result in the time being updated. Unless we don't save the entity and reject the save and inform the user nothing has changed so not saving.
Comment #5
hchonovso, we just agreed with @alexpott in IRC that we should update the timestamp on UI save.
Comment #6
hchonovhere providing test only patch and setting a default value for the revision_log field of the Node, as otherwise it is causing a change on node save. The current test should fail.
Comment #7
hchonovAnd here the test including the fix.
Comment #18
hchonovUnfortunatelly as I added a default value to the node revision_log field one REST test for node creation is failing, as there it was forgotten to remove the revision_log field for non admin users, so here I am adding also these chages.
Comment #19
hchonovAnd now both the test and the fix.
Comment #21
hchonovComment #22
mkalkbrennerIf we agree on providing the function setChangedTime() it should be part of the EntityChangedTrait. BTW it seems that getChangedTime() could be moved to this trait as well now. That could be added to this patch from my point of view.
Is this the right issue to fix that?
This test only covers Node and its UI. But the patch introduces a behavior for all entities implementing the EntityChangedInterface.
Therefor I recommend to extend
ContentEntityChangedTest first which is supposed to test the functions declared by EntityChangedInterface on the API level.
The UI should be covered by an additional function of ContentTranslationUITestBase. This way it will cover all the entities that are already affected by this patch.
Comment #23
hchonovAs suggested by @mkalkbrenner I've put setChangedTime and getChangedTime in the EntityChangedTrait.
The problem occurs here and the new test fails because of this case. If requested so I will create another issue.
Why should we create an test exactly in ContentTranslationUITestBase? At the moment my changes have nothig to do with the content translation module.
Comment #24
mkalkbrennerIt's a suggestion because the tests implemented there are automatically executed for all entity types that are affected by this patch while the test included in the patch only covers nodes!
Comment #25
alexpottI gave a framework manager point of view in #4
Comment #26
webchickAgreed with alexpott in #4.
Comment #27
BerdirThis is wrong.
Changing the changed time breaks the changed validation that we have on forms that happens *later*. This means that an old entity is always considered up to date. We *must* use the submitted, original changed time for that validation and only change it when the validation succeeded.
This is hiding the currently broken EntityChangedConstraintValidator that is comparing the saved changed date of all translations with the changed date of the current translation, which makes it impossible to save old translations.
Comment #28
mkalkbrenner@berdir is right.
Whenever an entity is saved via the UI it's original changed timestamp must checked if it's still the newest (or if a different user modified the entity in the meantime).
That's what the EntityChangedConstraint normally does. Therefor this issue here is also indirectly blocked by #2513094: ContentEntityBase::getTranslatedField and ContentEntityBase::__clone break field reference to parent entity.
Nevertheless we need to have a look at Content Translation if it deals correctly with that situation.
Comment #29
hchonovSo reworked the patch. Now ContentTranslationHandler::entityFormEntityBuild will update the changed timestamp on the metadata only if the entity has a metadata field.
The original entity changed field now will be updated in ContentEntityForm::submitForm after the validation has ran.
Comment #31
hchonovNow including the patch from #2534512: EntityChangedConstraint needs to compare changed time of all translations to reduce the test fails as with this changes now we bring up the broken EntityChangedConstraintValidator to fail.
Comment #33
hchonovSo the patch from #29 passes localy with applied both patches for #2534512: EntityChangedConstraint needs to compare changed time of all translations and for #2513094: ContentEntityBase::getTranslatedField and ContentEntityBase::__clone break field reference to parent entity.
So postponing this one on the both issues. As soon as they are fixed the patch from #29 should be put for retesting and it should pass.
Comment #34
hchonovComment #35
mkalkbrennerI still don't like that we only add test coverage for nodes. The patch modifies ContentEntityForm.
Comment #36
mkalkbrennerAs explained earlier I copied the test case to ContentTranslationUITestBase.
Without really changing the test we now have a great coverage of content entities in combination with content_translation. That's correct because module is also affected by this patch.
Let's see which side effects the additional test might discover.
Comment #37
mkalkbrennerComment #39
hchonovComment #40
mkalkbrennerAs @berdir suggested in https://www.drupal.org/node/2534512#comment-10156016 I merged that patch to see how the tests work here now.
Comment #43
hchonovThe test passes locally, so putting for retesting.
Comment #45
hchonovSo the problem is, that ContentTranslationUITestBase::doTestChangedTimeAfterSaveWithoutChanges is checking the changed field, but not all entities, which extend ContentTranslationUITestBase are implementing the EntityChangedInterface. So we should restrict the test ContentTranslationUITestBase::doTestChangedTimeAfterSaveWithoutChanges only to entites, which are implementing the EntityChangedInterface.
Comment #46
hchonovSo here we proved the false implementation of the EntityChangedConstraint by integrating the patch from #2534512: EntityChangedConstraint needs to compare changed time of all translations .
Now removing the fix for the EntityChangedConstraint and RTBC-ing the other issue, so that both patches land separately in core.
The patch should fail now, and it should be put for retest as soon as #2534512: EntityChangedConstraint needs to compare changed time of all translations gets commited.
Comment #48
BerdirThe thing is that the other issue won't be RTBC'd without test coverage. test coverage that this issue provides, at least in the form integration tests.
As I suggested in #2534512-13: EntityChangedConstraint needs to compare changed time of all translations, if we merge that issue into this, then we have a better chance of it getting committed I think. It's a one line fix, and we can explain in the issue summary why that is needed and close the other issue as a duplicate.
PS: You want to reference to other issues with "[ #2534512 ]" (without the spaces). That way, you get things like automatic coloring. If you are using dreditor (which you should) then you can just paste the issue URL and then press Tab and it will convert it automatically. I'm always irritated that your links don't have the color I'd expect, took me a while that you are probably building them by hand.
Comment #49
hchonovChanged title and updated the IS.
I've set #2534512: EntityChangedConstraint needs to compare changed time of all translations as duplicated, so the patch here is fixing also the problem with the EntityChangedConstraintValidator.
Comment #50
mkalkbrennerComment #51
hchonovComment #53
hchonovComment #54
Gábor HojtsyThe changes proposed here make sense to me, the issue summary is up to date and @Berdir's concerns have been resolved. Let's get this in, so we can fix #2480921: Make the node entity's revision_log untranslatable too finally :)
Comment #57
hchonovSetting again as RTBC as there was some error on the testbot and the patch failed, but after retesting it is passing again.
Comment #58
kataku CreditAttribution: kataku as a volunteer commentedSorry if this is just piling on to what you already know but i was desperate for this fix, I've tested this patch on 8.0.x head and it worked for me without error :)
Comment #59
plachA few nitpicks, if this happens to be rerolled:
We can use
.class
here, I thinkMissing PHP doc
Comment #60
hchonov@plach:
1. unfortunately we cannot use getClass as for example for the node entity the class will be "Drupal\node\Entity\Node" and here we check if the entity is implementing the interface "Drupal\Core\Entity\EntityChangedInterface" .
2. added the mising PHP doc.
Thanks for the nitpicks :)
Comment #61
plachI meant
EntityChangedInterface::class
:)Comment #62
hchonovOh, clearing that nitpick then as well... :).
The tests are using a different namespace, so there either we have to put the EntityChangedInterface in the usage definition or use the whole path when calling isSubclassOf, so I think using the whole path is fine in the test.
Comment #63
catchAll minor but a couple of real questions.
Nice adding this here and removing from the entity classes :)
a metadata
Why do we have to?
Double quotes?
Left over debug?
Why sleep(2) here and sleep(1) in the other test?
Comment #64
hchonov@catch:
Thanks for the remarks.
2. Corrected.
3. ContentEntityForm::submit will update the changed timestamp on submit after the entity has been validated, so that it does not break the EntityChanged constraint validator. I think it is better to have the same logic in CT for the changed metadata field even if there is no such constraint defined for the metatada field at the moment and moved now the logic in a submit function instead using an entity builder for this like in the previous case, which is being executed before the entity is saved. So I am just reproducing the logic from the Form API. I wrote a better comment in the patch as well.
4. Corrected.
5. Removed left over debugs.
6. Unintentionally. Now both are sleep(1).
Comment #65
plach#64 seems to address @catch's review.
Comment #66
catchYep. Committed/pushed to 8.0.x, thanks!
Comment #68
larowlanThis was an API break, so needs a change record.
Anyone with an entity implementing EntityChangedInterface would now get fatals until they add the method or the trait.
Thanks
Comment #69
hchonov@larowlan:
I just created a draft change record - EntityChangedInterface now also defines the function setChangedTime.
Should I publish it now?
Comment #70
larowlanthanks!
published