Problem/Motivation

Currently sorting on load happens only for blocks but taxonomy and contact categories also needs this

Proposed resolution

  • Add implementation ConfigStorageController::loadByProperties()
  • Add tests
  • Revert taxonomy tests

Commit the change

#1799600: Add test of sorting for configuration entities
#1981418: Drop taxonomy_vocabulary_sort() this depends on both issues

Original report by @tim.plunkett

And it needs some unit tests. Working on this.

Files: 
CommentFileSizeAuthor
#17 1893442-props-17.patch4.71 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 55,860 pass(es).
[ View ]
#14 interdiff.txt1.1 KBdawehner
#14 drupal-1893442-14.patch4.67 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 55,517 pass(es).
[ View ]
#8 1893442-load-8.patch4.6 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 55,437 pass(es).
[ View ]
#6 1893442-interdiff-6.txt1.46 KBandypost
#6 1893442-load-6.patch4.51 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 49,225 pass(es).
[ View ]
#1 configentity-1893442-1-FAIL.patch1.48 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 50,793 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#1 configentity-1893442-1-PASS.patch3.05 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 50,818 pass(es).
[ View ]

Comments

Status:Active» Needs review
Issue tags:-Needs tests
StatusFileSize
new3.05 KB
PASSED: [[SimpleTest]]: [MySQL] 50,818 pass(es).
[ View ]
new1.48 KB
FAILED: [[SimpleTest]]: [MySQL] 50,793 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

See attached.

I opened a related issue recently as well: #1887058: Nuke entity_load_multiple_by_properties() and EntityStorageController::loadByProperties(). I was toying with the idea of removing it altogether... entity_load_multiple_by_properties() as well as StorageController::loadByProperties(). Once that EFQ patch lands they will be mere wrappers for invoking EFQ. And we really should stop causing endless trails of function / method calls / forwards for no good reason.

There was a test in taxonomy vocabularies for this method, it was commented/removed in #1552396-71: Convert vocabularies into configuration

Assigned:tim.plunkett» Unassigned

This is done.

StatusFileSize
new4.51 KB
PASSED: [[SimpleTest]]: [MySQL] 49,225 pass(es).
[ View ]
new1.46 KB

+1 to RTBC
Patch just restores removed tests as pointed in #4

#6: 1893442-load-6.patch queued for re-testing.

StatusFileSize
new4.6 KB
PASSED: [[SimpleTest]]: [MySQL] 55,437 pass(es).
[ View ]

Chasing HEAD

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigStorageController.phpundefined
@@ -169,7 +169,13 @@ public function deleteRevision($revision_id) {
+    $entities = $this->load();
+    foreach ($values as $key => $value) {
+      $entities = array_filter($entities, function($entity) use ($key, $value) {
+        return $value === $entity->get($key);
+      });
+    }
+    return $entities;

Don't we have EQ for config entities as well? Sure it's doing basically the same, but I guess we could use it here.

Scope of the issue to move implementation to parent class

@dawehner for #9 I filed follow-up #1981516: Refactor ConfigStorageController to use entity query

Status:Needs review» Reviewed & tested by the community

I don't think it's a problem that we have both tests for vocabulary and config entity in general.

#8: 1893442-load-8.patch queued for re-testing.

Status:Reviewed & tested by the community» Needs work

+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigEntityUnitTest.phpundefined
@@ -43,6 +43,30 @@ public function testStorageControllerMethods() {
+    $style = $this->randomName();
...
+      'style' => $this->randomName(),

It is possible for this test to generate a random fail as randomName() could return the same string. We have to ensure that the style value is different... i.e. set different lengths.

Status:Needs work» Needs review
StatusFileSize
new4.67 KB
PASSED: [[SimpleTest]]: [MySQL] 55,517 pass(es).
[ View ]
new1.1 KB

Here is a small fix for that, but just by looking at the test I wouldn't assume that you actually need the ID here.

Status:Needs review» Reviewed & tested by the community

Nice catch

Status:Reviewed & tested by the community» Needs work

Needs a re-roll...

curl https://drupal.org/files/drupal-1893442-14.patch | git a
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  4784  100  4784    0     0   3326      0  0:00:01  0:00:01 --:--:--  6336
error: patch failed: core/modules/block/lib/Drupal/block/BlockStorageController.php:40
error: core/modules/block/lib/Drupal/block/BlockStorageController.php: patch does not apply

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new4.71 KB
PASSED: [[SimpleTest]]: [MySQL] 55,860 pass(es).
[ View ]

re-roll

#17: 1893442-props-17.patch queued for re-testing.

Status:Reviewed & tested by the community» Fixed

Committed 80da26f and pushed to 8.x. Thanks!

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

Issue summary:View changes

updated summary