Problem/Motivation

@xjm, from #2486177-132: Deleting an entity translation from the UI deletes the whole entity:

I'm in the process of reviewing this and I must admit the sorta-crazy around EntityDeleteFormTrait is snarling my brain. I read over #1728804: Introduce (Content)EntityDeleteForm and children to handle entity deletions for some background on why things are the way they are, but I'm still unclear on why, for example, ContentEntityConfirmFormBase is a thing (it seems only used for deletions), and why we don't just do away with EntityDeleteFormTrait at this point (when we're overriding it so much) and just put the respective versions of the methods in ContentEntityDeleteForm and EntityDeleteForm (which are the only usages). (For others' reference, ContentEntityDeleteForm does not extend EntityDeleteForm; and EntityDeleteForm has only config-specific stuff, which also confused me quite a bit.) Is there a reason to keep the trait other than for BC? Because then we could do away with the trait overriding cleverness.

Proposed resolution

Fold ContentEntityConfirmFormBase into EntityConfirmFormBase and move all the methods from EntityDeleteFormTrait to EntityDeleteForm.

Remaining tasks

  • Propose a solution
  • Write a patch
  • Review it

User interface changes

Nope.

API changes

Drupal\Core\Entity\ContentEntityConfirmFormBase and Drupal\Core\Entity\EntityDeleteFormTrait will be removed.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because we're just removing duplicated code.
Issue priority Not critical because the current duplicated code works, but the class hierarchy is very hard to understand.
Prioritized changes Followup to a critical (#2486177: Deleting an entity translation from the UI deletes the whole entity).
Disruption Small disruption for contributed and custom modules because it will require an internal refactoring to not use ContentEntityConfirmFormBase anymore.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

plach’s picture

amateescu’s picture

Issue summary: View changes
Priority: Normal » Major
Status: Active » Needs review
FileSize
16.54 KB
55.54 KB

While I was working on #2486177: Deleting an entity translation from the UI deletes the whole entity, I had the same feeling that EntityDeleteFormTrait is not really useful anymore since we're overriding most of its methods, and I actually started to refactor the code but the change ended up too big and out of scope for that issue. In short, big +1 to this issue :)

Here's a first pass at it. The do-not-test patch is the one that should be reviewed but I'm also posting one combined with #2486177-144: Deleting an entity translation from the UI deletes the whole entity to see where we stand with the tests.

amateescu’s picture

Issue summary: View changes

Status: Needs review » Needs work

The last submitted patch, 2: 2491057-combined.patch, failed testing.

Berdir’s picture

Bye, bye EntityDeleteFormTrait, you had a short but active live :)

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.

Arla’s picture

Issue tags: +Needs reroll

Not sure if this issue is still relevant.

I'm bothered that ContentEntityConfirmFormBase skips validation to make deletion easier, but does not defuse the validationRequired flag of the entity, so any non-deleting, entity-editing subclass must do that itself. The flag was introduced by #2453175: Remove EntityFormInterface::validate() and stop using button-level validation by default in entity forms.

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

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should 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.

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

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should 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.

pk188’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
16.44 KB

Re rolled.

pk188’s picture

Status: Needs review » Needs work

The last submitted patch, 11: 2491057-11-combined.patch, failed testing.

pk188’s picture

Status: Needs work » Needs review
FileSize
17.23 KB

Status: Needs review » Needs work

The last submitted patch, 13: 2491057-13-combined.patch, failed testing.
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Berdir’s picture

+++ /dev/null
@@ -1,122 +0,0 @@
-
-/**
- * Provides a generic base class for an entity-based confirmation form.
- */
-abstract class ContentEntityConfirmFormBase extends ContentEntityForm implements ConfirmFormInterface {

We can't just remove classes, we have to keep them as @deprecated now.

We're also missing more logic to change how the entity is build (actually we might want to avoid "building" the entity in confirm forms entirely, but not sure if that's another BC break, likely is.

harsha012’s picture

Status: Needs work » Needs review
FileSize
11.19 KB

As per #15 i have depreciated the class (https://www.drupal.org/core/deprecation) keeping in the mind of BC policy.
please validate the patch

harsha012’s picture

FileSize
11.66 KB

some text changes

The last submitted patch, 16: 2491057-16.patch, failed testing.
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Status: Needs review » Needs work

The last submitted patch, 17: 2491057-17.patch, failed testing.

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

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should 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.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should 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.

apaderno’s picture

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

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.

fenstrat’s picture

Here's a reroll of #17 with updated deprecation messages.

Status: Needs review » Needs work

The last submitted patch, 24: 2491057-24.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

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.

mglaman’s picture

phpstan-drupal added a rule to check when extending @internal code, and this is coming up. The issue seems to have atrophied, and wondering if #24 (reroll of #17) is still the correct approach and just needs to be fixed up to pass CI

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.

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

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

jweowu’s picture

I encountered this in our local CI pipeline. PHPStan was telling me this:

Class Drupal\foo\Form\FooDeleteForm extends
@internal class Drupal\Core\Entity\ContentEntityDeleteForm.
💡 Extend \Drupal\Core\Entity\ContentEntityConfirmFormBase.
See https://www.drupal.org/node/2491057

AFAICS suggesting that a delete form should be extending ContentEntityConfirmFormBase instead of ContentEntityDeleteForm is blatantly wrong, and PHPStan should not be recommending this and linking to this issue when the issue is unresolved.

Does anyone know specifically where this test and recommendation is implemented? It should surely be removed.

In the interim, I've told PHPStan to ignore this, like so:

/**
 * ...
 *
 * @phpstan-ignore-next-line
 */
class FooDeleteForm extends ContentEntityDeleteForm {
ravi.shankar’s picture

Status: Needs work » Needs review
FileSize
10.56 KB
10.7 KB

Added reroll of patch #24 on Drupal 10.1.x. and addressed Drupal CS issues of the patch as well.

apaderno’s picture

Status: Needs review » Needs work
pradhumanjain2311’s picture

Status: Needs work » Needs review
FileSize
10.58 KB
688 bytes

Try to fix PHPStan errors.

Anchal_gupta’s picture

I have fixed CS error.

apaderno’s picture

Status: Needs review » Needs work
fenstrat’s picture

A re-roll of #38 with updated deprecation messages, and using the messenger trait.

apaderno’s picture

Status: Needs review » Needs work
ameymudras’s picture

Status: Needs work » Needs review
FileSize
11.84 KB
779 bytes
apaderno’s picture

Status: Needs review » Needs work
Spokje’s picture

Assigned: Unassigned » Spokje
Issue tags: +Needs change record
Spokje’s picture

Assigned: Spokje » Unassigned
mrinalini9’s picture

Status: Needs work » Needs review
FileSize
13.45 KB
2.62 KB

Fixing custom commands failure issues in patch #42, please review it.

Thanks & Regards,
Mrinalini

Status: Needs review » Needs work

The last submitted patch, 46: 2491057-46.patch, failed testing. View results

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.