This is a follow-up from #2837022: Concurrently editing two translations of a node may result in data loss for non-translatable fields and detected thanks to the manual testing done by @matsbla so please make sure that he receives credit on this issue.
The problem found in #41 @matsbla is that content_translation allows for making only specific field properties translatable or not translatable and syncs the non-translatable properties in hook_entity_presave(), which means content_translation will be updating a field after the check for changes by ChangedItem::preSave() has run. This makes it impossible to update the changed timestamp, because the entity field preSave method is invoked before the entity presave hooks.
Invoke ChangedFieldItemList::preSave() once again after all the presave process has run to detect all changes that have been made during the presave process.
| Comment | File | Size | Author |
|---|---|---|---|
| #33 | 2912318-nr-bot.txt | 134 bytes | needs-review-queue-bot |
| #17 | interdiff-3-17.txt | 2.27 KB | hchonov |
| #17 | 2912318-17.patch | 5.36 KB | hchonov |
| #3 | 2912318-3.patch | 4.41 KB | hchonov |
| #3 | 2912318-3-FAIL.patch | 2.49 KB | hchonov |
Comments
Comment #2
hchonovComment #3
hchonovProviding a failing test and an updated path. This approach should resolve the both related issues #2617628: ChangedItem will not detect a change if another field item changes its value in field::preSave and #2863844: Ensure that the changed field will update its value even if it is not the last field in pre save.
Comment #4
hchonovComment #5
drummUpdating to the canonical tag for DrupalCon Vienna 2017.
Comment #7
matsbla commented@hchonov, thanks for working on this patch! I did some manual testings now and looks like it solves the problem, great work =)
Comment #8
charlotte.b commentedI'm current working on testing this with @joycelam at DrupalCon Vienna 2017
Comment #9
charlotte.b commentedBecause this is a follow up issue to https://www.drupal.org/node/2837022, I followed the steps in comment https://www.drupal.org/node/2837022#comment-12203105
Full steps:
- Activated language and content translation modules
- Added a second language (French)
- Go to content language settings page
- Select Content and select a content type
- Hit save configuration
- Go to the content type configured above and add a new image field
- Edit the field and Deselect file under translatable elements
- Created a new content page with an image for English
- Added a translation for French
- Both of the translations have the new image
- I edited both translations at the same time, updating the image for one
Both translations ended up with the same image, so I wasn't able to replicate the issue in https://www.drupal.org/node/2837022#comment-12203105 however the updated time only changed for the first translation and after applying the patch, both of the updated times are successfully updated. Can you please confirm that this bug is just to fix the updated time and not the issue referred to in the comment?
I have been working on this with my mentor @sugaroverflow please let me know if there are other steps I need to take to replicate this issue.
Comment #10
matsbla commentedhi charlotte, thanks for testing this! :)
> I wasn't able to replicate the issue in https://www.drupal.org/node/2837022#comment-12203105
So does it mean that when you follow the steps described there you get a lock? or what happens?
when I test with this patch I get a lock also when untranslatable image files are updated avoiding that they will be updated concurrently (which shold not happen). The correct updated time helps fire the lock.
Comment #11
hchonovReply to #9
I am glad to hear this :). We already have a failing test demonstrating this in a similar manner.
Yes, I confirm that the issue here is for properly updating the changed timestamp when an entity is being updated in a presave entity hook.
I think that the steps you've performed are enough as they are testing exactly the issue.
Comment #12
charlotte.b commented@matsbla before applying the patch I get a lock with the message (The content has either been modified by another user, or you have already submitted modifications. As a result, your changes cannot be saved.) on the form I press save on last. If the first form has an updated image, then this image is updated for all translations, however only the first translation has the updated time set.
After applying the patch if I repeat the test then I get no lock error if I hit save on the translation with a new image updated first, then quickly hit save on the translation with no updated image. The image doesn't get updated (I would expect it to contain the new image?), but the updated time is set correctly.
It seems there are two different things going on here, @matsbla and @hchonov please can you both confirm if this is what's expected :)
Comment #13
hchonov@charlotte.b this looks like a race condition to me. I didn't understand which translations have which image after the save? Do all translations have the image updated on the first translation saved or all have the original image with which the second translation is being saved? I would expect that all translations have the original image, which will confirm the race condition, as in this case because of doing this quickly the second submission retrieves the initial entity instead the one saved in the other tab as the submission from the first tab isn't completely ready yet and therefore the validation could not fail and prevent saving.
Could you please repeat these steps, but not "quickly"? :) I mean wait until the redirect on the first tab has occurred and then save the other translation with the untouched image.
Comment #14
charlotte.b commented@hchonov I just repeated, waiting until the first saves and get the same result, the original image is saved for both translations.
Comment #15
hchonov@charlotte.b, thank you for your testing. I've reproduced your steps and found out that the problem that we have here is the one from the other issue - #2837022: Concurrently editing two translations of a node may result in data loss for non-translatable fields. So if you apply the latest patch from the other issue together with the patch from the current issue you'll be prevented from saving the second translation. Could you please confirm that if none of the patches is applied you will be allowed to save the second translation and if both patches are applied you'll be prevented from saving the second translation with the message
'The content has either been modified by another user, or you have already submitted modifications. As a result, your changes cannot be saved.'?P.S. these are still two independent issues and therefore the current one should not be postponed on the other one.
Comment #16
charlotte.b commented@hchonov thank you, I applied the patch from #2837022: Concurrently editing two translations of a node may result in data loss for non-translatable fields and it now successfully locks, applies the first update and updates the timestamps as expected.
I think the other issue was confusing things, but the steps to replicate this issue in isolation are:
- Activated language and content translation modules
- Added a second language (French)
- Go to content language settings page
- Select Content and select a content type
- Hit save configuration
- Go to the content type configured above and add a new image field
- Edit the field and Deselect file under translatable elements
- Created a new content page with an image for English
- Added a translation for French
- Edit a translation and update the image
- The image is successfully saved for all translations but the timestamp is only updated for the translation that was edited
Apply the patch 2912318-3.patch and retest, the timestamp is updated for all translations
Comment #17
hchonovAfter I've looked over the patch again I've noticed two things that had to be cleared as well:
1. Iterating over the fields and finding the changed field and then iterating over each entity translation and explicitly retrieving the changed field should be more efficient then iterating over the entity translations and then each time over the fields.
2. In order to be consistent with
\Drupal\Core\Entity\ContentEntityStorageBase::invokeFieldMethod()we have to invoke the field presave method first on the current translation and then on any other translations.The patch should still be green, but since I've changed the patch I am putting the status back again to "Needs review".
Comment #18
charlotte.b commentedRetested with patch 2912318-17_0.patch and all works as expected :)
Comment #19
catchhmm I think I prefer the approach in #2617628: ChangedItem will not detect a change if another field item changes its value in field::preSave which ensures that ChangedItem runs last - that seems more obvious than having it run twice?
Comment #20
hchonov@catch, it runs as the last item during field presave, but still does not cover the changes made in hook_entity_presave, because the hook is invoked after the field presave. Ideally we would let it run only once, but it will be a BC break to remove the field presave method as currently if changes in fields are found during field presave the updated changed timestamp will be available in the hook_entity_presave. In order not to break BC we have to support -
This leaves us only one option and it is to check for changes twice. But this will be not that bad once #2863818: Fix notification issues in the entity/field on-change system is fixed as then it will be really easy to detect if something has changed :).
Comment #21
matsbla commentedI tested this patch again now. It solves the problem if two translations are edited concurrently, however the lock is not working if a new translations is added concurrently as another translation is edited (and change the non-translatable image file).
If the new translation is added concurrently, it then reverts the changes made back to the old image file. It still keeps the new image text, so in fact the image text for the new image is now set to the old image file.
Comment #24
matsbla commentedComment #33
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".
Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.