Problem/Motivation

Take the following code snippet:

$entity = entity_create('foo');
return $entity->isNew();

That should *always* return TRUE.

While that is the case for ConfigEntity, it is not so for ContentEntity.

Changing this revealed many places in our test coverage that rely on flawed assumptions.

Proposed resolution

Make the change

Remaining tasks

Fix all failing existing tests
Add explicit test coverage

User interface changes

N/A

API changes

N/A

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

Status: Active » Needs review
FileSize
6.98 KB
1.75 KB

Just sprinkling $entity->enforceIsNew(FALSE) everywhere is not ideal, I need to go back through and see if any of these should be entity load or if they should be saved first.

Berdir’s picture

Cases where we create temporary objects or re-create objects from the values (like serialization) should also use the factory that we apparently can't get done :) entity_create() for content entities is very slow and should be avoided when you don't need the default value logic...

The last submitted patch, 1: entity-2241655-1-withfixes.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 1: entity-2241655-1.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
9.58 KB
8.31 KB

Fixing and using setOriginalId() more helps.
I think there is another problem with menu links, but the test takes 6min locally and pegs my machine, so not dealing with that yet.

Status: Needs review » Needs work

The last submitted patch, 5: entity-2241655-5.patch, failed testing.

tim.plunkett’s picture

Priority: Normal » Major
Status: Needs work » Needs review
FileSize
11.07 KB
2.09 KB

Whoops, my changes to setOriginalId were a bit flawed.
But I did find the last menu fix.

Tests coming next.

Also, I meant to file this as major. Newly created entities should be new.

sun’s picture

Hm...

  1. +++ b/core/lib/Drupal/Core/Entity/Entity.php
    @@ -502,6 +502,12 @@ public function getOriginalId() {
       public function setOriginalId($id) {
    ...
    +    // If the specified ID is anything except NULL, this should mark this entity
    +    // as no longer new.
    +    if ($id !== NULL) {
    +      $this->enforceIsNew(FALSE);
    +    }
    

    I'd prefer to reverse this convenience check, in order to ensure sanity:

    If the passed original $id is NOT NULL, and if isNew() returns TRUE, blow up with an exception.

    That is, because that state makes no sense at all, and some code is clearly broken.

  2. +++ b/core/modules/comment/comment.module
    @@ -1282,6 +1282,8 @@ function comment_prepare_author(CommentInterface $comment) {
         $account = entity_create('user', array('uid' => 0, 'name' => $comment->getAuthorName(), 'homepage' => $comment->getHomepage()));
    +    // The anonymous user is not a new account, do not try to save it as one.
    +    $account->enforceIsNew(FALSE);
    

    Unless it doesn't already, I wonder whether UserStorageController should not blow up with an exception instead, in case you're trying to (re-)save the anonymous user (uid 0)...?

  3. +++ b/core/modules/file/lib/Drupal/file/Tests/FileManagedUnitTestBase.php
    @@ -174,7 +174,10 @@ function createFile($filepath = NULL, $contents = NULL, $scheme = NULL) {
    -    return entity_create('file', (array) $file);
    +    $file_entity = entity_create('file', (array) $file);
    +    // Mark this entity as not new, since it was saved directly.
    +    $file_entity->enforceIsNew(FALSE);
    +    return $file_entity;
    

    Merely creating a file entity (object) at runtime should not actually write + save a file + file entity.

    If that is really the case, then aren't we looking at a major bug in the File entity...?

  4. +++ b/core/modules/menu_ui/menu_ui.module
    @@ -564,7 +564,12 @@ function menu_ui_form_node_form_alter(&$form, $form_state) {
    +    $original_menu_id = !empty($node->menu) ? $node->menu->id() : NULL;
         $node->menu = entity_create('menu_link', $form_state['values']['menu']);
    +    // If this menu had a previous menu link associated, mark it as not new.
    +    if ($original_menu_id) {
    +      $node->menu->setOriginalId($original_menu_id);
    +    }
    

    Hm. This looks like a straight bug to me...?

    What's the sense of creating a new entity that seemingly represents an existing, and manually futzing with it to make it appear as "not new"...?

Berdir’s picture

+++ b/core/modules/file/lib/Drupal/file/Tests/FileManagedTestBase.php
@@ -165,7 +165,10 @@ function createFile($filepath = NULL, $contents = NULL, $scheme = NULL) {
       ->execute();
-    return entity_create('file', $file);
+    $file_entity = entity_create('file', $file);
+    // Mark this entity as not new, since it was saved directly.
+    $file_entity->enforceIsNew(FALSE);
+    return $file_entity;
   }
 

Quite sure that I fixed this somewhere in an issue to actually use the API to save them...

#2183231: Make ContentEntityDatabaseStorage generate static database schemas for content entities

tim.plunkett’s picture

#8.1
I thoroughly disagree.
When we call setOriginalId() with a real value, we're saying "associate this entity with a previous one, so it counts as an update". I don't think that is an exceptional condition.

#8.2
Are you saying that this should not be possible?

$account = user_load(0);
$account->save();

If so, we need a new issue for it, because its possible now. This is just being really stupid and using create, not load.

#8.3
This is more clear with the full context:

    // Write the record directly rather than using the API so we don't invoke
    // the hooks.
    $file = (array) $file;
    $file['fid'] = db_insert('file_managed')
      ->fields($file)
      ->execute();
    $file_entity = entity_create('file', $file);
    // Mark this entity as not new, since it was saved directly.
    $file_entity->enforceIsNew(FALSE);
    return $file_entity;

As @Berdir says, there are other issues where we're fixing this, but as it stands, we're writing directly to the table first, so we need to hack the isNew().

#8.4, yeah this does seem like a bug, but it's also a valid change. *shrug*

sun’s picture

At minimum, you uncovered a ton of bugs and awkward code here, so in any case, congratulations to that! :-)

Didn't study the other cases yet, but regarding #8.1, which was referring to this snippet:

   public function setOriginalId($id) {
...
+    // If the specified ID is anything except NULL, this should mark this entity
+    // as no longer new.
+    if ($id !== NULL) {
+      $this->enforceIsNew(FALSE);
+    }

The proposed change implies that the following condition exists or is possible:

setOriginalId() is called for an existing entity ($id != NULL), but the existing entity might be marked as new, and therefore we ensure that it is not marked as new.

In my book, that is a fundamental logic flaw. The precondition must not be possible in the first place, because an entity can only be updated (and thus have an original ID) if it is NOT new to begin with. It would very potentially trigger update/rename hooks instead of insert hooks for entities that do not exist (because $id != NULL), or technically even vice-versa, and likely many more illogical effects.

It's perfectly possible that I'm not aware of the code paths involved, but in that case, I still don't see why setOriginalId() would have any business with changing the status of isNew(). Property setter methods like setOriginalId() should not have game-changing side effects like that.

Therefore, if you invoke enforceIsNew($id !== NULL), but isNew() returns TRUE, then you're trying to set an original ID for an entity that does not exist (yet), which makes no sense, and should blow up.

In fact, I don't understand why enforceIsNew() accepts a value to begin with. — I certainly hope that was meant for testing purposes only... (?!)

tim.plunkett’s picture

We're using setOriginalId() in one specific case:

We are in a complex situation where we are either inserting or updating an entity, but we don't know which yet. So we create a new one!
And then later, we somehow find out that we need to be updating.
So we mark it as not new.

---

As I write this, it sounds pretty strange.

But our needs have been strange, and we currently need to support this flow.

If you'd like me to slap an @todo on each of those, I can certainly do that.

tim.plunkett’s picture

FileSize
12.75 KB
4.73 KB

Discussed with @sun more and decided to do just that.
Opened #2241865: Do not create a new file entity in order to overwrite an existing entity and added @todos.

sun’s picture

@Berdir: Can you have a look at this + see whether the separate issue of #2241865: Do not create a new file entity in order to overwrite an existing entity to fix all of that madness is sufficient to move forward here?

Berdir’s picture

  1. +++ b/core/modules/comment/comment.module
    @@ -1281,8 +1281,10 @@ function comment_prepare_author(CommentInterface $comment) {
       // The account has been pre-loaded by CommentViewBuilder::buildContent().
       $account = $comment->getOwner();
       if (empty($account->uid->value)) {
    +    // @todo Do not create a new entity in order to update it, see
    +    //   https://drupal.org/node/2241865
         $account = entity_create('user', array('uid' => 0, 'name' => $comment->getAuthorName(), 'homepage' => $comment->getHomepage()));
    -    // The anonymous user is not a new account, do not try to save it as one.
    +    // The anonymous user is not a new account, do not treat it as one.
         $account->enforceIsNew(FALSE);
       }
       return $account;
    

    I'm not sure why this is necessary, because this not the "create to update" use case, it should be temporary and isNew() shouldn't really matter. Is something really trying to save this or did you just add it because you were looking for entity_create() calls?

    I think a @todo for #1867228: Make EntityTypeManager provide an entity factory would make more sense if anything here, because that's exactly what that issue is supposed to provide a way to create an entity object based on a set of values.

  2. +++ b/core/modules/file/file.module
    @@ -173,6 +173,8 @@ function file_copy(File $source, $destination = NULL, $replace = FILE_EXISTS_REN
         $file->setFilename(drupal_basename($uri));
         // If we are replacing an existing file re-use its database record.
    +    // @todo Do not create a new entity in order to update it, see
    +    //   https://drupal.org/node/2241865
         if ($replace == FILE_EXISTS_REPLACE) {
    

    As mentioned above, the createFile() method and AFAIK at least one of the functions in file.module is being fixed already in the entity schema generation issue.

    The fix for those is fairly easy and not depending on anything else in that issue, can we maybe just copy them over instead of adding a hack and a @todo that will conflict with a critical beta blocker?

With the two mentioned issues, there are only very few things that get a @todo here which are relevant in the other issue, there are many other cases that currently already call enforceIsNew(FALSE), somewhat surprised that we're not seeing more fails in the field tests for example.

Anyway, would be great if we could merge in the real fix for the file cases where we have one already in #2183231: Make ContentEntityDatabaseStorage generate static database schemas for content entities and update the @todo for comment_prepare_view() to say something like "Use the entity factory to not create a new entity LINK", then I'm ok with moving forward.

tim.plunkett’s picture

Issue tags: -Needs tests
FileSize
18.81 KB
8.72 KB

Borrowed these from #2183231: Make ContentEntityDatabaseStorage generate static database schemas for content entities as @Berdir suggested, and added test coverage. This seems to be the first real unit test of ConfigEntityBase/ConfigEntityStorageBase...

Status: Needs review » Needs work

The last submitted patch, 16: entity-2241655-16.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
18.55 KB
2.28 KB

Whoops, too many changes. Just stuck with overriding getFieldDefinitions.

Status: Needs review » Needs work

The last submitted patch, 18: entity-2241655-18.patch, failed testing.

Berdir’s picture

Ah, no idea about the default views test fail, but FileMove is why we need the uuid() change from over there as well. This is an actual but that we ignore right now because our initial test fiels don't have UUID's.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
18.89 KB
530 bytes

I can't reproduce the views fail. Borrowed the file_move fix and all is well locally.

Status: Needs review » Needs work

The last submitted patch, 21: entity-create-2241655-21.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
19.18 KB
3.15 KB

I didn't rerun the phpunit tests after #2182239: Improve ContentEntityBase::id() for better DX went in.
That whole dance around the entity keys needed a couple extra mocks.

sun’s picture

Status: Needs review » Reviewed & tested by the community
alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed a62511f and pushed to 8.x. Thanks!

diff --git a/core/tests/Drupal/Tests/Core/Entity/ContentEntityDatabaseStorageTest.php b/core/tests/Drupal/Tests/Core/Entity/ContentEntityDatabaseStorageTest.php
index 6f25054..41358a6 100644
--- a/core/tests/Drupal/Tests/Core/Entity/ContentEntityDatabaseStorageTest.php
+++ b/core/tests/Drupal/Tests/Core/Entity/ContentEntityDatabaseStorageTest.php
@@ -114,8 +114,6 @@ public function testFieldSqlSchemaForEntityWithStringIdentifier() {
 
   /**
    * @covers ::create()
-   *
-   * @return \Drupal\Core\Entity\EntityInterface
    */
   public function testCreate() {
     $language_manager = $this->getMock('Drupal\Core\Language\LanguageManagerInterface');

Fixed in commit.

  • Commit a62511f on 8.x by alexpott:
    Issue #2241655 by tim.plunkett: EntityStorageInterface::create() should...
tim.plunkett’s picture

Assigned: tim.plunkett » Unassigned

Status: Fixed » Closed (fixed)

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