Re-saving a new bundle entity throws an exception

Facing the same issue here. We need to add tests reproducing the issue.

Probably obvious, but just in case, a workaround for contrib in the meanwhile: you can load the entity being saved again, and it will have the proper original id values.

Moving to needs work as tests are kinda clearly missing ...

1. +++ b/core/lib/Drupal/Core/Entity/EntityStorageBase.php
   @@ -470,15 +470,29 @@ protected function doPostSave(EntityInterface $entity, $update) {
   +    // For updated entities, we only change the original ID after the postSave()
   +    // methods and update hooks have run so those functions could correctly
   +    // identify any changes made to the ID.
   +    if ($update) {
   +      $entity->setOriginalId($entity->id());
   +    }
   

   Good catch. This could be the case for config entities theoretically.

2. +++ b/core/lib/Drupal/Core/Entity/EntityStorageBase.php
   @@ -470,15 +470,29 @@ protected function doPostSave(EntityInterface $entity, $update) {
        // After saving, this is now the "original entity", and subsequent saves
        // will be updates instead of inserts, and updates must always be able to
        // correctly identify the original entity.
   -    $entity->setOriginalId($entity->id());
   

   Doesn't that make parts of the documentation redundant now?

I guess we indeed just have implicit test coverage via nodes, comments as well as block_content entities.

but the test need to unset the originalId property or find another way to fake a form submission

Wouldn't it be better than to actually create a webtestbased test ? Sounds more honest then as a test instead of manually unsetting it.

1. +++ b/core/lib/Drupal/Core/Entity/EntityStorageBase.php
   @@ -470,15 +470,29 @@ protected function doPostSave(EntityInterface $entity, $update) {
        // After saving, this is now the "original entity", and subsequent saves
        // will be updates instead of inserts, and updates must always be able to
        // correctly identify the original entity.
   -    $entity->setOriginalId($entity->id());
   -
   

   these docs should be removed as they are pointless no ?

2. +++ b/core/modules/system/src/Tests/Entity/BundleEntityTest.php
   @@ -0,0 +1,41 @@
   + * Tests proper cloning of content entities.
   

   Are we really cloning here ?

@kristiaanvandeneynde
No reason not to provide different patches for 8.1 and 8.0

The core committers discussed this issue with the Entity and Field maintainers today. We agreed to downgrade this issue because it is only developer-facing and it is possible to work around it by setting the original ID before re-saving the entity. The error is also not unrecoverable as far as we can tell (so it doesn't entirely crash the site). An exception is a reasonable result to inform the developer that the API does not yet support this (even though that is unexpected and we probably want to fix it).

The summary could use some clarification. The first paragraph seems much broader in scope than the title suggests. Rather than "an entity", should it be "a configuration entity that is used for a bundle of another entity (like a content type/vocabulary)"? It would also be helpful to explain a usecase/scenario where this needs to be done in postSave().

Thanks for reporting this!

Interesting enough this sounds really similar to #2718839: hook_ENTITY_update() and hook_ENTITY_insert() don't allow to resave entities with a new revision, but just the for entity ID instead of the revision ID

Thanks @kristiaanvandeneynde, that helps!

OMG you have no idea how much pain I had with that particular bit of code.

+++ b/core/lib/Drupal/Core/Entity/EntityStorageBase.php
@@ -470,15 +470,29 @@ protected function doPostSave(EntityInterface $entity, $update) {
+    // After saving a new entity, modules may immediately re-save the entity in
+    // an insert hook or postSave method. If we don't set the original ID before
+    // that happens, the site may throw an exception when it incorrectly
+    // compares the ID to the original ID (NULL). An example of this can be
+    // found in \Drupal\Core\Config\Entity\ConfigEntityBundleBase::preSave().
+    if (!$update) {
+      $entity->setOriginalId($entity->id());
+    }

I don't see an example of a save inside a test. Ideally this should be tested as well.

+++ b/core/modules/system/src/Tests/Entity/BundleEntityTest.php
@@ -0,0 +1,32 @@
+/**
+ * Tests functionality specific to bundle entities.
+ *
+ * @group Entity
+ */
+class BundleEntityTest extends EntityUnitTestBase {
+

You shouldn't, just switch to EntityKernelTestBase and move the test to core/tests/Kernel/Entity or something like that, where the other entity tests are.

@kristiaanvandeneynde
I think having a save in a update hook would be nice as a test, as this is also a really common usecase.

Well for me its mostly about ensuring resaving actually works. I ran into issue inside editor module for example, so ideally try to enable that module in the test, just as an experiment.

Rerolled the patch.

I'm not convinced the patch in #42 requires a re-roll, it applies perfectly well for me. Perhaps @PavanBS was trying to apply it to 8.4.x? Retesting #42 to be sure, setting back to Needs Review in expectation of a test pass.

Thanks @Pavan B S for your interest in contributing! Note that I've saved this issue to remove any default credit for your attempted reroll, because the reroll was incorrect and not needed. (Adding the "Needs reroll" tag to a patch that actually applies is also not correct.) You can see https://www.drupal.org/patch/reroll#introduction for more information on how to reroll a patch, including how to tell from the automated testing system when a reroll is actually needed. Maybe you could look into some of the other contributor tasks for other issues, since this issue currently just needs peer code review.

@kristiaanvandeneynde, note that if you attach the test-only patch as the first attachment, rather than the second, that ensures that testbot will not mark the issue needs work (since the issue status is based on the last file attachment).

@xjm, thanks for the link, i will go through that link, and i will make sure that i will not do that mistake in future.

any chance to get this committed?

1. +++ b/core/lib/Drupal/Core/Entity/EntityStorageBase.php
   @@ -465,15 +465,29 @@ protected function doPostSave(EntityInterface $entity, $update) {
        // After saving, this is now the "original entity", and subsequent saves
        // will be updates instead of inserts, and updates must always be able to
        // correctly identify the original entity.
   -    $entity->setOriginalId($entity->id());
   ...
        unset($entity->original);
   

   This comment references the updating of the original ID, so it should be moved to the new description.

2. +++ b/core/modules/system/tests/modules/entity_test/entity_test.module
   @@ -411,6 +411,16 @@ function entity_test_entity_test_insert($entity) {
   +function entity_test_entity_test_bundle_insert($entity) {
   

   The type of the parameter is missing.

3. +++ b/core/tests/Drupal/KernelTests/Core/Entity/BundleEntityTest.php
   @@ -0,0 +1,35 @@
   +    $entity = EntityTestBundle::create(['label' => 'Foo']);
   +    $entity->set('id', 'foo');
   

   This could become a single line.

--------

any chance to get this committed?

Only when it is RTBC. You could help with the patch or the review of it.

I've implemented the changes suggested at #62 and re-rolled for 8.9.x and 9.0.x.
It needs review, please

Test only are passing and should fail.

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.

The tests-only patch should definitely fails so needs work for those.

Added patch against #64 in 10.1 version

Hiding patch #75 as it's incomplete and omitting important changes and/or files.

Adding to the list [#https://www.drupal.org/project/site_moderators/issues/3339883]