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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

Status: Active » Needs review
Issue tags: -Needs tests
FileSize
3.05 KB
1.48 KB

See attached.

xjm’s picture

fubhy’s picture

I opened a related issue recently as well: #1887058: Remove usages of 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.

andypost’s picture

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

tim.plunkett’s picture

Assigned: tim.plunkett » Unassigned

This is done.

andypost’s picture

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

andypost’s picture

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

andypost’s picture

FileSize
4.6 KB

Chasing HEAD

dawehner’s picture

+++ 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.

andypost’s picture

Scope of the issue to move implementation to parent class

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

dawehner’s picture

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.

alexpott’s picture

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

alexpott’s picture

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.

dawehner’s picture

Status: Needs work » Needs review
FileSize
4.67 KB
1.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.

andypost’s picture

Status: Needs review » Reviewed & tested by the community

Nice catch

alexpott’s picture

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
andypost’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
4.71 KB

re-roll

tim.plunkett’s picture

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

alexpott’s picture

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.

Anonymous’s picture

Issue summary: View changes

updated summary