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.

Files: 
CommentFileSizeAuthor
#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 ]

Comments

StatusFileSize
new1.57 KB
PASSED: [[SimpleTest]]: [MySQL] 39,333 pass(es).
[ View ]

first patch

StatusFileSize
new849 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

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

tagging

Issue tags:+lsd-csi

add lsd tag

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.

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.

Status:Needs work» Needs review
StatusFileSize
new1.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.

Title:in the GenericEntityController::buildQuery revision_id can not be an arrayLoad 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.

StatusFileSize
new1003 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.

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.

StatusFileSize
new13.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.

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.

Status:Needs work» Needs review
StatusFileSize
new12.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.

Issue tags:+Needs backport to 7.x

Needs backport to 7.x

Issue summary:View changes

filled out desc

#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)