Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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
Comment | File | Size | Author |
---|---|---|---|
#23 | interdiff.txt | 3.15 KB | tim.plunkett |
#23 | entity-create-2241655-23.patch | 19.18 KB | tim.plunkett |
Comments
Comment #1
tim.plunkettJust 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.Comment #2
BerdirCases 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...
Comment #5
tim.plunkettFixing 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.
Comment #7
tim.plunkettWhoops, 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.
Comment #8
sunHm...
I'd prefer to reverse this convenience check, in order to ensure sanity:
If the passed original
$id
is NOT NULL, and ifisNew()
returns TRUE, blow up with an exception.That is, because that state makes no sense at all, and some code is clearly broken.
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)...?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...?
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"...?
Comment #9
BerdirQuite 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
Comment #10
tim.plunkett#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?
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:
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*
Comment #11
sunAt 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:
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 ofisNew()
. Property setter methods likesetOriginalId()
should not have game-changing side effects like that.Therefore, if you invoke
enforceIsNew($id !== NULL)
, butisNew()
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... (?!)Comment #12
tim.plunkettWe'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.
Comment #13
tim.plunkettDiscussed 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.
Comment #14
sun@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?
Comment #15
BerdirI'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.
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.
Comment #16
tim.plunkettBorrowed 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...
Comment #18
tim.plunkettWhoops, too many changes. Just stuck with overriding getFieldDefinitions.
Comment #20
BerdirAh, 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.
Comment #21
tim.plunkettI can't reproduce the views fail. Borrowed the file_move fix and all is well locally.
Comment #23
tim.plunkettI 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.
Comment #24
sunCreated #2242639: Anonymous user account can be (re-)saved and doing so creates an entirely new user account
Comment #25
alexpottCommitted a62511f and pushed to 8.x. Thanks!
Fixed in commit.
Comment #27
tim.plunkett