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.

Files: 
CommentFileSizeAuthor
#31 drupal-convert-system-plugin-autocomplete-1987826-31.patch7.36 KBjsacksick
PASSED: [[SimpleTest]]: [MySQL] 56,844 pass(es).
[ View ]
#27 drupal-convert-system-plugin-autocomplete-1987826-27.patch7.36 KBjsacksick
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#25 drupal-convert-system-plugin-autocomplete-1987826-25.patch7.97 KBjsacksick
PASSED: [[SimpleTest]]: [MySQL] 56,786 pass(es).
[ View ]
#23 drupal-convert-system-plugin-autocomplete-1987826-23.patch7.93 KBjsacksick
PASSED: [[SimpleTest]]: [MySQL] 56,544 pass(es).
[ View ]
#22 drupal-convert-system-plugin-autocomplete-1987826-22.patch7.9 KBjsacksick
PASSED: [[SimpleTest]]: [MySQL] 56,417 pass(es).
[ View ]
#16 drupal-convert-system-plugin-autocomplete-1987826-15.patch7.9 KBjsacksick
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#14 drupal-convert-system-plugin-autocomplete-1987826-14.patch7.9 KBjsacksick
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#12 drupal-convert-system-plugin-autocomplete-1987826-12.patch7.88 KBjsacksick
PASSED: [[SimpleTest]]: [MySQL] 56,627 pass(es).
[ View ]
#10 drupal-convert-system-plugin-autocomplete-1987826-10.patch7.62 KBjsacksick
FAILED: [[SimpleTest]]: [MySQL] 58,024 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#8 drupal-convert-system-plugin-autocomplete-1987826-5.patch7.78 KBjsacksick
PASSED: [[SimpleTest]]: [MySQL] 55,495 pass(es).
[ View ]
#4 drupal-convert-system-plugin-autocomplete-1987826-4.patch7.03 KBjsacksick
PASSED: [[SimpleTest]]: [MySQL] 55,272 pass(es).
[ View ]
#3 drupal-convert-system-plugin-autocomplete-1987826-3.patch7.35 KBjsacksick
FAILED: [[SimpleTest]]: [MySQL] 55,733 pass(es), 20 fail(s), and 662 exception(s).
[ View ]

Comments

Assigned:Unassigned» nick_schuch

Went 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.

Status:Active» Needs review
StatusFileSize
new7.35 KB
FAILED: [[SimpleTest]]: [MySQL] 55,733 pass(es), 20 fail(s), and 662 exception(s).
[ View ]

I 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.

StatusFileSize
new7.03 KB
PASSED: [[SimpleTest]]: [MySQL] 55,272 pass(es).
[ View ]

Actually, it works fine, I had to put back system_plugin_ui_access() because it's still used by other menu items.

Assigned:nick_schuch» Unassigned

Assigned:Unassigned» nick_schuch

+++ b/core/modules/system/lib/Drupal/system/Access/SystemAccessCheck.phpundefined
@@ -0,0 +1,35 @@
+ * Access check for system routes.
+ */
+class SystemAccessCheck implements AccessCheckInterface {
+
+  /**
+   * {@inheritdoc}
+   */
+  public function applies(Route $route) {
+    return array_key_exists('_access_system_plugin_autocomplete', $route->getRequirements());
+  }

What about name it SystemPluginUICheck and using _access_system_plugin_ui ? This will not only used for the autocompletion controller, as you realized.

+++ b/core/modules/system/lib/Drupal/system/Access/SystemAccessCheck.phpundefined
@@ -0,0 +1,35 @@
+    $plugin_ui = \Drupal::service('plugin.manager.system.plugin_ui')->createInstance($request->attributes->get('plugin_id'));

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.

+++ b/core/modules/system/lib/Drupal/system/Controller/SystemController.phpundefined
@@ -0,0 +1,82 @@
+  protected $pluginUiManager;

just a nitpick: pluginUIManager.

+++ b/core/modules/system/lib/Drupal/system/Controller/SystemController.phpundefined
@@ -0,0 +1,82 @@
+    $string = drupal_strtolower(array_pop($string_typed));

This should be Unicode::strtolower instead.

+++ b/core/modules/system/lib/Drupal/system/Controller/SystemController.phpundefined
@@ -0,0 +1,82 @@
+      $plugin_ui = $this->pluginUiManager->getDefinition($plugin_id);
+      $manager = \Drupal::service($plugin_ui['manager']);

Mhhh there should be a better way for this, but I don't know of one, so this controller has to be containeraware.

+++ b/core/modules/system/system.moduleundefined
@@ -1046,10 +1046,7 @@ function system_menu() {
       }
       $items['system/autocomplete/' . $plugin_id] = array(
-        'page callback' => 'system_plugin_autocomplete',
-        'page arguments' => array($plugin_id),
-        'access callback' => 'system_plugin_ui_access',
-        'access arguments' => array($plugin_id),
+        'route_name' => 'system_plugin_autocomplete',
         'type' => MENU_CALLBACK,

if there is just a type MENU_CALLBACK in hook_menu you can drop it from there.

StatusFileSize
new7.78 KB
PASSED: [[SimpleTest]]: [MySQL] 55,495 pass(es).
[ View ]

Assigned:nick_schuch» jsacksick

StatusFileSize
new7.62 KB
FAILED: [[SimpleTest]]: [MySQL] 58,024 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Rerolled patch against the latest dev

Status:Needs review» Needs work

The last submitted patch, drupal-convert-system-plugin-autocomplete-1987826-10.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new7.88 KB
PASSED: [[SimpleTest]]: [MySQL] 56,627 pass(es).
[ View ]

Okay, the ControllerInterface has moved, here's a new version, I also removed the use of drupal_explode_tags.

+++ b/core/modules/system/lib/Drupal/system/Controller/SystemController.phpundefined
@@ -0,0 +1,84 @@
+  protected $pluginUIManager;

this should be named pluginUiManager
see https://drupal.org/node/608152#naming point 3

+++ b/core/modules/system/lib/Drupal/system/Access/SystemPluginUICheck.phpundefined
@@ -0,0 +1,53 @@
+class SystemPluginUICheck implements AccessCheckInterface {

SystemPluginUiCheck as well

+++ b/core/modules/system/lib/Drupal/system/Controller/SystemController.phpundefined
@@ -0,0 +1,84 @@
+  public function __construct(PluginUIManager $plugin_ui_manager) {

This should be probably be typehinted with PluginManagerInterface

StatusFileSize
new7.9 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Status:Needs review» Needs work

The last submitted patch, drupal-convert-system-plugin-autocomplete-1987826-14.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new7.9 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Same patch with reordered alphabetically use statements in the SystemController class.

Status:Needs review» Needs work
Issue tags:-WSCCI-conversion

The last submitted patch, drupal-convert-system-plugin-autocomplete-1987826-15.patch, failed testing.

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, drupal-convert-system-plugin-autocomplete-1987826-15.patch, failed testing.

Status:Needs work» Needs review

Status:Needs review» Needs work
Issue tags:+WSCCI-conversion

The last submitted patch, drupal-convert-system-plugin-autocomplete-1987826-15.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new7.9 KB
PASSED: [[SimpleTest]]: [MySQL] 56,417 pass(es).
[ View ]

StatusFileSize
new7.93 KB
PASSED: [[SimpleTest]]: [MySQL] 56,544 pass(es).
[ View ]

For some reasons, I had case sensitivity issues, sorry for the noise.

thanks, looking pretty much ready, but one thing:

+++ b/core/modules/system/system.moduleundefined
@@ -1023,11 +1023,7 @@ function system_menu() {
       $items['system/autocomplete/' . $plugin_id] = array(
-        'page callback' => 'system_plugin_autocomplete',
-        'page arguments' => array($plugin_id),
-        'access callback' => 'system_plugin_ui_access',
-        'access arguments' => array($plugin_id),
-        'type' => MENU_CALLBACK,
+        'route_name' => 'system_plugin_autocomplete',
       );

i believe we can totally remove this entry from hook_menu, since this is a MENU_CALLBACK

StatusFileSize
new7.97 KB
PASSED: [[SimpleTest]]: [MySQL] 56,786 pass(es).
[ View ]

Thanks for the active review, here is the new patch.

Status:Needs review» Needs work

+++ b/core/modules/system/lib/Drupal/system/Access/SystemPluginUiCheck.phpundefined
@@ -0,0 +1,53 @@
+ * Contains Drupal\system\Access\SystemPluginUiCheck.

Missing starting "\"

+++ b/core/modules/system/lib/Drupal/system/Access/SystemPluginUiCheck.phpundefined
@@ -0,0 +1,53 @@
+    return $plugin_ui->access(NULL);

Access checkers have to return static::DENY and static::ALLOW or they don't work as expected.

+++ b/core/modules/system/lib/Drupal/system/Controller/SystemController.phpundefined
@@ -0,0 +1,84 @@
+      $manager = \Drupal::service($plugin_ui['manager']);

Instead of using Drupal::service this controller should be marked as containeraware.

Status:Needs work» Needs review
StatusFileSize
new7.36 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

This is looking perfect now. Thank you very much. (hopefully the test pass).

Status:Needs review» Reviewed & tested by the community

This is looking perfect now. Thank you very much. (hopefully the test pass).

Status:Reviewed & tested by the community» Needs work

The last submitted patch, drupal-convert-system-plugin-autocomplete-1987826-27.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new7.36 KB
PASSED: [[SimpleTest]]: [MySQL] 56,844 pass(es).
[ View ]

Hopefully, this one will pass...

Status:Needs review» Reviewed & tested by the community

Great

Status:Reviewed & tested by the community» Fixed

Committed 09f10ef and pushed to 8.x. Thanks!

+++ b/core/modules/system/lib/Drupal/system/Access/SystemPluginUiCheck.php
@@ -0,0 +1,55 @@
+class SystemPluginUiCheck implements AccessCheckInterface {
...
+  public function applies(Route $route) {
+    return array_key_exists('_access_system_plugin_ui', $route->getRequirements());
+  }

I think this should now use the appliesTo() method provided by StaticAccessCheckInterface. Should we do this here or should I open a new issue?

Automatically closed -- issue fixed for 2 weeks with no activity.