Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Part of #1945406: [meta] Convert all of confirm_form() to ConfirmFormBase
There's 3 confirm_form() usages
user_admin_role_delete_confirn() - needs access controller for Role entity
user_cancel_confirm_form() & user_multiple_cancel_confirm()
Related issues
#1872870: Implement a RoleListController and RoleFormController
#1938884: Replace the fallback user listing with a list controller
#1851086: Replace admin/people with a View
Comment | File | Size | Author |
---|---|---|---|
#101 | drupal8.user-module.1946466-101.patch | 32.07 KB | jibran |
#101 | interdiff.txt | 778 bytes | jibran |
#100 | drupal8.user-module.1946466-100.patch | 32.07 KB | jibran |
#100 | interdiff.txt | 614 bytes | jibran |
#96 | drupal8.user-module.1946466-96.patch | 32.06 KB | jibran |
Comments
Comment #1
andypostinitial patch
Comment #2
andypostProper patch
Comment #4
andypostFix test for new string
Comment #6
andypostShould be green now.
Patch will require re-roll after path changes #1872870: Implement a RoleListController and RoleFormController
Comment #6.0
andypostUpdated issue summary.
Comment #7
andypostLet's wait for generic controller #1947432: Add a generic EntityAccessCheck to replace entity_page_access()
Comment #8
andypostPatch could be done without waiting unified access check and could be re-rolled after #1947892: Improve DX with EntityAccessControllerInterface
Re-roll for #1939660: Use YAML as the primary means for service registration
Next step here is convert user_cancel_confirm_form()
Comment #9
andypostInitial patch form user cancellation forms.
Seems needs common base class/interface to implement common functionality.
Also found
user_admin()
still uses$_POST
and that's data passed touser_multiple_cancel_confirm()
So needs follow-up to refactor this ugliness
Comment #10.0
(not verified) CreditAttribution: commentedUpdated issue summary.
Comment #11
tim.plunkettA lot of that is cleaned up in #1851086: Replace admin/people with a View. Perhaps this should be postponed.
Comment #12
andypostI think we should convert cancel and delete confirm forms but cancel_multiple could be done with
#1839516: Introduce entity operation providers
Comment #13
andypostFiled #1992428: Convert Roles management to a Controller to finish roles conversion after #1872870: Implement a RoleListController and RoleFormController
So probably better move role-delete form conversion in that issue and deal here with user-cancel forms
Comment #14
andypostRe-roll for current HEAD
Comment #16
jibran#14: 1946466-confirm-user-14.patch queued for re-testing.
Comment #18
andypostRe-roll after #1992428: Convert Roles management to a Controller
Comment #20
andypostfix route
EDIT still get in UserCancelTest
Fatal error: Call to a member function id() on a non-object in /home/andypost/www/core8/core/modules/user/lib/Drupal/user/Form/UserCancelConfirm.php on line 139
Comment #21
andypostIt needs to call a constructor to init protected
$this->account
variableComment #22
tim.plunkettThis looks absolutely insane. How is this okay?
Comment #23
andypostAnother approach could be implementation of userCancel() method for user entity
Also we could implement this on top of #1846172: Replace the actions API
Also related #1851086: Replace admin/people with a View
Comment #24
tim.plunkettWorking on this.
Comment #25
tim.plunkettNot 100% happy with this yet, UserCancelConfirm is weird (in HEAD too, not the fault of the previous patch author).
Comment #27
tim.plunkettBC NG OMG
Comment #28
tim.plunkettThis is unfortunate, but necessary.
Comment #29
andypostShould be ok
Comment #30
alexpottThis needs reworking due to #2011018: Reconcile entity forms and confirm forms
Comment #31
tim.plunkettThis might need more work, but here's an initial reroll.
Comment #33
tim.plunkettSome updates.
Comment #35
tim.plunkettUghh.
Comment #36
tim.plunkettOpened #2022875: Resolve difference between submitForm(), submit(), and save() in EntityFormController for that last interdiff.
Comment #38
tim.plunkett#35: user-1946466-35.patch queued for re-testing.
Comment #40
tim.plunkettReroll, injected more.
Comment #42
tim.plunkettBorrowing code from #1990544: Convert system_modules() to a Controller.
Comment #43
jibran#42: user-1946466-42.patch queued for re-testing.
Comment #45
tim.plunkettuid / id() changes
Comment #46
jibranI tested it on simpletest.me. When I tried to delete a user from user edit page it has some issue sometimes it takes me back to
admin/people
without out doing anything and sometimes it takes me to confirm from.Comment #47
tim.plunkettWhat do you mean "without doing anything"?
simplytest.me doesn't have outgoing emails enabled, which is the focus of a majority of this functionality. It works find during real testing.
Comment #48
jibran"without doing anything" mean it doesn't block or delete the user it just redirects me back to
admin/people
.Comment #49
jibran#46 is due to some existing bug in HEAD. Patch is working fine both for single and multiple users.
Thanks @tim.plunkett for helping me figuring this out and for being patient with my review.
PS: I blame the timing of my review 4:27am. :(
Comment #50
alexpottNeeds a reroll...
Comment #51
Unitoch CreditAttribution: Unitoch commentedI'm going to try rerolling this patch.
Comment #52
Unitoch CreditAttribution: Unitoch commentedHere's a rerolled version of #45 above.
Comment #53
jibranBack to RTBC
Comment #54
catchWhy isn't the translation service injected? Declaring global $user in several methods isn't great either - could that be a class property at least - the only thing needed is the uid.
Could we not resolve that here?
the own account, should be 'their own account?
Comment #55
tim.plunkettWith this patch,
user_cancel_methods()
is called from three places:\Drupal\user\Form\UserMultipleCancelConfirm
\Drupal\user\AccountSettingsForm
\Drupal\user\Form\UserCancelForm
Those three do not (and cannot) share a common parent, so where should
user_cancel_methods()
live?Comment #56
tim.plunkettSo I've continued to use t().
Fixed the rest.
I'll open a follow-up for dealing with user_cancel_methods() directly.EDIT: #2049579: Replace user_cancel_methods() with something OO
Comment #58
tim.plunkettRerolled after #2045275: Convert user properties to methods.
Comment #59
jibranWe can get
$account
from$this->request
I wish @sun can review this. This is ugly this should use
theme_item_list
.Comment #60
tim.plunkettFixed the first part.
The second one is more complex than theme_item_list, the
'#type' => 'hidden',
does a bit more.That's very much out of scope.
Comment #61
jibran#54 and #59 is addressed. Created #2049921: Update the markup in UserMultipleCancelConfirm::buildForm() for second point in #59. Back to RTBC.
Comment #62
alexpott'account' has changed to '_account' see #2043781: Drupal::request()->attributes->get('account') may conflict with an account object loaded from the path
Comment #63
Crell CreditAttribution: Crell commentedJust a reroll for #62. No other changes.
Comment #64
alexpottShouldn't be using user_access here - it's been deprecated.
Comment #65
tim.plunkettKilled deprecated things. This didn't pass 100% for me locally, but lets see what the bot says.
Comment #67
tim.plunkett_user_mail_notify was converted to use NG users now.
Comment #68
becw CreditAttribution: becw commentedShould this hook_menu entry be deleted entirely?
Also, when I went to test the multiple-user-cancel confirm form manually, I got a PHP notice and no form:
I suspect that we should have a test for
admin/people/cancel
. I'm going to try to fix this up this afternoon.Comment #69
tim.plunkettAll of the type MENU_CALLBACK were only in hook_menu for routing and not menu links, so yes, they can be deleted outright.
If I just apply the patch and try to test, I also get that error. But clearing the cache, I don't anymore.
Comment #70
becw CreditAttribution: becw commentedAh, you're right, my bad.
drupal_set_message() probably shouldn't be used inside this class, if possible. Is there a service for this yet?
I'm guessing/hoping that updating
user_cancel_methods()
is out-of-scope for this issue.Comment #71
becw CreditAttribution: becw commentedComment #72
tim.plunkettdrupal_set_message has no equivalent yet.
user_cancel_methods is #2049579: Replace user_cancel_methods() with something OO
Comment #73
jibran#67: user-1946466-67.patch queued for re-testing.
Comment #75
jibranStraight reroll.
Comment #76
thedavidmeister CreditAttribution: thedavidmeister commentedI get Notice: Array to string conversion in form_process_checkbox() (line 3257 of core/includes/form.inc). on the create user page.
I don't think it's related to this patch though, I reverted and cleared the cache and the notice was still there #2069929: Notice: Array to string conversion in form_process_checkbox() (line 3257 of core/includes/form.inc). on create user page.
Comment #77
thedavidmeister CreditAttribution: thedavidmeister commentedI was looking over this tonight, ran out of time to properly RTBC or not, the forms do seem to be working though, which is good :)
Comment #78
jibranThanks @thedavidmeister for the review. I am going to RTBC it once more after #49, #53 and #61. I think I can RTBC it because I only rerolled it.
Comment #79
alexpottuse $this->t() from FormBase
No need to pass the request in anymore
Let's use the FormBase::getCurrentUser() method
We should be using FormBase's getRequest() method.
Let's use FormBase::getCurrentUser()
Should be inject the url generator service
We can use the _title property on the route definition and remove this completely.
Comment #80
jibranFixed #79.
Comment #82
jibranFixed Tests.
Comment #83
jibranAlso updated
UserMultipleCancelConfirm
.Comment #84
tim.plunkett#83: 1946466-83.patch queued for re-testing.
Comment #86
jibran3-way merge :D
Comment #87
jibranReroll.
Comment #88
dawehnerLet's also maybe wait until FormBase gets a currentUser method.
What about using $entity->uri()?
You could just use !$override_access
Is there a specific reason to not call parent::buildForm at the top of the parent function?
On the longrun we should store that information somewhere else, maybe in a tempstore?
Let's use entity.manager now.
There will be a patch relative soon which provides a $this- >redirect() method, so we maybe we see that one before this one is ready.
Missing _title key
Comment #89
jibranThanks @dawehner for the review.
parent::buildForm($form, $form_state);
is adding form action element I think it is ok to call it at the end.Comment #90
dawehnerGreat!
Comment #91
alexpottLets use $this->getCurrentUser() and not store $user on the object. We're creating an unnecessary dependency on buildForm() in all the other functions that use $this->user
Comment #92
disasm CreditAttribution: disasm commentedAddressing #91.
Comment #94
jibran#92: drupal8.user-module.1946466-92.patch queued for re-testing.
Comment #96
jibranFixes after #1963394: ConfirmFormBase::getCancelPath() should allow for a route and #2077513: Refactor ControllerBase to be more consistent with FormBase.
Comment #98
jibran#96: drupal8.user-module.1946466-96.patch queued for re-testing.
Comment #99
dawehnerThe documentation for two different classes should be a little bit different.
Comment #100
jibranHow about "Provides a confirmation form for cancelling multiple users account." for
UserMultipleCancelConfirm
.Comment #101
jibranFixed comment to
Provides a confirmation form for cancelling multiple user accounts.
Comment #103
dawehner#101: drupal8.user-module.1946466-101.patch queued for re-testing.
Comment #104
dawehnerThe form was tested manually and the code looks fine for me, tomorrow as well as tonight
Comment #105
webchickCommitted and pushed to 8.x. Thanks!
Comment #106
jibranYay!!! thank you everybody.
Comment #107
tstoecklerThis part looks weird to me. ConfirmFormInterface::getCancelRoute() does not document returning NULL. What is the wanted effect here? Redirecting to the front page? In that case we should return the '' route. I'm not really sure, but I think either this should be updated or the documentation should be amended in case returning NULL is a valid thing.
Comment #108
jibran@tstoeckler see #1963394-26: ConfirmFormBase::getCancelPath() should allow for a route
Comment #109
tstoecklerHmm... Thanks @jibran, but I'm afraid I don't really understand what that comment is supposed to tell me. Anyway, I found this @todo a little below the code, which probably explains this. A @todo on getCancelRoute() would have been nice, but meh...
Comment #110.0
(not verified) CreditAttribution: commentedUpdated issue summary.