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 withEntityDeleteFormTrait
at this point (when we're overriding it so much) and just put the respective versions of the methods inContentEntityDeleteForm
andEntityDeleteForm
(which are the only usages). (For others' reference,ContentEntityDeleteForm
does not extendEntityDeleteForm
; andEntityDeleteForm
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 solutionWrite 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
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. |
Comment | File | Size | Author |
---|---|---|---|
#46 | interdiff_42-46.txt | 2.62 KB | mrinalini9 |
#46 | 2491057-46.patch | 13.45 KB | mrinalini9 |
#42 | interdiff_40_42.txt | 779 bytes | ameymudras |
#42 | 2491057-42.patch | 11.84 KB | ameymudras |
#40 | 2491057-interdiff-38-40.txt | 3.21 KB | fenstrat |
Comments
Comment #1
plachComment #2
amateescu CreditAttribution: amateescu as a volunteer commentedWhile 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.
Comment #3
amateescu CreditAttribution: amateescu as a volunteer commentedComment #5
BerdirBye, bye EntityDeleteFormTrait, you had a short but active live :)
Comment #7
ArlaNot 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.
Comment #10
pk188 CreditAttribution: pk188 at OpenSense Labs commentedRe rolled.
Comment #11
pk188 CreditAttribution: pk188 at OpenSense Labs commentedComment #13
pk188 CreditAttribution: pk188 at OpenSense Labs commentedComment #15
BerdirWe 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.
Comment #16
harsha012 CreditAttribution: harsha012 as a volunteer and at Red Crackle commentedAs per #15 i have depreciated the class (https://www.drupal.org/core/deprecation) keeping in the mind of BC policy.
please validate the patch
Comment #17
harsha012 CreditAttribution: harsha012 as a volunteer and at Red Crackle commentedsome text changes
Comment #22
apadernoComment #24
fenstratHere's a reroll of #17 with updated deprecation messages.
Comment #31
mglamanphpstan-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
Comment #34
jweowu CreditAttribution: jweowu at Catalyst IT commentedI encountered this in our local CI pipeline. PHPStan was telling me this:
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:
Comment #35
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedAdded reroll of patch #24 on Drupal 10.1.x. and addressed Drupal CS issues of the patch as well.
Comment #36
apadernoComment #37
pradhumanjain2311 CreditAttribution: pradhumanjain2311 at OpenSense Labs for DrupalFit commentedTry to fix PHPStan errors.
Comment #38
Anchal_gupta CreditAttribution: Anchal_gupta at Srijan | A Material+ Company for Drupal India Association commentedI have fixed CS error.
Comment #39
apadernoComment #40
fenstratA re-roll of #38 with updated deprecation messages, and using the messenger trait.
Comment #41
apadernoComment #42
ameymudras CreditAttribution: ameymudras at Salsa Digital commentedComment #43
apadernoComment #44
SpokjeComment #45
SpokjeComment #46
mrinalini9 CreditAttribution: mrinalini9 at Srijan | A Material+ Company for Drupal India Association commentedFixing custom commands failure issues in patch #42, please review it.
Thanks & Regards,
Mrinalini