ConfigEntityBase isNew() method checks that id() returns '' or NULL

  final public function isNew() {
    // Configuration entity IDs are strings, and '0' is a valid ID.
    return !empty($this->enforceIsNew) || $this->id() === NULL || $this->id() === '';
  }

but id() always returns a value so createDuplicate() is broken too

CommentFileSizeAuthor
#5 config_entity-isNew-1970110-5.patch621 bytesyched

Comments

andypost’s picture

andypost
he/him
Primary language Russian
commented
Issue tags: +Needs tests, +Field API

Taggin

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Issue tags: +Configuration system

Tagging

yched’s picture

yched commented
Title: Field instance isNew() method is broken » Should ConfigEntity::isNew() be based on id or uuid ?
Component: field system » configuration system

Hmm.
According to ConfigEntityBase, isnew() is FALSE the moment the $config_entity has a non empty id.
(or between entity_create() and save(), when enforceIsNew is TRUE)

That seems wrong to me ?

Entity does :

  public function isNew() {
    return !empty($this->enforceIsNew) || !$this->id();
  }

That seems very much tied to a pre-D8 model where "a $node is considered new before it got saved and received a serial id in the process".

If so, I'd say the equivalent of this for ConfigEntities is the presence of a UUID, not an id, because ConfigEntities will always have an id (a machine name) ?

Bumping to the config system for feedback.

andypost’s picture

andypost
he/him
Primary language Russian
commented

It was introduced in #1813832: Entity wrongly checks existence of ID in isNew() method

yched’s picture

yched commented
Status: Active » Needs review
StatusFileSize
new config_entity-isNew-1970110-5.patch621 bytes

Discussed this with @Berdir and @fago.

ConfigStorageController::create() calls $entity->enforceIsNew() already.
ConfigEntityBase::isNew() should be based only on the enforceIsNew flag.

Status: Needs review » Needs work
Issue tags: -Needs tests, -Configuration system, -Field API

The last submitted patch, config_entity-isNew-1970110-5.patch, failed testing.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Needs work » Needs review
Issue tags: +Needs tests, +Configuration system, +Field API

#5: config_entity-isNew-1970110-5.patch queued for re-testing.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Reviewed & tested by the community

Makes sense, checking Id for config entities has no meaning as it's always manually assigned.

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Thanks.

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