function Drupal\Node\NodeForm::save() does not return a value

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.

This will need a test case showing the issue.

This can't be tested because that return type is bogus. save() is used as a submit callback, submit callbacks have no return value. This was just copied from the Entity save method for no reason. We should instead remove the @return from \Drupal\Core\Entity\EntityFormInterface::save().

samit.310@gmail.com → made their first commit to this issue’s fork.

I Created a new MR(with new branch) against 11.x, as per my analysis it should be a small 1 to 2 line fix, Please review it,
It has 1 phpstan error, ideally it should not be there as save() function has {@inheritdoc} comment block.

Thanks
Samit K.

Still needs tests.

Cannot be tested as per #12 I also think this would fall into the bucket of a trivial fix that would be too much effort to test.

Re the rest of #12 - yes it's sort of bogus, but it is used in places to determine if it was a new or existing entity (as per the doc block).

Looking through other parts of core, there are many that are missing return values though, so just fixing NodeForm isn't really going to help us much (See CommentForm, CommentTypeForm, ActionFormBase, etc)

So I agree, we should instead deprecate the return value and remove it entirely at some point.

Should the return value from EntityInterface::save also be removed? And if so, should we create a separate issue for that or can we deal with that here?

> Re the rest of #12 - yes it's sort of bogus, but it is used in places to determine if it was a new or existing entity (as per the doc block).

Yes. There are two things here though.

As a form callback, returning a value is meaningless. It's not used, form callbacks don't have a return value.

But, the problem are $return = parent::save() calls. Like for example core/modules/media/src/MediaTypeForm.php. If we remove the return in EntityForm::save(), that will break.

I'm not exactly sure how we should deprecate that. We could start with just removing the documentation for it and creating a change record, but I don't think we can actually trigger a deprecation.

Instead of using the save return value, form classes can do this:

$is_new = $this->entity->isNew();
parent::save();
if ($is_new) {
  // created message.
}
else {
  // updated message.
}

Easier and doesn't need those strange constants.

The current save() method is a trivial oneliner. I'm not sure why we even have a separate submitForm() and save() submit callback. Maybe to allow to customize it. I'd love to improve that in general. We have a trillion of slightly different save implementations, some log, some display a message, some just one, some different depending on new/updated. Not a single one of them catches exceptions to avoid a fatal error if something goes wrong during saving. If we change something, maybe we could deprecate the whole method and replace it with something that has sensible default logic. mabe a similar pattern as confirm forms to customize the messages if necessary?

> Should the return value of EntityInterface::save() and EntityStorageInterface::save() also be removed? And if so, should we create a separate issue for that or can we deal with that here?

The change in the current MR is wrong. No, we don't want to change EntityInterface::save() nor EntityStorageInterface::save(). *only* EntityForm::save(). The MR should just change the form class, *not* the storage class.

Submitted too early.

I'm aware that the proposal has a way larger scope then this issue. And there are several open related issues about it. Im also not sure how we'd deprecate and switch since it's a defined form callback and subclasses rely on it being called? A flag to opt-in to a new method that we'd deprecate again in D12 where we switch the default? A bit like the show revision ui flag we have on entity forms.

Sorry for the notification spam. ideas keep popping up.

We could add a trait like EntityFormDefaultSaveTrait or something. The actions form can check if that's present and if yes, use the new save() method, if not it doesn't and the current save() is changed to trigger a deprecation message. In D12/13, we include that trait by default and remove the old method.

I know there were other issues about this: 📌 Improve consistency in EntityFormInterface::save() implementations Needs work