- 🇺🇸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.
Did not test.
This will need a test case to show the issue.
- last update
10 months ago Custom Commands Failed - last update
10 months ago Custom Commands Failed - last update
10 months ago Custom Commands Failed - last update
10 months ago Custom Commands Failed - 🇸🇪Sweden TwoD Sweden
This patch modifies one of the existing revisioning test classes to include changes to a multi-value field (stored in a dedicated table), which is required for the SQL content entity storage implementation to run the code affected by the described issue.
The use case presented here aims to cut down on the number of required revisions to be created, especially when no fields are actually modified. I have the need for the same use case as the number of revisions created for minor changes can get excessive, and this would be a simple way to avoid some of those.
The modified test runs a sequence of changes to an entity, creating a new revision each time, to check that certain types of field modifications across translations are either prevented through validation errors or correctly stored. This is similar to having the Content Moderation module active for the entity as it forces revisions to be created for every modification performed via the entity form, or the separate publish-only form.
The changes add parameters for preventing the new multi-value field from being modified, and preventing a new revision from being created.
ThecreateRevision()
method is still invoked to copy how the entity form sets up the entity, and if no new revision is desired the test simply calls$entity->setNewRevision(FALSE)
. This also preserves the setup of$entity->original
so field modifications are accurately detectable.The test data provider adds two steps which first create a new pending Italian revision with changes to all the fields, and then turns that latest revision into the new default revision without any other changes.
This triggers the observed behavior in theSqlContentEntityStorage
class where it skips writing the latest revision's field values into the main table for the entity, leaving them only in the field's revision table, because it does not detect an actual change.
This does not happen for the label field or the untranslatable field as those are handled by the shared table method, which does not have the same "optimization".Note, I added the new multi-value field to the test entity for simplicity, but if the slight additional overhead is considered too much for other tests using the same entity it could be added in the test class itself.
- last update
10 months ago 30,646 pass, 5 fail - last update
10 months ago 30,646 pass, 5 fail - last update
10 months ago 30,645 pass, 7 fail - last update
10 months ago 30,640 pass, 7 fail - 🇸🇪Sweden TwoD Sweden
It was easier just to use the new field in the modified test class.
- last update
10 months ago Custom Commands Failed - last update
10 months ago Custom Commands Failed - last update
10 months ago Custom Commands Failed - last update
10 months ago Custom Commands Failed - last update
10 months ago 30,663 pass, 1 fail - last update
10 months ago 30,663 pass, 1 fail - last update
10 months ago 30,656 pass, 5 fail - last update
10 months ago 30,657 pass, 3 fail - Status changed to Needs review
10 months ago 3:40am 27 November 2023 - 🇸🇪Sweden TwoD Sweden
It's getting really late so I'm just pushed up a new fix based on what @Nixou did.
I noticed the query was very similar to what
getLatestTranslationAffectedRevisionId()
already does, but it also handles translations, which weren't always covered with the fix in #6.I also had to wrap the new condition in a try/catch because of what happens in
FieldableEntityDefinitionUpdateTest
. The entity type is in the process of being updated to say it's revisionable, but the tables don't seem to exist (yet?). Maybe the try/catch is hiding another bug, but I don't have time to investigate that further right now.
Could use an extra pair of eyes if anyone has the time, so marking NR. - 🇺🇸United States smustgrave
Think this is something that will need sub maintainer to take a look at.
- Status changed to Needs work
7 months ago 10:31am 22 February 2024 The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.