hasTranslationChanges on multiple languages outside of saving process is costly

Open on Drupal.org →

Created on 16 October 2018, about 6 years ago

Updated 2 February 2023, almost 2 years ago

Problem/Motivation

Comment from @maximpodorov in #2833049-16: ContentEntityBase::hasTranslationChanges will compare a forward revision with the default one instead of the newest forward revision → :

BTW, if ContentEntityBase::hasTranslationChanges() is called several times (e.g. for different languages) and (see its code) $this->original is not set, $original is loaded from the DB every time. I suggest to cache $original in the entity. This can be related to #2961399: Support parallel translation forward revisions on untranslatable fields

This is a problem only if hasTranslationChanges is called outside of a saving process, which currently is the case in two constraint validators:

\Drupal\Core\Entity\Plugin\Validation\Constraint\EntityUntranslatableFieldsConstraintValidator::validate()
\Drupal\content_translation\Plugin\Validation\Constraint\ContentTranslationSynchronizedFieldsConstraintValidator::validate()

Proposed resolution

Set the original entity before calling hasTranslationChanges over multiple entity translations?
Pass the original entity as an argument to hasTranslationChanges?

Remaining tasks

User interface changes

API changes

Data model changes

🐛 Bug report

Status

Needs work

Version

10.1 ✨

Component

Last updated about 17 hours ago

Maintained by
🇬🇧United Kingdom @catch
🇨🇭Switzerland @berdir
🇩🇪Germany @hchonov

Created by

🇩🇪Germany hchonov 🇪🇺🇩🇪🇧🇬

Live updates comments and jobs are added and updated live.

Needs Review Queue Initiative
Used to track the progress of issues reviewed by the Drupal Needs Review Queue Initiative.

Needs tests
The change is currently missing an automated test that fails when run with the original code, and succeeds when the bug has been fixed.

Incomplete comments

Sign in to follow issues

Merge Requests

!9587hasTranslationChanges on multiple languages outside of saving process is costly
Open
🇨🇦Canada pavlosdan
updated 3 months ago

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

Comment almost 2 years ago →
🇺🇸United States smustgrave
This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request → as a guide.

The MR will need to be updated for 10.1
Also will need test case to show the issue.

Did not test fix.
First commit to issue fork.
Merge request !9587Resolve #3007031 "D11 hastranslationchanges on multiple" → (Open) created by pavlosdan
Pipeline finished with Failed
3 months ago
Total: 271s
#290844
Comment 3 months ago →
🇨🇦Canada pavlosdan
Rebased MR against 11.x, added test, and fixed some minor coding standard issues gitlab caught.
Pipeline finished with Success
3 months ago
Total: 654s
#290849
Comment 3 months ago →
🇺🇸United States smustgrave
smustgrave → changed the visibility of the branch 3007031-hastranslationchanges-on-multiple to hidden.
Comment 3 months ago →
🇺🇸United States smustgrave
The test-only job appears to be passing when it should be failing. So probably need to be tweaked some.

Thanks for picking up
Comment 3 months ago →
🇨🇦Canada pavlosdan
Ah yes, didn't think about that. The test just shows that it's more performant if the original entity is set in $entity->original before calling hasTranslationChanges().

I ll adjust to test the validator changes too.

contrib.social Blog FAQ Discussions

Production build 0.71.5 2024