Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Problem/Motivation
Right now, ConfirmFormBase is needed for a confirmation form.
There is no way to provide something like a delete form using EntityFormControllerInterface
Proposed resolution
Add EntityConfirmFormBase
Remaining tasks
User interface changes
n/a
API changes
API addition
Related Issues
Comment | File | Size | Author |
---|---|---|---|
#37 | confirm_alter-2011018-37.patch | 1.77 KB | andypost |
#36 | confirm_alter-2011018-36.patch | 1.77 KB | andypost |
#35 | confirm_alter-2011018-35.patch | 1.21 KB | tim.plunkett |
#31 | entity-confirm-2011018-31.patch | 115.32 KB | tim.plunkett |
#31 | interdiff.txt | 989 bytes | tim.plunkett |
Comments
Comment #1
tim.plunkettWe have no interface called ConfirmFormInterface, since everything unique to ConfirmFormBase is protected.
So these classes are not related in any technical way.
One solution is to make the methods public. They all return strings. It's not really important if these are called or not.
Leaving out that switch to keep it reviewable.
This adds the new base class and does one conversion (View delete form).
Comment #2
tim.plunkettHere it is with the interface.
Comment #3
ParisLiakos CreditAttribution: ParisLiakos commentedoverall i like this:) one more DX win!
Since we are here..
If we are depending on Request why dont we have it as argument on buildForm() and store it in a property?
Like we did for ban module..
Unless of course its not possible for confirm forms yet?
Comment #4
tim.plunkettSure why not!
Comment #6
tim.plunkettMissed a use statement.
Comment #8
tim.plunkettWell that would mean reimplementing all of the forms. That can be a follow-up and we can commit the patch in #2, or we could do it all here.
This is a start, there are still a half dozen forms left.
Comment #9
tim.plunkettComment #11
tim.plunkettLast couple.
Because of inheritance, I had to add both EntityConfirmFormBase and EntityNGConfirmFormBase, so the diffstat is a bit lopsided. It won't be that bad in the end.
Comment #13
tim.plunkettI missed the ones not called "Delete" :)
Comment #15
tim.plunkettAlright, fixed those last test failures.
Comment #16
ParisLiakos CreditAttribution: ParisLiakos commentedmy english are not perfect but i think its: *a* generic
Comment #17
ParisLiakos CreditAttribution: ParisLiakos commentedhmm i can see why you do this, but maybe there is a reason it was like that?
this could use a comment
since we are touching those lets order them alphabetically
Comment #18
tim.plunketthook_entity_info_alter() is used to override other set values, hook_entity_info() is used to provide new values for other entity types.
Views UI does this for Views.
Fixed the other 3 things.
Comment #19
ParisLiakos CreditAttribution: ParisLiakos commentedgreat..only minus here is that we are duplicating logic between entity and entityNG forms, but one is to be axed anyway..so marking rtbc, i am sure bot will have no problems
great job, thanks!
Comment #21
tim.plunkettRerolled after #1987668: Convert config_test module to routes
Comment #22
ParisLiakos CreditAttribution: ParisLiakos commentedthe interdiff shows how many less lines you have to write now for an entity delete confirm form:)
Comment #23
catchI don't think we should change the methods from protected to public. Why not an empty interface if necessary? What's the problem with just having the protected method on the abstract class - that's what abstract classes are good at - enforcing internal implementation rather than public API.
Comment #24
tim.plunkettThe patch in #1 does what I think you're asking.
Except then you end up with two (three counting NG) base classes, ConfirmFormBase and EntityConfirmFormBase with the same protected methods and the same docs and the same functionality, but with nothing shared between.
Nothing needs to check the interface, so an empty one would be useless.
I discussed this issue at length with msonnabaum, and he agreed making them public is the best we can do without multiple inheritance.
Tentatively re-RTBC-ing.
Comment #25
tim.plunkettReroll for #1998140: Remove backward compatible ControllerInterface
Comment #26
tim.plunkettRerolled for #1893772: Move entity-type specific storage logic into entity classes.
Comment #28
tim.plunkett#26: entity-confirm-form-2011018-26.patch queued for re-testing.
Comment #29
ParisLiakos CreditAttribution: ParisLiakos commentedComment #30
alexpottCouple of minor nits
The implement does not return $this and doing so would be unnecessary.
Talked about this with @timplunkett and he said we should be calling parent::form() here... probably before we add $form['replacement'].
Comment #31
tim.plunkettNice catch on the @return self.
We do want to call parent::form(), but still at the end. It adds in default values for stuff like langcode, and we've been calling it afterward in other places.
Comment #32
dawehnerThese chances are fine!
Comment #33
alexpottCommitted 73fbcf8 and pushed to 8.x. Thanks!
Need to review existing change records to see if any need an update.
Comment #34
andypostThere's some unpredictable behavior detected that needs follow-up #1946456-55: Convert taxonomy_term_confirm_delete() and taxonomy_vocabulary_confirm_delete() to the new form interface
Comment #35
tim.plunkettWhat about this, to ensure they're not caught by BASE_FORM_ID_alter?
Comment #36
andypostI think better make a name '_confirm_form' suffix.
It works fine now but there's no ability to pass anything from container to form
Comment #37
andypostRemoved useless spaces.
Change notice should point about that alter should use
hook_form_$entitytype_confirm_form_alter
Comment #38
tim.plunkettThanks @andypost, sorry for the temporary regression.
Comment #39
alexpottCommitted fd5f765 and pushed to 8.x. Thanks!
Comment #40
tim.plunkettComment #41
jibranAs per confirm_form() deprecated in favor of \Drupal\Core\Form\ConfirmFormBase form controller should extend
Drupal\Core\Form\ConfirmFormBase
.If confirm form is used to provide some kind of entity function then form controller should extend
Drupal\Core\Entity\EntityConfirmFormBase
see class VocabularyDeleteForm for further details.For entity confirm form routing.yml entry should look like this
see taxonomy.routing.yml for further details.
As this is entity form
_entity_form: 'taxonomy_vocabulary.delete'
we have to add form classDrupal\taxonomy\Form\VocabularyDeleteForm
to entity info using@EntityType
or hook_entity_info or hook_entity_info_alter.See Core/Entity/Vocabulary.php
"delete" = "Drupal\taxonomy\Form\VocabularyDeleteForm"
for@EntityType
entry or action_entity_info$entity_info['action']['controllers']['form']['delete'] = 'Drupal\action\Form\ActionDeleteForm';
forhook_entity_info_alter
entry.Comment #42
catch#2083415: [META] Write up all outstanding change notices before release.
Comment #43
xjmTagging "Missing change record", as completing the change record updates here blocks a beta: #2083415: [META] Write up all outstanding change notices before release
The patch for this issue was committed June 17, 2013, meaning that the change record updates for this issue have been outstanding for over seven months. Someone knowledgeable about the issue (cough @tim.plunkett) should review the draft @jibran has provided and ensure that the relevant change records get updated.
Comment #44
balagan CreditAttribution: balagan commentedThis issue needs work, either because of jibran's comment: https://drupal.org/comment/7683993#comment-7683993, if not that, then the change notice should be written. So changing status accordingly.
Comment #45
ParisLiakos CreditAttribution: ParisLiakos commentedI added an example for EntityConfirmFormBase in https://drupal.org/node/1945416
since the regression with form ids is fixed, i dont think there is something more needed here
Comment #46
jibran@balagan it was NR because in #41 I purposed the change record draft. Issue is was fixed a long ago.