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

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


andypost’s picture

Issue tags:+Needs tests, +Field API


larowlan’s picture

Issue tags:+Configuration system


yched’s picture

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

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
new621 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.