Problem/Motivation

Entity::isNew() should check that ID-value exists but currently it check for equality to FALSE

Follow-up from this about machine name usage #1814962: Disallow machine names to start with numbers
Currently machine-readable names allows to use '0' (string zero) for value
Mostly all of config entities use machine names as id()
But core disallows delete entities with '0' as key because ConfigEntityBase::isNew() uses empty to check value

Proposed resolution

1) Fix isNew() method as patch #2 suggests
2) Disallow usage of '0' as machine-name (needs more work ans tests)

Original text
Currently config entities can't use '0' (string zero) as machine name

CommentFileSizeAuthor
#34 1813832-config-isnew-34-just-test.patch1.24 KBandypost
#31 config-isnew-31-just-test.patch1.23 KBandypost
#22 config-isnew-21.patch1.96 KBandypost
#16 config-isnew-test-only-16.patch1.01 KBandypost
#16 config-isnew-16.patch1.99 KBandypost
#2 config-isnew-testonly-2.patch969 bytesandypost
#2 config-isnew-2.patch1.92 KBandypost
#1 config-isnew-testonly-1.patch1.49 KBandypost
#1 config-isnew-1.patch1.99 KBandypost
config-isnew-0.patch1.4 KBandypost
config-isnew-testonly-0.patch1.4 KBandypost

Comments

andypost’s picture

andypost
he/him
Primary language Russian
commented
StatusFileSize
new config-isnew-1.patch1.99 KB
new config-isnew-testonly-1.patch1.49 KB

heh it needs real implementation

andypost’s picture

andypost
he/him
Primary language Russian
commented
Priority: Normal » Major
Issue tags: +Configurables
StatusFileSize
new config-isnew-2.patch1.92 KB
new config-isnew-testonly-2.patch969 bytes

Actually only delete() method affected by this, so test is changed to make sure that delete works

podarok’s picture

podarok
🇺🇦 he/him/his
Primary language Ukrainian
Location Ukraine
commented
Status: Needs review » Reviewed & tested by the community

#2 looks good for me

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented

Can I get more information on why this is a) desirable, and b) constitutes a major bug?

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Priority: Major » Normal
Status: Reviewed & tested by the community » Needs review
dries’s picture

dries commented
Priority: Normal » Major
Status: Needs review » Fixed

Nice fix/improvement. I'm comfortable with changing the existing tests. Committed to 8.x. Thanks.

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Fixed » Needs work

Um, what?

This seems like a really bad idea to me.

xjm’s picture

xjm
she/her
Primary language English
commented

Given that:

  • We are converting this data back and forth between YAML and PHP,
  • The boolean FALSE is stored as 0 in YAML, and
  • PHP's loose typing causes no end of bugs related to empty values.

It seems like a bad idea to me to allow something that evaluates to empty as a key, especially as the ID. I'd want to see a very strong case for the need for this change before considering it. Can we revert this until we can discuss the implications?

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Priority: Major » Normal

I think Dries's commit was a cross-post with mine given the timestamps, and given xjm's comments which seem worthy of consideration, reverted this patch from 8.x, pending more discussion.

Also, when I asked andypost why this was major in IRC he said something like "because it's hard to track down" but I don't think that qualifies against the "significant repercussions" definition at http://drupal.org/node/45111. Not worth holding up features for this unless we get more details on what caused this patch to be written in the first place.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs work » Needs review

ok, let's leave this normal but I disagree... no matter.

Currently we allow usage of '0' as machine-name but have a bug when trying to delete entities with this key

xjm’s picture

xjm
she/her
Primary language English
commented
Title: Allow use '0' as id for entity » Disallow 0 as an ConfigEntity ID
Status: Needs review » Needs work

So, according to @andypost, right now it's possible to create something with 0 as the key, but not delete it. I'd recommend fixing this by disallowing whatever path is currently being used to create them with zero as a key.

xjm’s picture

xjm
she/her
Primary language English
commented

In form_process_machine_name it looks like a single zero is in fact possible as a machine name currently.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Title: Disallow 0 as an ConfigEntity ID » Disallow '0' as machine readable name or just allow delete entities with '0' as key
Status: Needs work » Needs review

Suppose better title, actually describes the issue

Updated summary

xjm’s picture

xjm
she/her
Primary language English
commented

Alright, if we want to allow this patch in as a stopgap to fix the bug with deletion, here's what I'd personally like to see:

  • @todo for a followup.
  • Adding another entity to the existing test instead of replacing the current one.
  • Can we explore why isNew() has bearing on whether we can delete things? It wasn't obvious to me at all looking in the interface and storage controller, so I'd like to know why that is the fix.
andypost’s picture

andypost
he/him
Primary language Russian
commented

@xjm You can found affected code in /core/lib/Drupal/Core/Entity/Entity.php

  /**
   * Implements EntityInterface::delete().
   */
  public function delete() {
    if (!$this->isNew()) {
      entity_get_controller($this->entityType)->delete(array($this->id()));

so !$this->id(); in isNew() methods makes type-cast of string '0' to bool

Agreed that tests could be extended with addition of entity with '0' machine name

andypost’s picture

andypost
he/him
Primary language Russian
commented
StatusFileSize
new config-isnew-16.patch1.99 KB
new config-isnew-test-only-16.patch1.01 KB

Patch with test that adds a entity with '0' and tries to delete

andypost’s picture

andypost
he/him
Primary language Russian
commented
Title: Disallow '0' as machine readable name or just allow delete entities with '0' as key » Entity wrongly checks existence of ID in isNew() method

Changing title - actually isNew() should check that ID-value exists but currently it check for equality to FALSE

Also filed #1814962: Disallow machine names to start with numbers

andypost’s picture

andypost
he/him
Primary language Russian
commented
Issue summary: View changes

Updated issue summary.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

We can check for === NULL instead.

xjm’s picture

xjm
she/her
Primary language English
commented

#18 sounds safest to me. Edit: I'll expand the scope of the followup issue.

xjm’s picture

xjm
she/her
Primary language English
commented

Oh, I see #1814962: Disallow machine names to start with numbers has been been re-scoped already.

How should we respond if someone tries to create an entity with an ID that's an empty string, FALSE, etc.? Is that a separate issue from this or #1814962: Disallow machine names to start with numbers? Edit: Maybe we should throw an exception, and yes, I do think it's probably a separate issue, since it's not isNew()'s business to catch that.

andypost’s picture

andypost
he/him
Primary language Russian
commented

=== NULL is a good idea!
1814962 is about UI, nothing stops to entity_create('entity', array('id'=>'0')) so this issue makes sense too

andypost’s picture

andypost
he/him
Primary language Russian
commented
StatusFileSize
new config-isnew-21.patch1.96 KB

Patch

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

Alright, this looks better to me now (assuming it passes tests).

I opened #1817176: Determine how to handle empty values for entity IDs for the question about empty IDs on creation.

podarok’s picture

podarok
🇺🇦 he/him/his
Primary language Ukrainian
Location Ukraine
commented

#22 yet one to RTBC
let`s make this in
following #1817176: Determine how to handle empty values for entity IDs now

webchick’s picture

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

Looks good. Thanks for the follow-up.

Committed and pushed to 8.x. Thanks!

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Fixed » Needs work

This commit broke HEAD.
Can we revert it for now and figure it all out?

tim.plunkett’s picture

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

Here are the fails

Identical result set	Other	ArgumentUserUIDTest.php	34	Drupal\views\Tests\Comment\ArgumentUserUIDTest->testCommentUserUIDTest()
Identical result set	Other	FilterUserUIDTest.php	45	Drupal\views\Tests\Comment\FilterUserUIDTest->testCommentUserUIDTest()

I can try to debug this tomorrow once it's reverted.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented

Yep. git revert 453b2517f5c

LOL, I'm really sorry andypost. :) We'll get there!

andypost’s picture

andypost
he/him
Primary language Russian
commented

@sun is trying to change this a different way #1798944: Convert config_test entity forms to EntityFormController

andypost’s picture

andypost
he/him
Primary language Russian
commented

About '0' value - we have UID=0 in core, so related but by the same nature #1825568: Random test failure in Drupal\system\Tests\Entity\EntityFormTest

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs work » Needs review
StatusFileSize
new config-isnew-31-just-test.patch1.23 KB

Probably this was fixed, let's get just test patch

Status: Needs review » Needs work

The last submitted patch, config-isnew-31-just-test.patch, failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented 
+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigEntityTest.php
@@ -252,6 +252,21 @@ function testCRUDUI() {
     $id = $edit['id'];
...
+    // Create a configuration entity with '0' machine name.
+    $edit = array(
+      'id' => '0',
...
+    $this->assertUrl("admin/structure/config_test/manage/$id/edit");
+    $this->assertUrl("admin/structure/config_test/manage/$id/delete");

$id still contains the random ID instead of '0'.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs work » Needs review
StatusFileSize
new 1813832-config-isnew-34-just-test.patch1.24 KB

Here's a fixed version

sun’s picture

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

Thanks, I think this is a regression test that is worth to add.

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Comitted/pushed the test. Thanks!

Status: Fixed » Closed (fixed)

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

yched’s picture

yched commented

Cross-linking for the folks that took part in the thread: #1970110: Should ConfigEntity::isNew() be based on id or uuid ?

yched’s picture

yched commented
Issue summary: View changes

Updated issue summary.