Support from Acquia helps fund testing for Drupal Acquia logo

Comments

nick_schuch’s picture

Assigned: Unassigned » nick_schuch
nick_schuch’s picture

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.

jsacksick’s picture

Status: Active » Needs review
FileSize
7.35 KB

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.

jsacksick’s picture

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

nick_schuch’s picture

Assigned: nick_schuch » Unassigned
nick_schuch’s picture

dawehner’s picture

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.

jsacksick’s picture

jsacksick’s picture

Assigned: nick_schuch » jsacksick
jsacksick’s picture

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.

jsacksick’s picture

Status: Needs work » Needs review
FileSize
7.88 KB

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

ParisLiakos’s picture

+++ 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

jsacksick’s picture

Status: Needs review » Needs work

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

jsacksick’s picture

Status: Needs work » Needs review
FileSize
7.9 KB

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.

jsacksick’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

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

jsacksick’s picture

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.

jsacksick’s picture

Status: Needs work » Needs review
FileSize
7.9 KB
jsacksick’s picture

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

ParisLiakos’s picture

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

jsacksick’s picture

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

dawehner’s picture

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.

jsacksick’s picture

Status: Needs work » Needs review
FileSize
7.36 KB
dawehner’s picture

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

dawehner’s picture

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.

jsacksick’s picture

Status: Needs work » Needs review
FileSize
7.36 KB

Hopefully, this one will pass...

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Great

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

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

tstoeckler’s picture

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