Updated: Comment #N

Problem/Motivation

In ConfigStorageController::save(), the first check is 

    $id = $entity->id();
    if ($id === NULL || $id === '') {
      throw new EntityMalformedException('The entity does not have an ID.');
    }

But then after some other code loading the CMI object directly, we have this

    // Build an ID if none is set.
    if (!isset($entity->{$this->idKey})) {
      $entity->{$this->idKey} = $entity->id();
    }

This should be completely unnecessary, and has the side effect of forcing the ID key to be public.

Proposed resolution

Remove this line, and change the ID property on at least one entity to protected.

Remaining tasks

N/A

User interface changes

N/A

API changes

When the id() method is overridden to use something other than $entity->{$idKey}, the id() method must be used in toArray() so that it is set properly.

CommentFileSizeAuthor
#19 configentity-2224833-19.patch10.25 KBtim.plunkett
#18 configentity-2224833-18.patch10.32 KBtim.plunkett
#16 configentity-2224833-16.patch6.74 KBtim.plunkett
#11 configentity-2224833-11.patch7.03 KBtim.plunkett
#9 configentity-2224833-9.patch4.23 KBtim.plunkett
#6 interdiff.txt10.42 KBtim.plunkett
#6 configentity-2224833-6.patch16.1 KBtim.plunkett
#4 configentity-2224833-4.patch6 KBtim.plunkett
#1 configentity-2224833-1b.patch4.03 KBtim.plunkett
#1 configentity-2224833-1a.patch3.91 KBtim.plunkett
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Active » Needs review
FileSize
configentity-2224833-1a.patch3.91 KB
configentity-2224833-1b.patch4.03 KB

If a compound entity ID is used, then it cannot use $entity->get('id') or equivalent in toArray(), it must use id().

A follow-up to #2016679: Expand Entity Type interfaces to provide methods, protect the properties should be to make all config entity properties protected, and we can improve ConfigEntityBase::toArray() accordingly.

The only difference between A and B is that B makes $id protected for EntityDisplayBase

Status: Needs review » Needs work

The last submitted patch, 1: configentity-2224833-1b.patch, failed testing.

The last submitted patch, 1: configentity-2224833-1a.patch, failed testing.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Needs work » Needs review
FileSize
configentity-2224833-4.patch6 KB

FieldInstanceConfig
__wakeup() -> toArray() -> id() -> $this->field->name -> BOOM

Can be $this->field_name

Status: Needs review » Needs work

The last submitted patch, 4: configentity-2224833-4.patch, failed testing.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Needs work » Needs review
Issue tags: -Quick fix
FileSize
configentity-2224833-6.patch16.1 KB
interdiff.txt10.42 KB

Not so quick anymore.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented 
+++ b/core/modules/entity/lib/Drupal/entity/EntityDisplayBase.php
@@ -23,7 +23,7 @@
-  public $id;
+  protected $id;

This one was done as an example. Should I go through the rest of the config entities and make this change? If not, this patch should be good to go.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Related issues: +#2016679: Expand Entity Type interfaces to provide methods, protect the properties

Getting this issue in now will let us change each entity type in their respective subissue in #2016679: Expand Entity Type interfaces to provide methods, protect the properties.
I think we should tackle those individually and get this in as-is.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Issue summary: View changes
FileSize
configentity-2224833-9.patch4.23 KB

Per #8, I'm stripping this down to the absolute minimum changes.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Needs work » Needs review
FileSize
configentity-2224833-11.patch7.03 KB

Restoring the image changes from #6 :(

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Needs work » Needs review
FileSize
configentity-2224833-16.patch6.74 KB

Ugh, still cut out too much.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Assigned: tim.plunkett » Unassigned
Status: Needs work » Needs review

Okay forget this. I remember adding those changes for a reason, and I guess this was why.

#6 needs review.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
FileSize
configentity-2224833-18.patch10.32 KB

Okay one more try for a stripped down version.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
FileSize
configentity-2224833-19.patch10.25 KB

git 3-way merge++

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Title: Remove redundant ID manipulation in ConfigStorageController::save() » Remove redundant ID manipulation in ConfigEntityStorage::save()
dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs review » Reviewed & tested by the community

This seems to be perfect, I can't see something horrible

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

  • Commit 684f0cb on 8.x by webchick: 
    Issue #2224833 by tim.plunkett: Remove redundant ID manipulation in...

Status: Fixed » Closed (fixed)

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