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

Files: 
CommentFileSizeAuthor
#5 config_entity-isNew-1970110-5.patch621 bytesyched
PASSED: [[SimpleTest]]: [MySQL] 55,834 pass(es). View

Comments

andypost’s picture

Issue tags: +Needs tests, +Field API

Taggin

larowlan’s picture

Issue tags: +Configuration system

Tagging

yched’s picture

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

yched’s picture

Status: Active » Needs review
FileSize
621 bytes
PASSED: [[SimpleTest]]: [MySQL] 55,834 pass(es). View

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

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

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

Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Thanks.

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