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
In ContentEntityStorageBase::doSave():
// @todo Consider returning a different value when saving a non-default
// entity revision. See https://www.drupal.org/node/2509360.
$return = $entity->isDefaultRevision() ? SAVED_UPDATED : FALSE;Proposed resolution
TBD
Remaining tasks
TBD
User interface changes
None
API changes
TBD
Data model changes
None
Issue fork drupal-2509360
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #1
plachComment #10
apadernoComment #11
apadernoFALSEis documented to be returned when saving the entity failed. That is the wrong value to return, as the entity could have been successfully saved.Comment #12
dpiHit this myself after adding return type int to a save method on a revisionable entity. Which means that the interface is documented incorrectly at
\Drupal\Core\Entity\EntityInterface::saveas accepting only returning int. But it is in factint|FALSE.What exactly is the reason
\Drupal\Core\Entity\ContentEntityStorageBase::doSavereturnsFALSE? Surely theFALSEit is still success, so why falsey?Also has another cascading effect in that it breaks typed
\Drupal\Core\Entity\EntityFormInterface::saveimplementors.Comment #14
dpiWell, tests pass. So thats great I guess, so nothing in core seems to be relying on this, or tests it.
Putting it up for review/discussion.
Comment #15
mmbkThis solves the problem with some failing tests in https://www.drupal.org/project/drupal/issues/3264370
The failures where introduced after fixing Drupal\Node\NodeForm::save()
Comment #16
alexpottObviously we don't have test coverage if this very very odd behaviour because no tests have failed. I think this is telling us we should add a test. I also think that this is a bug because returning FALSE when something has been saved is very odd.
Comment #17
larowlanShould we add an extra constant here? We could use bitwise operators.
Comment #18
BerdirFWIW, I never liked those return values in the first place and think we should consider to deprecate having a return value entirely if that's feasible to do.
\Drupal\node\NodeForm::save() shows that it's pretty easy to get this information yourself, just call $entity->isNew() before saving. Same for the default revision. (It also still has bogus code that checks if the node has an id, which hasn't worked like that since Drupal 6, we have separate issues about catching exceptions instead).
Not sure if the DX of that isn't actually better than these constants (which are also on the list of constants to move from include fields and might need some change anyway).
Comment #19
mmbkHmm, I think having no return value for a 'save' function at all, is not a good idea, as a developer I want to know whether an entity was saved successfully without having to catch exceptions, but returning a boolean would be fine. The additional information whether the record is new, or is a draft revision could/should retrieved by other means.
Changes of the return value will affect probably all EntitySave and FormSave methods 2525794
Concerning the actual code, imho it is definitely wrong to return FALSE, when the record was actually saved successfully.
Comment #20
Berdir> Hmm, I think having no return value for a 'save' function at all, is not a good idea, as a developer I want to know whether an entity was saved successfully without having to catch exceptions
That ship has sailed about 10 years ago :)
There is no "saving failed" return value, a failure means an exception. A try/catch is the only correct approach to check if saving worked and to prevent a failure from bubbling up and interrupting everything. And yes, core and specifically entity forms are doing a very bad job at that.
Comment #24
acbramley CreditAttribution: acbramley at PreviousNext for Service NSW commentedJust hit this in the exact same way as #12
Comment #25
kim.pepper