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 #1971384: [META] Convert page callbacks to controllers
For instructions on how to convert a page callback into a controller, see the WSCCI Conversion Guide.
Comment | File | Size | Author |
---|---|---|---|
#177 | 1978976-177.patch | 1.88 KB | lussoluca |
#173 | interdiff.txt | 721 bytes | kim.pepper |
#173 | 1978976-shortcut-set-controller-173.patch | 24.61 KB | kim.pepper |
#171 | interdiff.txt | 1.3 KB | kim.pepper |
#171 | 1978976-shortcut-set-controller-171.patch | 24.46 KB | kim.pepper |
Comments
Comment #1
kgoel CreditAttribution: kgoel commentedComment #2
kgoel CreditAttribution: kgoel commentedI am not sure about construct method but uploading patch so someone can review the patch and guide me in the right direction.
Thanks
Comment #4
kgoel CreditAttribution: kgoel commentedComment #6
kgoel CreditAttribution: kgoel commentedComment #8
Crell CreditAttribution: Crell commentedI'm pretty sure we can inject the entity manager via create/construct so we don't need this function anymore.
current_path() shouldn't be used anymore. Instead, buildForm() should take Request $request as an additional parameter, and save that object to a property. Then here you can do $this->request->attributes->get('system_path').
(Note: Tim Plunkett may disagree with me here; there's some weirdness around forms and caching that limits their injectability at the moment.)
Comment #9
kgoel CreditAttribution: kgoel commentedI have added entity manager but not sure if I need to remove this function -
Also, not sure how to do this. Is there any example, I can follow or documentation will help.
Comment #11
Crell CreditAttribution: Crell commentedLine break before the @param.
Modify this to take a Request $request parameter (you'll need another use statement above). Then $this->currentPath = $request->attributes->get('system_path');
Add a second parameter to this call, $container->get('request'). (This is not a great way of doing it, but the alternative doesn't work on forms yet.)
Also, no space after static.
I think check_plain() now has a static method you can call instead, on the String:: class. (I don't recall if that landed yet or not.)
Now replace this line with url($this->currentPath) instead. (The generator isn't quite here yet for the injected version, so url() is still necessary.)
Also, there's some trailing whitespace to clean up. dreditor will highlight it for you.
Comment #12
kgoel CreditAttribution: kgoel commentedThanks Crell!
I have checked core/modules/action/lib/Drupal/action/Controller/ActionController.php and it seems check_plain is still being used.
Comment #14
kgoel CreditAttribution: kgoel commentedComment #16
kgoel CreditAttribution: kgoel commentedComment #18
dawehnerYou probably want to store the actual manager here :) $this->entityManger = $entity_manager
Then use $this->entityManager->getStorageController('shortcut')->load() .. Personally I prefer to just store the storage controller and not the full entity manager
This should by on the validateForm and submitForm method instead.
entity_create/shortcut_set_load could also just use the entity storage controller.
Comment #19
kgoel CreditAttribution: kgoel commentedI have addressed some of the issues under comment #18. Need to look at some doc as how to add validate and submit form method.
Comment #20
kgoel CreditAttribution: kgoel commentedTook care of this. This patch addressed all the issues.
Comment #21
kgoel CreditAttribution: kgoel commentedComment #22
Crell CreditAttribution: Crell commentedThere is no $request in this scope, so this line will fail.
For a controller, you never need the request in the constructor. Instead, you can get it passed into the controller method itself.
I don't think this line is right... There is no request entity for you to get...
If you put Request $request = NULL at the end of this method's parameters, you'll get the request object passed to you directly. No need for it in the constructor.
Comment #23
Crell CreditAttribution: Crell commentedComment #24
kgoel CreditAttribution: kgoel commentedNeed some clarification on the following -
$this->currentPath = $request->attributes->get('system_path');
Looking at form interface api (submit form) it takes only two arguments and so not sure how can I pass request argument in the controller method. That's why I passed request argument in the constructor method.
Comment #25
Crell CreditAttribution: Crell commentedActually what I said before is blocked on #1998166: Use the controller resolver to inject parameters into HtmlFormController.
Once that's in, the buildForm() method will work just like any other controller method. As long as parameters you add after the first two are nominally optional (ie, have a default defined), they still will pass the interface and will get parameters passed to them by name just like controllers, including the Request object.
Comment #26
kgoel CreditAttribution: kgoel commentedComment #27
dawehnerit should be $sets = $this->entityManager ....
You could also load a single shortcut if you use the storage controller.
Comment #29
Crell CreditAttribution: Crell commented#26: convert_shortcut_set_switch-26.patch queued for re-testing.
Comment #30
dawehnerComment #31
kgoel CreditAttribution: kgoel commentedComment #32
dawehnerno need for this line.
... should also use the storage controller method.
Comment #33
kgoel CreditAttribution: kgoel commentedI am not sure how to use storage controller here.
Comment #34
tim.plunkettThis needs a custom access checker, "switch" is not a standard entity operation. For now I've made it _access: 'TRUE'.
The interdiff is with whitespace changes ignored, the validate and submit methods were indented wrong.
Comment #35
dawehnerManual testing works pretty fine. So here is incredible nitpick.
There is a space missing here.
Comment #36
kgoel CreditAttribution: kgoel commentedComment #38
kgoel CreditAttribution: kgoel commented#36: shortcut-1978976-36.patch queued for re-testing.
Comment #40
Crell CreditAttribution: Crell commented#36: shortcut-1978976-36.patch queued for re-testing.
Comment #42
dawehnerThis should fix most of the errors.
Comment #44
kgoel CreditAttribution: kgoel commentedComment #45
dawehnerThere we go.
Comment #46
tim.plunkettUnrelated to this issue, should this be a method on the storage controller? Or a static method on the entity class?
Now that the storage controller has an assignUser() method, we should use that
This needs to be fixed
Comment #47
dawehnerYeah either on the entity class or some other service (the storage controller should have different logic).
Comment #48
dawehnerReplaced these two methods on the storage controller but we could put even more on the longrun.
Comment #50
dawehnerLet's not remove the used method.
Comment #52
dawehnerThis should work now.
Comment #54
dawehnerComment #55
Crell CreditAttribution: Crell commentedI think this can be simplified. The administer marker can be moved to its own _permission check. Then this checker can just check "does this user have a permission AND is editing their own account" ? ALLOW : DENY.
Then we switch the access mode to ANY. That makes it a good example of how "admin override permissions" can work for other routes.
If we're accepting $_account as a parameter then we don't need global $user. They're the same object. (===)
Oh this is so confusing. $_account vs. $account? I have no idea which is which here. These need better names.
We don't need loadMultiple() here. A single load() will do since we're only looking for one item.
Oh yikes. That's right, $account is what we call the active account, not $_account. This is again seriously confusing. Honestly right now I don't even know which variable is which anymore in this patch, which is a bad sign. :-) All of these need better names.
An underscored variable in the pattern is rarely appropriate.
Comment #56
dawehnerOkay, let's rename _account to user, as that is what it is. It is a user entity.
Comment #57
Crell CreditAttribution: Crell commentedI'm a little concerned about this todo. Should we investigate further? When might it not exist?
Otherwise I think we're good.
Comment #58
dawehnerNo, it is known, see menu_item_route_access().
The account object should maybe copied from the parent request.
Comment #59
Crell CreditAttribution: Crell commentedFor a mocked request for menu access checking, yes, we should pass the account down through. If that resolves that todo, great. If not, we should note in the todo what circumstances that would be, even if it's just a @see menu_item_route_access().
Comment #60
dawehnerLet's wait until #2036629: 'shortcut' entity type is very confusing and should be renamed to 'shortcut_set' is in.
Comment #61
dawehnerRerolled.
Comment #62
Crell CreditAttribution: Crell commentedShould this be using the static check then?
$GLOBALS why?
(Maybe we should let #2062151: Create a current user service to ensure that current account is always available get committed, then use that.)
Seems OK otherwise.
Comment #63
dawehnerYeah, we can do it properly now.
Comment #65
dawehnerThere we go
Comment #67
dawehner#65: shortcut-1978976-65.patch queued for re-testing.
Comment #69
pfrenssenFound some nitpicks:
Why is this called SetSwitchAccessCheck instead of ShortcutSetSwitchAccessCheck? What is a setswitch?
Parameter is not documented.
This todo may be removed.
ShortcutSetSwitch better describes what this is about.
Missing period.
Ends in a semicolon instead of a period.
Comment #70
dawehnerRenamed it to SwitchShortcutSetAccessCheck which for sure gives way more context.
Ups!
Renamed to SwitchShortcutSet
Some additional changes here and there.
Comment #71
tim.plunkettAre we actually doing this now? I know most other access checkers still do $request->attributes->get('_account'), I was thinking we'd start passing $account to access checkers when we want to phase out _account.
#2073813: Add a UrlGenerator helper to FormBase and ControllerBase is close, keep that in mind. Also this should really be "The URL generator."
$account = $this->currentUser();
$this->getRequest(), we should never rely on $this->request
EntityDisplayModeFormBase, FilterFormatFormControllerBase, and DateFormatFormBase all use entity.query for this, since we don't want the loading.
Comment #73
dawehnerWhatever you think is the way to get this patch in.
So let's not deal with injection then.
I don't even get these failures and for sure they pass locally as every good test.
Comment #75
dawehnerJust yet another rerole.
Comment #77
dawehner#75: shortcut_set-1978976-75.patch queued for re-testing.
Comment #78
Crell CreditAttribution: Crell commentedLet's just get this in; we need to get the conversions out of the way.
Comment #79
alexpott@crell are you sure? You are rejecting this change over in #2076411: Remove the request scope from the current user service
Comment #80
tim.plunkettHeh. Yeah, this and many other conversions are blocked until that change is made.
Comment #81
xjmThanks for your work on this issue! Please see #1971384-43: [META] Convert page callbacks to controllers for an update on the routing system conversion process.
Comment #82
jibran#75: shortcut_set-1978976-75.patch queued for re-testing.
Comment #84
h3rj4n CreditAttribution: h3rj4n commentedNeeds reroll
Comment #85
dawehnerRerolled ... I moved some of the existing services around as
Comment #86
tim.plunkettWe use _access_* in most other places, why the switch?
So this is ALL, is it worth specifying for when the default switch happens?
Comment #87
dawehnerI don't care ... all I did was to rerole the already RTBC patch :) I do agree that for every special case where it is not obvious _access is helpful.
Some examples where the behavior is obvious: _permission, _role.
Good catch.
Comment #88
googletorp CreditAttribution: googletorp commentedThis looks good to me.
Comment #89
tim.plunkettThis can't go in as part of this.
Comment #90
alexpottPatch no longer applies.
Comment #91
David Hernández CreditAttribution: David Hernández commentedQuick reroll
Comment #93
dawehnerJust did a reroll.
Comment #94
dawehner... sorry but #91 seems wrong anyway.
Comment #95
vijaycs85Comment #97
David Hernández CreditAttribution: David Hernández commented#93: shortcut-1978976-91.patch queued for re-testing.
Comment #99
tim.plunkett#2107137: Fix the DX for declaring custom access checkers went in, so this can be cleaned up.
Comment #100
jibranI don't think anything left here. Coding review is fine so RTBC.
Comment #101
alexpottPatch no longer applies.
Comment #102
dawehnerThere we go.
Comment #103
jibranBack to RTBC
Comment #104
tim.plunkettReroll.
Comment #105
Xano104: shortcut-1978976-104.patch queued for re-testing.
Comment #106
Xano104: shortcut-1978976-104.patch queued for re-testing.
Comment #108
XanoRe-roll.
Comment #110
Xano108: drupal_1978976_108.patch queued for re-testing.
Comment #111
Crell CreditAttribution: Crell commentedRollin' rollin' rollin'...
Comment #112
webchickSorry, no longer applies.
Comment #113
jibranconflict with #2131851: Form errors must be specific to a form and not a global.
Comment #115
jibran113: 1978976-113.patch queued for re-testing.
Comment #117
jibranNon-pass
Comment #118
jibran113: 1978976-113.patch queued for re-testing.
Comment #120
jibran113: 1978976-113.patch queued for re-testing.
Comment #121
webchick113: 1978976-113.patch queued for re-testing.
Comment #122
tim.plunkettRerolled.
Comment #124
tim.plunkettDidn't reroll properly after #2021779: Decouple shortcuts from menu links.
Comment #126
dawehnerI realized that core is currently broken in the sense that creating a new shortcut set for a user does not add the default shortcut links.
Comment #127
pfrenssenStraight reroll.
Comment #129
mr.baileys127: shortcut_set-1978976-127.patch queued for re-testing.
Comment #131
Xano127: shortcut_set-1978976-127.patch queued for re-testing.
Comment #133
InternetDevels CreditAttribution: InternetDevels commentedNew reroll.
Comment #134
Albert Volkman CreditAttribution: Albert Volkman commentedReroll
Comment #135
dawehnerI don't really like to move that away from its own access check service what yeah this is fine. Note: Technically these values aren't booleans.
Comment #136
alexpottdrupal-convert_shortcut_set_to_controller-1978976-134.patch no longer applies.
Comment #137
longwaveRerolled including change from #2203407: Replace #attached library array values with provider-namespaced strings
Comment #139
longwaveReuploading, #137 failure is evidence for #2188897: Intermittent fails in Drupal\views_ui\Tests\DisplayTest
Comment #140
longwaveComment #141
andypostback to rtbc
Comment #143
longwave139: 1978976-shortcut_set_switch-137.patch queued for re-testing.
Comment #145
pcambraJust a plain re roll on the yaml files, hopefully the testbot would allow us to RTBC this.
Comment #147
XanoComment #149
XanoComment #151
Xano149: drupal_1978976_149.patch queued for re-testing.
Comment #152
Crell CreditAttribution: Crell commentedIs this random patch flotsam?
This feels like it belongs on the shortcut storage controller instead. There's nothing form-specific about it.
Are you sure? I thought the current user was passed in, not the contextual user.
I don't think null is ever allowed anymore.
Shouldn't this be an AccessInterface::KILL?
Comment #153
tim.plunkett152.1 that's if you use git show/git format-patch and not git diff.
152.2 Consider opening a follow-up, but this is the same pattern used for all exists callbacks
152.5 Generally no, deny is fine I think.
Comment #154
dawehnerWhile it is TRUE that we do things wrong all over the place thought here it is used for the machine name validation BS.
Comment #156
dawehnerThis could be it.
Comment #158
vijaycs85\Drupal\shortcut\Form\SwitchShortcutSet::checkAccess is not normal access callback and doesn't have $account argument.
Comment #159
dawehnerThis is certainly good to go,
Comment #160
shivachevva CreditAttribution: shivachevva commented158: 1978976-shortcut_set_controller-158.patch queued for re-testing.
Comment #162
shivachevva CreditAttribution: shivachevva commentedUpdating with re-rolled patch. (shortcut files were moved.)
Comment #163
David Hernández CreditAttribution: David Hernández commentedGood to go, AFAIK.
Comment #164
alexpottNot used.
https://drupal.org/node/2073813 is in
Comment #165
vijaycs85Updating...
Comment #166
alexpottDoesn't the change in the interdiff mean we are missing test coverage for this?
Comment #168
dawehnerThis can't work, given that _system_path is a path and not a route name.
Comment #169
kim.pepperThis isn't necessary as the ShortcutSetStorage does this for us already.
In addition, I've made a number of changes including:
Comment #171
kim.pepperThe access check needs the shortcut set user passed in too.
Comment #173
kim.pepperSomewhere along the way, the check for
$account->hasPermission('access shortcuts')
got dropped. Re-adding.Comment #174
Crell CreditAttribution: Crell commentedThis was RTBC before, so "it compiles, ship it!"
(Yes I read the patch over and didn't notice anything obviously wrong, but didn't fine-tooth-comb it.)
Comment #175
alexpottCommitted a8a5f10 and pushed to 8.x. Thanks!
Comment #177
lussolucaThe constructor of ShortcutSetStorage should use ConfigFactoryInterface type hint instead of ConfigFactory otherwise no contrib can override the config.factory service (as webprofiler does).
Comment #178
vijaycs85Valid point. let's test it. Hope it is OK to commit as part of this issue as it is a minor change.
Comment #179
Crell CreditAttribution: Crell commentedComment #180
alexpottCommitted 41c6402 and pushed to 8.x. Thanks!