Problem/Motivation

The "weight" property name is hardcoded in the
\Drupal\Core\Config\Entity\ConfigEntityBase
\Drupal\Core\Config\Entity\DraggableListBuilder

The real name of the "weight" property is configurable in the @ConfigEntityType() annotation so the classes above should use that configuration.

Proposed resolution

Replace the hardcoded $entity->weight with $entity->$weight_key, where the $weight_key is the result of the $entity_type->getKey('weight')

CommentFileSizeAuthor
#12 interdiff.txt2.22 KBtim.plunkett
#12 configentitybase_and-2429169-12.patch8.53 KBtim.plunkett
#6 drupal-configentity_weight-2429169-6.patch6.28 KBsweetchuck
#4 drupal-configentity_weight-2429169-inter-1-4.patch.txt903 bytessweetchuck
#4 drupal-configentity_weight-2429169-4.patch4.56 KBsweetchuck
#1 drupal-configentity_weight-2429169-1.patch4.2 KBsweetchuck

Comments

sweetchuck’s picture

sweetchuck
Primary language Hungarian
Location Budapest
commented
Status: Active » Needs review
StatusFileSize
new drupal-configentity_weight-2429169-1.patch4.2 KB

Status: Needs review » Needs work

The last submitted patch, 1: drupal-configentity_weight-2429169-1.patch, failed testing.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Issue tags: +Needs tests 
+++ b/core/lib/Drupal/Core/Config/Entity/DraggableListBuilder.php
@@ -40,6 +40,13 @@
+   * HTML class to identify the weight input elements.
+   *
+   * @var string
+   */
+  protected $weightClass = 'weight';

I think this is redundant, and should just use weightKey.

This needs tests to expose the bug.

sweetchuck’s picture

sweetchuck
Primary language Hungarian
Location Budapest
commented
Status: Needs work » Needs review
StatusFileSize
new drupal-configentity_weight-2429169-4.patch4.56 KB
new drupal-configentity_weight-2429169-inter-1-4.patch.txt903 bytes

Clear the class name.
Without any test.

Status: Needs review » Needs work

The last submitted patch, 4: drupal-configentity_weight-2429169-4.patch, failed testing.

sweetchuck’s picture

sweetchuck
Primary language Hungarian
Location Budapest
commented
Status: Needs work » Needs review
StatusFileSize
new drupal-configentity_weight-2429169-6.patch6.28 KB

The existing tests are suppose that the entities are sorted by weight-label, but the config_test entity type has no defined "weight key".

Remaining task:
Write more test with a new config_weight_test entity type which has "weight" key.

Status: Needs review » Needs work

The last submitted patch, 6: drupal-configentity_weight-2429169-6.patch, failed testing.

sweetchuck’s picture

sweetchuck
Primary language Hungarian
Location Budapest
commented

The same problem with the "status" field.

\Drupal\Core\Config\Entity\ConfigEntityBase::setStatus()
\Drupal\Core\Config\Entity\ConfigEntityBase::status()

These methods are directly use the $this->status field.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented

Closed #2052083: EntityType::getKey() should be used instead of hardcoding id, status, weight, etc. as a dupe, even though it was much older.

sweetchuck’s picture

sweetchuck
Primary language Hungarian
Location Budapest
commented

Something is wrong in \Drupal\Tests\search\Unit\SearchPageRepositoryTest::testSortSearchPages()
When I modify the \Drupal\search\Entity\SearchPage::sort() to not call the parent then the test is green.

// Replace the last line:
return parent::sort($a, $b);
// with this:
return 0;

The parent::sort() is the modified \Drupal\Core\Config\Entity\ConfigEntityBase::sort() I think the problem is around \Drupal\Tests\search\Unit\TestSearchPage or the instantiation in testSortSearchPages().
Maybe the missing entityManager or entityTypeDefinition.

The error message.

Drupal\Tests\search\Unit\SearchPageRepositoryTest::testSortSearchPages
Drupal\Core\DependencyInjection\ContainerNotInitializedException: \Drupal::$container is not initialized yet. \Drupal::setContainer() must be called with a real container.

/.../core/lib/Drupal.php:129
/.../core/lib/Drupal.php:157
/.../core/modules/search/tests/src/Unit/SearchPageRepositoryTest.php:288
/.../core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php:240
/.../core/modules/search/src/Entity/SearchPage.php:214
/.../core/modules/search/src/SearchPageRepository.php:113
/.../core/modules/search/tests/src/Unit/SearchPageRepositoryTest.php:269
tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented 
+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php
@@ -236,13 +236,20 @@ public function createDuplicate() {
+    $entity_type = $a->getEntityType();

It's because we're calling this now, and the entityType/entityManager isn't mocked.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Needs work » Needs review
StatusFileSize
new configentitybase_and-2429169-12.patch8.53 KB
new interdiff.txt2.22 KB

Borrowing the uasort fix from BlockRepository.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Title: ConfigEntityBase and DraggableListBuilder ignore the entity_keys configuration » ConfigEntityBase::sort() ignores the entity_keys configuration
Status: Needs review » Needs work

The draggable list builder bit isn't a bug, the sort still is.
Still needs tests.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented

Let's close this in favor of #2052083: EntityType::getKey() should be used instead of hardcoding id, status, weight, etc..

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Needs work » Closed (duplicate)