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.
CommentFileSizeAuthor
#46 interdiff_42-46.txt2.62 KBmrinalini9
#46 2491057-46.patch13.45 KBmrinalini9
#42 interdiff_40_42.txt779 bytesameymudras
#42 2491057-42.patch11.84 KBameymudras
#40 2491057-interdiff-38-40.txt3.21 KBfenstrat
#40 2491057-40.patch11.26 KBfenstrat
#38 interdiff-2491057-37_38.txt695 bytesanchal_gupta
#38 2491057-38.patch11.25 KBanchal_gupta
#37 interdiff_35-37.txt688 bytespradhumanjain2311
#37 2491057-37.patch10.58 KBpradhumanjain2311
#35 reroll_diff_24-35.txt10.7 KBravi.shankar
#35 2491057-35.patch10.56 KBravi.shankar
#24 interdiff-2491057-17-24.txt2.63 KBfenstrat
#24 2491057-24.patch11.47 KBfenstrat
#17 2491057-17.patch11.66 KBharsha012
#16 2491057-16.patch11.19 KBharsha012
#13 2491057-13-combined.patch17.23 KBpk188
#11 2491057-11-combined.patch17.86 KBpk188
#10 2491057-10-do-not-test.patch16.44 KBpk188
#2 2491057-combined.patch55.54 KBamateescu
#2 2491057-do-not-test.patch16.54 KBamateescu

Issue fork drupal-2491057

Command icon 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:

About issue forks

Comments

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
commented
Parent issue: » #2486177: Deleting an entity translation from the UI deletes the whole entity
amateescu’s picture

amateescu commented
Issue summary: View changes
Priority: Normal » Major
Status: Active » Needs review
StatusFileSize
new 2491057-do-not-test.patch16.54 KB
new 2491057-combined.patch55.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

amateescu commented
Issue summary: View changes

Status: Needs review » Needs work

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

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

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

arla commented
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

pk188 commented
Status: Needs work » Needs review
Issue tags: -Needs reroll
StatusFileSize
new 2491057-10-do-not-test.patch16.44 KB

Re rolled.

pk188’s picture

pk188 commented
StatusFileSize
new 2491057-11-combined.patch17.86 KB

Status: Needs review » Needs work

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

pk188’s picture

pk188 commented
Status: Needs work » Needs review
StatusFileSize
new 2491057-13-combined.patch17.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

berdir
Primary language German
Location Switzerland
commented 
+++ /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

harsha012 commented
Status: Needs work » Needs review
StatusFileSize
new 2491057-16.patch11.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

harsha012 commented
StatusFileSize
new 2491057-17.patch11.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.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
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

fenstrat
he/him
Primary language English
Location Australia
commented
Status: Needs work » Needs review
StatusFileSize
new 2491057-24.patch11.47 KB
new interdiff-2491057-17-24.txt2.63 KB

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

mglaman
Primary language English
Location WI, USA
commented

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

jweowu commented

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

ravi.shankar commented
Status: Needs work » Needs review
StatusFileSize
new 2491057-35.patch10.56 KB
new reroll_diff_24-35.txt10.7 KB

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

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Status: Needs review » Needs work
pradhumanjain2311’s picture

pradhumanjain2311 commented
Status: Needs work » Needs review
StatusFileSize
new 2491057-37.patch10.58 KB
new interdiff_35-37.txt688 bytes

Try to fix PHPStan errors.

anchal_gupta’s picture

anchal_gupta commented
StatusFileSize
new 2491057-38.patch11.25 KB
new interdiff-2491057-37_38.txt695 bytes

I have fixed CS error.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Status: Needs review » Needs work
fenstrat’s picture

fenstrat
he/him
Primary language English
Location Australia
commented
Status: Needs work » Needs review
StatusFileSize
new 2491057-40.patch11.26 KB
new 2491057-interdiff-38-40.txt3.21 KB

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

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Status: Needs review » Needs work
ameymudras’s picture

ameymudras commented
Status: Needs work » Needs review
StatusFileSize
new 2491057-42.patch11.84 KB
new interdiff_40_42.txt779 bytes
avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Status: Needs review » Needs work
spokje’s picture

spokje
Primary language Dutch
commented
Assigned: Unassigned » spokje
Issue tags: +Needs change record
spokje’s picture

spokje
Primary language Dutch
commented
Assigned: spokje » Unassigned
mrinalini9’s picture

mrinalini9 commented
Status: Needs work » Needs review
StatusFileSize
new 2491057-46.patch13.45 KB
new interdiff_42-46.txt2.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.

olmyr made their first commit to this issue’s fork.

xjm’s picture

xjm
she/her
Primary language English
commented

Amending attribution.

xjm’s picture

xjm
she/her
Primary language English
commented

We do need an actual merge request here. :) It should also address #34 and #15; please don't just reroll old patches as MRs without looking at their content and comparing it with the reviews. Thanks!

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Issue tags: +Needs merge request

mrinalini9 opened merge request !12550

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.