Problem/Motivation

Most EntityFormInterface::save() implementations provide a similar logic, with very small variations.

Proposed resolution

It should be possible to make sure all implementations follow exactly the same pattern and even better factor out the common logic in the parent implementations if/when possible.

Remaining tasks

  • Write a patch
  • Review it

User interface changes

None

API changes

None

Data model changes

None

CommentFileSizeAuthor
#35 2525794-nr-bot.txt144 bytesneeds-review-queue-bot
#32 2525794-32.patch36.29 KBAnkit.Gupta
#31 2525794-31.patch36.34 KBAnchal_gupta
#31 2525794-30_31.txt4.83 KBAnchal_gupta
#30 2525794-30.patch36.29 KBSuresh Prabhu Parkala
#13 improve_consistency_in-2525794-13.patch38.21 KBkostyashupenko
#10 interdiff-2525794-8-10.txt2.85 KBdcmul
#10 improve_consistency_in-2525794-10.patch38.21 KBdcmul
#8 interdiff-2525794-6-8.txt546 bytesdcmul
#8 improve_consistency_in-2525794-8.patch37.13 KBdcmul
#6 interdiff-2525794-3-6.txt29.65 KBdcmul
#6 improve_consistency_in-2525794-6.patch37.13 KBdcmul
#4 improve_consistency_in-2525794-3.patch6.25 KBdcmul
#2 improve_consistency_in-2525794-2.patch5.12 KBdcmul
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach as a volunteer and for Drupal Association commented

In #2453175-124: Remove EntityFormInterface::validate() and stop using button-level validation by default in entity forms @Berdir said:

+++ b/core/modules/user/src/RegisterForm.php
@@ -102,8 +102,9 @@ public function save(array $form, FormStateInterface $form_state) {

-    // Make sure we do not skip the parent's entity save logic.
-    parent::save($form, $form_state);
+    // Save has no return value so this cannot be tested.
+    // Assume save has gone through correctly.
+    $account->save();

Kinda sad to see this nonense re-added (or the removal reverted). Can we explicitly mention this in the follow-up that you opened?

dcmul’s picture

dcmul CreditAttribution: dcmul as a volunteer commented
Status: Active » Needs review
FileSize
improve_consistency_in-2525794-2.patch5.12 KB

This is my take, not sure I am on the right track. If this is ok, I can work on all the implementations of EntityFormInterface::save()
I have done 2 things,
1. Captured the int value that returned by EntityInterface::save
2. Returned that value at the end of the method. As per the docs

   * @return int
   *   Either SAVED_NEW or SAVED_UPDATED, depending on the operation performed.

Status: Needs review » Needs work

The last submitted patch, 2: improve_consistency_in-2525794-2.patch, failed testing.

dcmul’s picture

dcmul CreditAttribution: dcmul as a volunteer commented
Status: Needs work » Needs review
FileSize
improve_consistency_in-2525794-3.patch6.25 KB

removed syntax error

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach as a volunteer and for Drupal Association commented
Status: Needs review » Needs work

Thanks, the patch is looking very good, already!

  1. +++ b/core/modules/forum/src/Form/ForumForm.php
@@ -97,7 +97,7 @@ public function save(array $form, FormStateInterface $form_state) {
-    return $term;
+    return $status;

    Nice catch :) 

  2. +++ b/core/modules/node/src/NodeForm.php
@@ -373,13 +373,13 @@ public function buildEntity(array $form, FormStateInterface $form_state) {
+    if ($status == SAVED_NEW) {

    To make things more consistent, would it make sense to always use a switch instead of using ifs from time to time? 

  3. +++ b/core/modules/node/src/NodeForm.php
@@ -411,6 +411,7 @@ public function save(array $form, FormStateInterface $form_state) {
+    return $status;

    Good point about this: we should not break the interface contract. 

  4. +++ b/core/modules/user/src/RegisterForm.php
@@ -102,15 +102,18 @@ public function save(array $form, FormStateInterface $form_state) {
+    if($status == SAVED_NEW) {
+      $this->logger('user')->notice('New user: %name %email.', array('%name' => $form_state->getValue('name'), '%email' => '<' . $form_state->getValue('mail') . '>', 'type' => $account->link($this->t('Edit'), 'edit-form')));
+    }
+    else {
+      $this->logger('user')->notice('Can not create New user: %name %email.', array('%name' => $form_state->getValue('name'), '%email' => '<' . $form_state->getValue('mail') . '>', 'type' => $account->link($this->t('Edit'), 'edit-form')));
+      return $status;
+    }
     // Add plain text password into user account to generate mail tokens.

    Please revert this hunk, the previous code was correct: we are in the user register form, so we are always creating a new user entity. The return value will never indicate an error condition, an exception will be thrown in that case.

One side note: when posting a new patch, an interdiff (a diff between the previous patch and the current one) is welcome because it makes reviewing the changes easier, instead of skimming through the whole patch every time.

dcmul’s picture

dcmul CreditAttribution: dcmul as a volunteer commented
Status: Needs work » Needs review
FileSize
improve_consistency_in-2525794-6.patch37.13 KB
interdiff-2525794-3-6.txt29.65 KB

Thanks for your review.

The implementations below are pending, their structure is a little bit complex
Drupal\shortcut\Form\SetCustomize::save
Drupal\book\Form\BookOutlineForm::save
Drupal\comment\CommentForm::save

Status: Needs review » Needs work

The last submitted patch, 6: improve_consistency_in-2525794-6.patch, failed testing.

dcmul’s picture

dcmul CreditAttribution: dcmul as a volunteer commented
Status: Needs work » Needs review
FileSize
improve_consistency_in-2525794-8.patch37.13 KB
interdiff-2525794-6-8.txt546 bytes

fixing the syntax error

Status: Needs review » Needs work

The last submitted patch, 8: improve_consistency_in-2525794-8.patch, failed testing.

dcmul’s picture

dcmul CreditAttribution: dcmul as a volunteer commented
Status: Needs work » Needs review
FileSize
improve_consistency_in-2525794-10.patch38.21 KB
interdiff-2525794-8-10.txt2.85 KB

Hope the tests pass this time.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented
Version: 8.1.x-dev » 8.2.x-dev
Status: Needs review » Needs work
Issue tags: -Novice +Needs reroll

Patch doesn't apply to 8.1.x.

Refactoring and/or code changes will have to happen in 8.2.x for a task.

Pretty sure this isn't a novice issue.

kostyashupenko’s picture

kostyashupenko
Primary language Russian
Location Omsk
CreditAttribution: kostyashupenko at Skilld commented
Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
improve_consistency_in-2525794-13.patch38.21 KB

Reroll of #10

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Status: Needs review » Needs work

The last submitted patch, 13: improve_consistency_in-2525794-13.patch, failed testing.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

mmbk’s picture

mmbk
Primary language German
Location Meißen
CreditAttribution: mmbk as a volunteer and at erdfisch commented

This is imho a quite important issue and is so long abandoned. I guess this issue addresses too many components so that it is hard to handle. Splitting it into sub tasks might speed up the development here.

I started by creating a subtask for the NodeForm::save() method.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Version: 9.5.x-dev » 10.1.x-dev
Related issues: +#2030291: Allow actions to postpone saving the modified entity, +#3282744: SAVED_DELETED is not used in D10, D9, or D8 and should be removed

linking related

- #2030291: Allow actions to postpone saving the modified entity
- #3282744: SAVED_DELETED is not used in D10, D9, or D8 and should be removed

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Issue tags: +Needs reroll
Suresh Prabhu Parkala’s picture

Suresh Prabhu Parkala CreditAttribution: Suresh Prabhu Parkala at Specbee commented
FileSize
2525794-30.patch36.29 KB

Just a re-roll. Please review.

Anchal_gupta’s picture

Anchal_gupta CreditAttribution: Anchal_gupta at Srijan | A Material+ Company for Drupal India Association commented
FileSize
2525794-30_31.txt4.83 KB
2525794-31.patch36.34 KB

I have Fixed the custom command failed. Please Review

Ankit.Gupta’s picture

Ankit.Gupta CreditAttribution: Ankit.Gupta at Dotsquares Ltd. commented
Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
2525794-32.patch36.29 KB

Reroll the patch #30 with Drupal 10.1.x

Chi’s picture

Chi CreditAttribution: Chi commented

array('@id' => $entity->id(), '%entity_type' => $entity->getEntityTypeId()))

Why does it apply long array syntax?

-      if ($is_new) {
-        $message = t('%entity_type @id has been created.', ['@id' => $entity->id(), '%entity_type' => $entity->getEntityTypeId()]);
-      }
-      else {
-        $message = t('%entity_type @id has been updated.', ['@id' => $entity->id(), '%entity_type' => $entity->getEntityTypeId()]);
+      switch ($status) {
+        case SAVED_NEW:
+          $message = t('%entity_type @id has been created.', array('@id' => $entity->id(), '%entity_type' => $entity->getEntityTypeId()));
+          break;
+        case SAVED_UPDATED:
+          $message = t('%entity_type @id has been updated.', array('@id' => $entity->id(), '%entity_type' => $entity->getEntityTypeId()));
+          break;
       }

In PHP 8 the above code can be written in more readable way using match expression.

$message_args = ['@id' => $entity->id(), '%entity_type' => $entity->getEntityTypeId()];
$message = match($result) {
  \SAVED_NEW => t('%entity_type @id has been created.', $message_args),
  \SAVED_UPDATED => t('%entity_type @id has been updated.', $message_args),
};

As a bonus it'll throw an exception when save operation returns unexpected value.

Chi’s picture

Chi CreditAttribution: Chi commented
Related issues: +#3333688: \Drupal\comment\CommentForm::save does not return a value

Agreed with #26. This issue is too broad and has to be split into sub-tasks.

needs-review-queue-bot’s picture

needs-review-queue-bot CreditAttribution: needs-review-queue-bot as a volunteer commented
Status: Needs review » Needs work
FileSize
2525794-nr-bot.txt144 bytes

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

dpi’s picture

dpi
Location Perth, Australia
CreditAttribution: dpi as a volunteer commented
Issue tags: +PHPStan-0

Adding from #3347670: Fix "Method ::save() should return int but return statement is missing"

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.