Problem/Motivation

The Workspace module (#2784921: Add Workspaces experimental module) will be making heavy use of entity revisions by swapping each loaded entity with the revision assigned to a specific workspace. This will add one database query for each entity that is swapped, because we only have a "single load" method for revisions.

Proposed resolution

Add a loadMultipleRevisions() method to the entity storage.

Remaining tasks

Review.

User interface changes

Nope.

API changes

API addition: a new loadMultipleRevisions() method is added to \Drupal\Core\Entity\EntityStorageInterface and all the storages provide a base implementation for it.

Data model changes

Nope.

Original issue summary

The ::load method always for any number of conditions, but if a condition match the revision id field, it is treated special and converted to the $revision_id variable. This is then passed off to buildQuery. This is then used to join in the revision table.

The problem is that the current way allows for only a single revision id to be pass. I feel this is contrary to the fact that the load method is a mulitple entity load method. If we allow an array for revision_id, in the buildQuery method, this would allow for a multiple load, when load is called with many revision_ids.

Our particular case, is that we are implementing a wrapper controller class, that first looks up what revision should be queried, before passing it off to the particular entities load. and we need to be able to do this for multiple entities at one time.

CommentFileSizeAuthor
#51 interdiff-51.txt6.74 KBamateescu
#51 1730874-51.patch26.46 KBamateescu
#46 interdiff-46.txt3.31 KBamateescu
#46 1730874-46.patch26.01 KBamateescu
#44 interdiff-43.txt4.4 KBamateescu
#44 1730874-43.patch25.35 KBamateescu
#38 interdiff-38.txt1.44 KBamateescu
#38 1730874-38.patch25.21 KBamateescu
#33 interdiff-33.txt816 bytesamateescu
#33 1730874-33.patch24.62 KBamateescu
#31 interdiff-31.txt2.35 KBamateescu
#31 1730874-31.patch24.61 KBamateescu
#29 interdiff-29.txt2.54 KBamateescu
#29 1730874-29.patch22.98 KBamateescu
#26 interdiff.txt1.2 KBamateescu
#26 1730874-26.patch22.97 KBamateescu
#25 1730874-24.patch23.18 KBamateescu
#16 drupal-entity_multiple_revisions-1730874-16.patch12.9 KBindytechcook
FAILED: [[SimpleTest]]: [MySQL] 41,542 pass(es), 314 fail(s), and 6,693 exception(s). View
#13 drupal-entity_multiple_revisions-1730874-13-test-only.patch13.59 KBindytechcook
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-entity_multiple_revisions-1730874-13-test-only.patch. Unable to apply patch. See the log in the details link for more information. View
#10 drupal-entity_multiple_revisions-1730874-10-test-only.patch1003 bytesdisasm
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-entity_multiple_revisions-1730874-10-test-only.patch. Unable to apply patch. See the log in the details link for more information. View
#8 1730874_1.patch1.01 KBgoogletorp
PASSED: [[SimpleTest]]: [MySQL] 40,569 pass(es). View
#2 1730874.patch849 bytese2thex
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1730874_0.patch. Unable to apply patch. See the log in the details link for more information. View
#1 1730874.patch1.57 KBe2thex
PASSED: [[SimpleTest]]: [MySQL] 39,333 pass(es). View
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

e2thex’s picture

FileSize
1.57 KB
PASSED: [[SimpleTest]]: [MySQL] 39,333 pass(es). View

first patch

e2thex’s picture

FileSize
849 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1730874_0.patch. Unable to apply patch. See the log in the details link for more information. View

Here is a clean version

e2thex’s picture

Status: Active » Needs review
Issue tags: +Entity system

tagging

e2thex’s picture

Issue tags: +lsd-csi

add lsd tag

Berdir’s picture

Version: 7.15 » 8.x-dev

New features need to be added to 8.x first and then backported.

Note that #1184272: Remove deprecated $conditions support from entity controller completely revamps how revisions are loaded but it doesn't allow for multiple revisions either.

Berdir’s picture

Issue tags: -Entity system, -lsd-csi

#2: 1730874.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Entity system, +lsd-csi

The last submitted patch, 1730874.patch, failed testing.

googletorp’s picture

Status: Needs work » Needs review
FileSize
1.01 KB
PASSED: [[SimpleTest]]: [MySQL] 40,569 pass(es). View

It seems like there is something wrong with the patch format from #2, so tried to remake the patch.

Berdir’s picture

Title: in the GenericEntityController::buildQuery revision_id can not be an array » Load multiple revisions at once
Status: Needs review » Needs work
Issue tags: +Needs tests

It's not that easy anymore. There's now loadRevision() on the controller, which returns a single revision explicitly.

I was considering making it a multiple operation, but decided against that as the required change would imho become too big for that other issue.

So, what needs to happen here to make this possible:

- loadRevision() should accept an array of revision id's, with a type hint.
- entity_revision_load_multiple() should be added, entity_revision_load() should become a wrapper of that function
- The attachLoad() stuff probably needs to be refactored, the revision argument removed, we should instead check each entity if it's the current revision (isCurrentRevision()), create two arrays of entities and call the field attach function accordingly.
- And of course, this needs tests.

disasm’s picture

FileSize
1003 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-entity_multiple_revisions-1730874-10-test-only.patch. Unable to apply patch. See the log in the details link for more information. View

If I understand this issue correctly, the entity field query should take an array of revision id's. As such, I've created a simple test that passes an array of 2 revisions to EntityFieldQuery and expects an array of 2 entities to be returned. If I understand Berdir correctly, the current attempt at writing patches falls short of implementing it, so I'm not including the original in my patch, just a test only.

disasm’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, drupal-entity_multiple_revisions-1730874-10-test-only.patch, failed testing.

indytechcook’s picture

FileSize
13.59 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-entity_multiple_revisions-1730874-13-test-only.patch. Unable to apply patch. See the log in the details link for more information. View

This is a start but not working yet.

Berdir’s picture

Status: Needs work » Needs review
+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageController.phpundefined
@@ -295,13 +295,13 @@ protected function buildPropertyQuery(EntityFieldQuery $entity_query, array $val
+    if ($revision_ids) {
+      $query->join($this->revisionTable, 'revision', "revision.{$this->idKey} = base.{$this->idKey} AND revision.{$this->revisionKey} IN :revisionIds", array(':revisionIds' => $revision_ids));

Should be "IN (:revision_ids)"

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageController.phpundefined
@@ -355,14 +355,16 @@ protected function buildQuery($ids, $revision_id = FALSE) {
+      foreach ($queried_entities as $entity) {
+        if ($entity->isDefaultRevision()) {
+          field_attach_load($this->entityType, $queried_entities);
+        }
+        else {
+          field_attach_load_revision($this->entityType, $queried_entities);

You're looping over the entities, check the one you loaded and then call field_attach_load() for all entities. Which means that it will be executed multiple times.

What you probably should do is loop over them, create two arrays, one that is revision-specific and one that isn't. Then call the correct field attach function for each if the array is not empty.

In the long term, we should probably get rid of the second function and deal with this within field_attach_load().

Remember to set issues to needs review if you upload a patch. If you don't want to have tests run on it, you should append the -do-not-test.patch suffix. The problem with keeping it at needs work is that all patches will be sent to the testbot once it's set to needs review so you will have to wait longer once you're actually interested in the results :)

Status: Needs review » Needs work

The last submitted patch, drupal-entity_multiple_revisions-1730874-13-test-only.patch, failed testing.

indytechcook’s picture

Status: Needs work » Needs review
FileSize
12.9 KB
FAILED: [[SimpleTest]]: [MySQL] 41,542 pass(es), 314 fail(s), and 6,693 exception(s). View

Thanks @Berdir.

I've updated the patch. I wasn't ready to have it reviewed but having the tests run by the bot instead of locally is a good idea. Let's see where we are at. I'm sure tests will need to be updated and the test from #10 needs to be added

Status: Needs review » Needs work

The last submitted patch, drupal-entity_multiple_revisions-1730874-16.patch, failed testing.

attiks’s picture

Issue tags: +Needs backport to 7.x

Needs backport to 7.x

attiks’s picture

Issue summary: View changes

filled out desc

Berdir’s picture

#2095283: Remove non-storage logic from the storage controllers removes the weird $revision_id argument from the attach function, would however still only support loading either revisions or the latest version of all entities, although that might actually be fine.

If someone wants to work on this then this should keep the loadRevision() method and instead add a new loadRevisions or loadRevisionMultiple() method (kind of weird here, but it's a common pattern, so not sure)

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

amateescu’s picture

Title: Load multiple revisions at once » Add support for loading multiple revisions at once
Version: 8.4.x-dev » 8.5.x-dev
Category: Feature request » Task
Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs tests, -Entity system, -lsd-csi, - +Workflow Initiative
Related issues: +#2912562: Add support for loading multiple entity revisions at once

Here's a fresh patch, written from scratch in #2912562: Add support for loading multiple entity revisions at once so there's no interdiff. Includes test coverage and everything, so that tag is no longer needed.

amateescu’s picture

Oops, forgot to attach the patch :/

amateescu’s picture

I just looked at the original patches from this issue and I like this hunk more than what I had.

amateescu’s picture

Any chance someone wants to review this patch? It would be a big performance improvement for #2784921: Add Workspaces experimental module.

hchonov’s picture

@amateescu, it looks good and is pretty straightforward change.

I've encountered just some minor nits ...

  1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
    @@ -597,22 +608,24 @@ protected function populateAffectedRevisionTranslations(ContentEntityInterface $
    +   * @param string $key
    

    What about naming the parameter $entity_key?

  2. +++ b/core/lib/Drupal/Core/Entity/EntityStorageInterface.php
    @@ -83,6 +83,17 @@ public function loadUnchanged($id);
    +   * Loads multiple revisions.
    

    I would use "multiple entity revisions".

  3. +++ b/core/lib/Drupal/Core/Entity/EntityStorageInterface.php
    @@ -83,6 +83,17 @@ public function loadUnchanged($id);
    +   *   The specified entity revisions or empty array if none found.
    

    "..or an empty array..."

  4. +++ b/core/lib/Drupal/Core/Entity/EntityStorageInterface.php
    @@ -83,6 +83,17 @@ public function loadUnchanged($id);
    +  public function loadMultipleRevisions(array $revision_ids);
    

    Could we move this method to ContentEntityStorageInterface? I know it is probably placed there because of the method ::loadRevision() living in this interface as well, but actually regular entities have nothing to do with revisions.

  5. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
    @@ -523,11 +525,15 @@ protected function mapFromStorageRecords(array $records, $load_from_revision = F
    +    $record_key = !$load_from_revision ? $this->idKey : $this->revisionKey;
    
    @@ -1167,21 +1181,22 @@ protected function loadFromDedicatedTables(array &$values, $load_from_revision)
    +        $value_key = !$load_from_revision ? $row->entity_id : $row->revision_id;
    

    instead of negating we could exchange the places in the ternary operator?

  6. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
    @@ -608,20 +614,29 @@ protected function loadFromSharedTables(array &$values, array &$translations) {
    +    // Support the case when a single revision ID is passed in.
    +    $load_single = !is_array($revision_ids);
    +    $revision_ids = (array) $revision_ids;
    ...
    -    return $revision;
    +    return $load_single ? (isset($revisions[$revision_ids]) ? $revisions[$revision_ids] : NULL) : $revisions;
    

    As this is a protected method we are allowed to enforce that the parameter is an array. Why do we have to support passing a single value?

amateescu’s picture

@hchonov, thanks for the review!

Re #28:

1, 2, and 3: Fixed.

4: IMO it's better to be consistent and offer the method at the same place where we have loadRevision(). We could open a followup if you want to move them out to a separate entity storage interface :)

5: I've used the negated condition because I wanted to be consistent with all the other similar checks that we have in that file..

6: That's because I wanted to provide full BC support for contrib/custom code which might be calling that method. However, after looking a bit more into it I noticed that there was a flaw in the return value, easy to fix though.

hchonov’s picture

4. I thought so :). I guess we'll do this in D9, so nothing to do or discuss here.

5. Ok.

6. Let's then trigger a deprecated error in case the method isn't called with an array e.g.:

if (!is_array($revision_ids)) {
  @trigger_error('Passing a single revision ID to "\Drupal\Core\Entity\ContentEntityStorageBase::doLoadRevisionFieldItems()" is deprecated in Drupal 8.5.x and will be removed before Drupal 9.0.x. An array of revision IDs should be given instead..', E_USER_DEPRECATED);
}
amateescu’s picture

Sure, let's do that :)

hchonov’s picture

Status: Needs review » Needs work
+++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
@@ -244,27 +244,38 @@ public function finalizePurge(FieldStorageDefinitionInterface $storage_definitio
+    // The hooks are invoked keyed by entity revision ID so we have to do one by
+    // one.
...
       $this->invokeStorageLoadHook($entities);
       $this->postLoad($entities);

+++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
@@ -607,20 +613,33 @@ protected function loadFromSharedTables(array &$values, array &$translations) {
-    // Build and execute the query.

I am sorry, but I've noticed now that the last part of the sentence doesn't sound that good - "do one by one.", but we'll have to rewrite anyway because of the next point.

+++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
@@ -244,27 +244,38 @@ public function finalizePurge(FieldStorageDefinitionInterface $storage_definitio
+    foreach ($revisions as $revision) {
       $entities = [$revision->id() => $revision];
       $this->invokeStorageLoadHook($entities);
       $this->postLoad($entities);
     }

Let's put the entity revisions in an array keyed by the entity ID and execute both ::invokeStorageLoadHook and ::postLoad with that array, as this way we'll have count($revisions) - 1 less calls to both methods, which means <code>count($revisions) - 1 less hook invocations and entity class postLoad invocations :). Otherwise we'll invoke all the methods for each entity revision one by one, which doesn't make sense as the executed methods expect an array of entities anyway. Needs work for this.

amateescu’s picture

Status: Needs work » Needs review
FileSize
24.62 KB
816 bytes

Let's put the entity revisions in an array keyed by the entity ID

We can't do that because the hook implementations expect a structure like this:

[
  '<entity_id_1>' => $entity_object_1,
  '<entity_id_2>' => $entity_object_2,
]

and with your proposal we would give them a structure like this:

[
  '<entity_id_1>' => [$entity_object_1_rev_1, $entity_object_1_rev_2],
  '<entity_id_2>' => [$entity_object_2_rev_3, $entity_object_2_rev_4],
]

That would break every hook_entity_load() implementation..

I've re-worded the comment, hope it's more clear now :)

hchonov’s picture

Oh ok, this is the case where we are loading multiple revisions for the same entity. But what if we are not? Could we build groups of entities for which to call the hooks together? What I mean based on your example would result into:

Group 1:
[
'<entity_id_1>' => $entity_object_1_rev_1,
'<entity_id_2>' => $entity_object_2_rev_3,
]

and

Group 2:
[
'<entity_id_1>' => $entity_object_1_rev_2,
'<entity_id_2>' => $entity_object_2_rev_4,
]

This will be still faster as in this case we'll execute the hook logic only 2 times instead of 4 times.

hchonov’s picture

I suggest something like this:

// The hooks are executed with an array of entities where the array key is
// the entity ID. As we could load multiple revisions for the same entity ID
// at once we have to build groups of entities where the same entity ID is
// present only once.
$entity_groups = [];
foreach ($revisions as $revision) {
  $id = $revision->id();
  foreach ($entity_groups as &$entity_group) {
    if (!isset($entity_group[$id])) {
      $entity_group[$id] = $revision;
      continue 2;
    }
  }
  // If no suitable group has been found then create a new one.
  $entity_groups[] = [$id => $revision];
}
// The hooks are invoked keyed by entity ID so we have invoke them for each
// revision.
foreach ($entity_groups as $entities) {
  $this->invokeStorageLoadHook($entities);
  $this->postLoad($entities);
}
amateescu’s picture

TBH I think that's an unnecessary optimization and it makes the revisions you receive in the hook implementation look a bit arbitrary. Consider this case:

Group 1:
[
'<entity_id_1>' => $entity_object_1_rev_1,
'<entity_id_2>' => $entity_object_2_rev_3,
]

Group 2:
[
'<entity_id_2>' => $entity_object_2_rev_4,
]

The hook implementations will receive two objects on the first call and just one on the second call simply because they are revisions of a different entity. That looks a bit awkward IMO but I'm interested to hear what others think about it.

hchonov’s picture

With your last example we'll still have one hook invocation less. But this is actually a rare use case.

I was thinking that this functionality makes perfect sense for the module entity_reference_revisions and the use case there is that you usually reference different entities, in which case this optmization makes perfect sense. I see the use case of loading different revisions for the same entity as the use case that will not occur that often like the other one.

Paragraphs relies on entity_reference_revisions and there this optimization makes perfekt sense as well.

amateescu’s picture

Right, the entity_reference_revisions use case is covered pretty well by your approach. Let's do it then :)

hchonov’s picture

Status: Needs review » Reviewed & tested by the community

Thank you! It looks better now and ready to go :)

larowlan’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs change record
  1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
    @@ -244,27 +244,53 @@ public function finalizePurge(FieldStorageDefinitionInterface $storage_definitio
    +      $id = $revision->id();
    ...
    +        if (!isset($entity_group[$id])) {
    +          $entity_group[$id] = $revision;
    ...
    +      $entity_groups[] = [$id => $revision];
    

    minor nit: but thing readability would be aided here if we named the variable revision_id rather than id, so it is clear what id we're keying it by

  2. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
    @@ -607,20 +613,33 @@ protected function loadFromSharedTables(array &$values, array &$translations) {
    +      @trigger_error('Passing a single revision ID to "\Drupal\Core\Entity\ContentEntityStorageBase::doLoadRevisionFieldItems()" is deprecated in Drupal 8.5.x and will be removed before Drupal 9.0.0. An array of revision IDs should be given instead.', E_USER_DEPRECATED);
    

    We need a change record and this needs to reference it

Berdir’s picture

Status: Needs review » Needs work

The implementation here seems fine. I'm a bit worried about the minor changes to the protected methods. As mentioned by @larowlan, we should have a change record that lists all those changes, in case someone has been overriding the methods. See also below.

  1. +++ b/core/lib/Drupal/Core/Entity/EntityStorageInterface.php
    @@ -83,6 +83,17 @@ public function loadUnchanged($id);
    +   *
    +   * @return \Drupal\Core\Entity\EntityInterface[]
    +   *   The specified entity revisions or an empty array if none found.
    

    Worth documenting explicitly that they will be keyed by revision ID?

  2. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
    @@ -607,20 +613,33 @@ protected function loadFromSharedTables(array &$values, array &$translations) {
    +    if (!is_array($revision_ids)) {
    +      @trigger_error('Passing a single revision ID to "\Drupal\Core\Entity\ContentEntityStorageBase::doLoadRevisionFieldItems()" is deprecated in Drupal 8.5.x and will be removed before Drupal 9.0.0. An array of revision IDs should be given instead.', E_USER_DEPRECATED);
    

    I see you're also added the deprecation exception for tests, we should possibly also have a follow-up to remove that, I think that's what's been suggested in #2607260: [meta] Core deprecation message introduction, contrib testing etc..

    Also, this deprecation message only covers one side of the equation, in case someone calls it with a single revision ID. but if there's an implementation expecting a single value, it would fail.

    The thing is, we have two options:

    1. What this patch does, make those slight changes to the arguments, which could break subclasses if they rely on those arguments.

    2. Add new methods, deprecate the old. But that would mean we would no longer call those methods (at least for loading revisions, maybe both). and that would break overriddes even more.

    Given that, I think those changes are acceptable (aka option 1), changing protected methods is allowed, even though most of those methods are specifically designed to be overridden. I think the most problematic one is buildQuery() because we actually add a new argument there?

hchonov’s picture

Re #40.1

If we rename it then to $entity_id as it is the ID of the entity and $revision_id usually is used to reference the revision ID of an entity.

Re #40.2 & #41:

Our BC policy states the following about protected methods:

Protected methods of a class should be assumed @internal and subject to change unless either the class or method itself are marked with @api. Drupal leaves most internal methods protected rather than private to allow for one-off customized subclasses when needed, but in most cases that "escape hatch" should not be relied upon indefinitely. If no alternative presents itself consider filing a feature request for a more directly supported approach.

The methods might be overridden, but still we've defined in our BC policy that they might change and we're not making promise to provide BC layer when they change. The latest patch contains a BC layer for SqlContentEntityStorage::doLoadRevisionFieldItems() even if it isn't required to.

Re #41 about buildQuery:

I think the most problematic one is buildQuery() because we actually add a new argument there?

Actually we don't add a new parameter to buildQuery but as of now require that it is an array instead of a single value exactly like the change in doLoadRevisionFieldItems. We could probably add there the same BC layer by converting a single value into an array and trigger deprecation notice exactly like in SqlContentEntityStorage::doLoadRevisionFieldItems().

I do understand that it is nice to add the BC layer whenever possible, but I am not sure that we have to do it even at places where we're not required to.

Berdir’s picture

> I think that's what's been suggested in

Looks like the latest approach there is to have an automatic detection based on the version specified in the message.

amateescu’s picture

Status: Needs work » Needs review
Issue tags: -Needs change record
FileSize
25.35 KB
4.4 KB

Fixed #40 and #41 and here's the change record: https://www.drupal.org/node/2924915

hchonov’s picture

@amateescu, what do you think about adding the same BC layer to buildQuery and mentioning the change to its parameter in the change record as well?

amateescu’s picture

hchonov’s picture

Status: Needs review » Reviewed & tested by the community

@amateescu, thank you for addressing all the remarks since #40.

catch’s picture

+++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
@@ -244,27 +244,53 @@ public function finalizePurge(FieldStorageDefinitionInterface $storage_definitio
+    $entity_groups = [];
+    foreach ($revisions as $revision) {
+      $entity_id = $revision->id();
+      foreach ($entity_groups as &$entity_group) {
+        if (!isset($entity_group[$entity_id])) {
+          $entity_group[$entity_id] = $revision;
+          continue 2;
+        }
+      }
+      // If no suitable group has been found then create a new one.
+      $entity_groups[] = [$entity_id => $revision];
+    }
+

It'd be nice to do without the continue 2 if we could, although also this is pretty compact so it's not too bad. What about the outer loop as an array_walk() or something? If the nested foreach is the best option that's OK, but didn't see it discussed.

I couldn't find much else to complain about.

hchonov’s picture

What about something like this:

$entity_groups = [];
$entity_group_mapping = []:
foreach ($revisions as $revision) {
  $entity_id = $revision->id();
  $entity_group_key = isset($entity_group_mapping[$entity_id]) ? $entity_group_mapping[$entity_id] + 1 : 0;
  $entity_group_mapping[$entity_id] = $entity_group_key;
  $entity_groups[$entity_group_key][$entity_id] = $revision;
}
catch’s picture

Status: Reviewed & tested by the community » Needs work

That's much better, CNW for that change.

+++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
@@ -244,27 +244,53 @@ public function finalizePurge(FieldStorageDefinitionInterface $storage_definitio
 
   /**
    * Actually loads revision field item values from the storage.
    *
-   * @param int|string $revision_id
-   *   The revision identifier.
+   * @param array $revision_ids
+   *   An array of revision identifiers.
    *
-   * @return \Drupal\Core\Entity\EntityInterface|null
-   *   The specified entity revision or NULL if not found.
+   * @return \Drupal\Core\Entity\EntityInterface[]
+   *   The specified entity revisions or an empty array if none are found.
    */
-  abstract protected function doLoadRevisionFieldItems($revision_id);
+  abstract protected function doLoadRevisionFieldItems($revision_ids);
 

Also I'm not sure about this change - it's an abstract method on a base class, so we're expecting subclasses to implement it. Should we be adding doLoadRevisionsFieldItems() and deprecating this method instead of changing the signature? This is a bit borderline with the bc policy tbh.

amateescu’s picture

Status: Needs work » Needs review
FileSize
26.46 KB
6.74 KB

@catch, sure thing, I don't mind adding a new method for that, it even allows a cleaner BC implementation :)

amateescu’s picture

If we're happy with the interdiff from #51, we need to update the change record to mention the new method.

catch’s picture

#51 looks good to me, thanks for adding the new method and it does seem a bit cleaner.

Berdir’s picture

Agreed, looks good to me as well.

amateescu’s picture

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityStorage.php
@@ -135,6 +135,13 @@ public function loadRevision($revision_id) {
+  public function loadMultipleRevisions(array $revision_ids) {
+    return [];
+  }

This is the part that concerns me the most tbh, so I opened an issue to discuss it better: #2926540: Split revision-handling methods to a separate entity storage interface

I also updated the CR.

hchonov’s picture

Status: Needs review » Reviewed & tested by the community

Nice. Let's get this in :).

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed 0b90895 and pushed to 8.5.x. Thanks!

  • catch committed 0b90895 on 8.5.x
    Issue #1730874 by amateescu, e2thex, indytechcook, disasm, googletorp,...
penyaskito’s picture

This commit made the lingotek tests fail. I'm still researching why, but maybe it introduced a BC issue.

penyaskito’s picture

OK, this is fixing an undesired bug at the same time, that somehow my tests were relying on:

Before this commit, entity_revision_load('node', NULL) would return the first node revision. After this commit, would return NULL which I would expect.

hchonov’s picture

@penyaskito, I guess everything is good then and there is nothing to worry about.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.