It's a follow-up from #1781372-113: Add an API for listing (configuration) entities

Introduced API uses sorting of config entities which throws notice when weight of items are equal

More background from #1552396-72: Convert vocabularies into configuration

Thats caused by https://bugs.php.net/bug.php?id=50688 and language_manager that is not initialized in drupal_container()

Related issues
#1893442: Move BlockStorageController::loadByProperties() into ConfigStorageController
#1981418: Drop taxonomy_vocabulary_sort() - we still have @todo in code pointing to this issue

Files: 
CommentFileSizeAuthor
#24 1799600-24.patch5.96 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 56,207 pass(es). View
#22 interdiff.txt1.18 KBandypost
#22 1799600-sorttest-22.patch5.95 KBandypost
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1799600-sorttest-22.patch. Unable to apply patch. See the log in the details link for more information. View
#18 1799600-config-list-test-18.patch5.82 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 55,401 pass(es). View
#17 interdiff.txt2.33 KBandypost
#17 1799600-config-list-test-17.patch5.78 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 54,045 pass(es). View
#10 1799600-config-list-test-10.patch5.39 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 50,123 pass(es). View
#8 1799600-config-list-test-8.patch4.89 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 48,335 pass(es), 2 fail(s), and 0 exception(s). View
#4 1799600-config-list-test-4.patch4.81 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 42,186 pass(es), 0 fail(s), and 1 exception(s). View
#1 1799600-config-list-test-1.patch4.98 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 41,981 pass(es), 0 fail(s), and 1 exception(s). View

Comments

andypost’s picture

Status: Active » Needs review
FileSize
4.98 KB
FAILED: [[SimpleTest]]: [MySQL] 41,981 pass(es), 0 fail(s), and 1 exception(s). View

Just added a weight for test config entity and test that shows exception

Status: Needs review » Needs work

The last submitted patch, 1799600-config-list-test-1.patch, failed testing.

tim.plunkett’s picture

Tagging.

andypost’s picture

Title: Fix sorting of configuration entities » Add test of sorting for configuration entities
FileSize
4.81 KB
FAILED: [[SimpleTest]]: [MySQL] 42,186 pass(es), 0 fail(s), and 1 exception(s). View

Here's patch with test, suppose the issue was fixed with sorting removal for keys

sun’s picture

Status: Needs work » Needs review
+++ b/core/modules/config/tests/config_test/lib/Drupal/config_test/ConfigTest.php
@@ -42,4 +42,10 @@ class ConfigTest extends ConfigEntityBase {
+  /**
+   * The weight of the configuration entity in relation to others.
+   *
+   * @var integer
+   */
+  public $weight = 0;

Not all config entities need a weight - this property needs to go into individual config entity classes.

Status: Needs review » Needs work

The last submitted patch, 1799600-config-list-test-4.patch, failed testing.

sun’s picture

Status: Needs work » Postponed (maintainer needs more info)

Sorry, my comment in #5 was completely off — I misinterpreted the affected files.

That said, I still do not understand what exact bug this issue tries to fix. The information provided in this issue so far does not clarify either. The issue summary should be updated to precisely state what's wrong and under which circumstances a PHP notice is triggered.

andypost’s picture

Status: Postponed (maintainer needs more info) » Needs review
FileSize
4.89 KB
FAILED: [[SimpleTest]]: [MySQL] 48,335 pass(es), 2 fail(s), and 0 exception(s). View

Here's a re-roll of test.

The purpose of the patch to show that sorting/comparison of 2 config entities throws warning, Proper fix probably depends on php version

Status: Needs review » Needs work

The last submitted patch, 1799600-config-list-test-8.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
FileSize
5.39 KB
PASSED: [[SimpleTest]]: [MySQL] 50,123 pass(es). View

It looks like now sorting is no more issue, test passed locally. But I think it's better to include this test because config entity has ConfigEntityBase::sort() method that is not covered by tests yet, the same applies to ConfigEntityListController::load which calls sort()

andyceo’s picture

Status: Needs review » Needs work

The last submitted patch, 1799600-config-list-test-10.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
Issue tags: +Configuration system, +Configurables

#10: 1799600-config-list-test-10.patch queued for re-testing.

tim.plunkett’s picture

Component: configuration system » configuration entity system
aspilicious’s picture

#10: 1799600-config-list-test-10.patch queued for re-testing.

damiankloip’s picture

Status: Needs review » Needs work

Always a big fan of more test coverage :)

+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigEntityListTest.phpundefined
@@ -88,6 +88,22 @@ function testList() {
+    $entity = entity_create('config_test', array(
+      'id' => 'alpha',
+      'label' => 'Alpha',
+      'weight' => 1,
+    ));
+    $entity->save();
+    $entity = entity_create('config_test', array(
+      'id' => 'omega',
+      'label' => 'Omega',
+      'weight' => 1,
+    ));
+    $entity->save();
+    $list = $controller->load();
+    $entity = end($list);

Would this be better with an assertIdentical($actual, $expected) to test the array order (maybe just test array keys of the actual against an expected order array)? Maybe we should add another entity to the mix too that has a lower weight than these two, then we are not just covering equal weights.

+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigEntityListTest.phpundefined
@@ -88,6 +88,22 @@ function testList() {
+    $this->assertEqual($entity->label, 'Omega');

Might as well call the label() method here?

+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigEntityListTest.phpundefined
@@ -142,8 +162,8 @@ function testListUI() {
+    $this->assertLink('Edit', 1);
+    $this->clickLink('Edit', 1);

Not really a fan of using the index here, maybe assertLinkByHref instead? Same goes for the changes to the delete assertions.

+++ b/core/modules/config/tests/config_test/lib/Drupal/config_test/Plugin/Core/Entity/ConfigTest.phpundefined
@@ -56,6 +56,13 @@ class ConfigTest extends ConfigEntityBase {
+   * The weight of the configuration entity in relation to others.

Is this in relation to others? only when we list it, otherwise I would just call this 'The weight of the configuration entity'. Do we just use @var int too?

andypost’s picture

Status: Needs work » Needs review
FileSize
5.78 KB
PASSED: [[SimpleTest]]: [MySQL] 54,045 pass(es). View
2.33 KB

Changes with #16

andypost’s picture

FileSize
5.82 KB
PASSED: [[SimpleTest]]: [MySQL] 55,401 pass(es). View

re-roll

Status: Needs review » Needs work
Issue tags: -Configuration system, -Configurables

The last submitted patch, 1799600-config-list-test-18.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
Issue tags: +Configuration system, +Configurables

#18: 1799600-config-list-test-18.patch queued for re-testing.

dawehner’s picture

This is looking great so far.

+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigEntityListTest.phpundefined
@@ -98,6 +98,27 @@ function testList() {
+    $entity = entity_create('config_test', array(
...
+    $entity = entity_create('config_test', array(
...
+    $entity = entity_create('config_test', array(

Do you know whether there should be used the storage controller directly to create the entities in the test? This would make it easier to convert it to unit tests at some point.

+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigEntityListTest.phpundefined
@@ -98,6 +98,27 @@ function testList() {
+    $list = $controller->load();
+    $this->assertIdentical(array_keys($list), array('beta', 'dotted.default', 'alpha', 'omega'));

It would be great to refer somehow to ConfigEntityBase::sort(), as that's what is tested here. That's just easier for people to figure out what's done here. What do you think about a custom assertion message?

+++ b/core/modules/config/tests/config_test/lib/Drupal/config_test/ConfigTestFormController.phpundefined
@@ -35,6 +35,11 @@ public function form(array $form, array &$form_state, EntityInterface $entity) {
+      '#title' => 'Weight',

I know the other form elements don't mark the title as translatable, but shouldn't that be done?

andypost’s picture

FileSize
5.95 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1799600-sorttest-22.patch. Unable to apply patch. See the log in the details link for more information. View
1.18 KB

1) fixed
2) We are testing that load() returns in right order
3) we never use t() in testing modules

Status: Needs review » Needs work

The last submitted patch, 1799600-sorttest-22.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
5.96 KB
PASSED: [[SimpleTest]]: [MySQL] 56,207 pass(es). View

Rerolled.

dawehner’s picture

ignore comment.

andypost’s picture

@dawehner I cant rtbc my patch :( bot is happy

damiankloip’s picture

Status: Needs review » Reviewed & tested by the community

I think I can... I only rerolled it.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed cbb579b and pushed to 8.x. Thanks!

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary.