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.
Comments
Comment #1
nick_schuch CreditAttribution: nick_schuch commentedComment #2
nick_schuch CreditAttribution: nick_schuch commentedWent to test this and in the process discovered this issue: http://drupal.org/node/1980338. Applied to get around it for now. But need to keep this issue in mind.
Comment #3
jsacksick CreditAttribution: jsacksick commentedI worked a bit on this, I have issues with the Access class (InvalidArgumentException: No check has been registered for access_check.system in Drupal\Core\Access\AccessManager->loadCheck()), apart from that the controller seems to work fine.
Comment #4
jsacksick CreditAttribution: jsacksick commentedActually, it works fine, I had to put back
system_plugin_ui_access()
because it's still used by other menu items.Comment #5
nick_schuch CreditAttribution: nick_schuch commentedComment #6
nick_schuch CreditAttribution: nick_schuch commentedComment #7
dawehnerWhat about name it SystemPluginUICheck and using _access_system_plugin_ui ? This will not only used for the autocompletion controller, as you realized.
Similar to the controller you should also inject the plugin manager in here. What you can do is using "arguments: ['@plugin.manager.system.plugin_uid']" in the services file.
just a nitpick: pluginUIManager.
This should be Unicode::strtolower instead.
Mhhh there should be a better way for this, but I don't know of one, so this controller has to be containeraware.
if there is just a type MENU_CALLBACK in hook_menu you can drop it from there.
Comment #8
jsacksick CreditAttribution: jsacksick commentedComment #9
jsacksick CreditAttribution: jsacksick commentedComment #10
jsacksick CreditAttribution: jsacksick commentedRerolled patch against the latest dev
Comment #12
jsacksick CreditAttribution: jsacksick commentedOkay, the ControllerInterface has moved, here's a new version, I also removed the use of drupal_explode_tags.
Comment #13
ParisLiakos CreditAttribution: ParisLiakos commentedthis should be named
pluginUiManager
see https://drupal.org/node/608152#naming point 3
SystemPluginUiCheck
as wellThis should be probably be typehinted with PluginManagerInterface
Comment #14
jsacksick CreditAttribution: jsacksick commentedComment #16
jsacksick CreditAttribution: jsacksick commentedSame patch with reordered alphabetically use statements in the SystemController class.
Comment #18
jsacksick CreditAttribution: jsacksick commented#16: drupal-convert-system-plugin-autocomplete-1987826-15.patch queued for re-testing.
Comment #20
jsacksick CreditAttribution: jsacksick commented#16: drupal-convert-system-plugin-autocomplete-1987826-15.patch queued for re-testing.
Comment #22
jsacksick CreditAttribution: jsacksick commentedComment #23
jsacksick CreditAttribution: jsacksick commentedFor some reasons, I had case sensitivity issues, sorry for the noise.
Comment #24
ParisLiakos CreditAttribution: ParisLiakos commentedthanks, looking pretty much ready, but one thing:
i believe we can totally remove this entry from hook_menu, since this is a MENU_CALLBACK
Comment #25
jsacksick CreditAttribution: jsacksick commentedThanks for the active review, here is the new patch.
Comment #26
dawehnerMissing starting "\"
Access checkers have to return static::DENY and static::ALLOW or they don't work as expected.
Instead of using Drupal::service this controller should be marked as containeraware.
Comment #27
jsacksick CreditAttribution: jsacksick commentedComment #28
dawehnerThis is looking perfect now. Thank you very much. (hopefully the test pass).
Comment #29
dawehnerThis is looking perfect now. Thank you very much. (hopefully the test pass).
Comment #31
jsacksick CreditAttribution: jsacksick commentedHopefully, this one will pass...
Comment #32
dawehnerGreat
Comment #33
alexpottCommitted 09f10ef and pushed to 8.x. Thanks!
Comment #34
tstoecklerI think this should now use the appliesTo() method provided by StaticAccessCheckInterface. Should we do this here or should I open a new issue?