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
#15 shortcut-convert-edit-form-1978964-15.patch3.3 KBsanchiz
PASSED: [[SimpleTest]]: [MySQL] 55,427 pass(es).
[ View ]
#13 shortcut-convert-edit-form-1978964-13.patch2.82 KBsanchiz
PASSED: [[SimpleTest]]: [MySQL] 55,663 pass(es).
[ View ]
#9 shortcut-convert-edit-form-1978964-9.patch2.88 KBsanchiz
PASSED: [[SimpleTest]]: [MySQL] 55,474 pass(es).
[ View ]
#7 shortcut-convert-edit-form-1978964-7.patch3.26 KBsanchiz
PASSED: [[SimpleTest]]: [MySQL] 55,635 pass(es).
[ View ]
#4 shortcut-convert-edit-form-1978964-4.patch5.84 KBsanchiz
PASSED: [[SimpleTest]]: [MySQL] 56,024 pass(es).
[ View ]
#2 shortcut-convert-edit-form-1978964-2.patch1.64 KBsanchiz
FAILED: [[SimpleTest]]: [MySQL] 55,365 pass(es), 9 fail(s), and 0 exception(s).
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new1.64 KB
FAILED: [[SimpleTest]]: [MySQL] 55,365 pass(es), 9 fail(s), and 0 exception(s).
[ View ]

Initial patch

Status:Needs review» Needs work

You seem to have forgotten the name file :)

diff --git a/.htaccess b/.htaccess
index 45abcc7..101c954 100644
--- a/.htaccess
+++ b/.htaccessundefined
@@ -147,3 +147,7 @@ DirectoryIndex index.php index.html index.htm
     </FilesMatch>
   </IfModule>
</IfModule>
+
+#custom
+php_value error_reporting 1
+php_flag display_errors on

This feels out of scope :)

+++ b/core/modules/shortcut/shortcut.moduleundefined
@@ -91,13 +91,7 @@ function shortcut_menu() {
-    'type' => MENU_LOCAL_TASK,
...
-    'weight' => 10,

We should not remove this lines, because that's still needed.

+++ b/core/modules/shortcut/shortcut.moduleundefined
@@ -91,13 +91,7 @@ function shortcut_menu() {
-    'access callback' => 'shortcut_set_edit_access',
+++ b/core/modules/shortcut/shortcut.routing.ymlundefined
@@ -11,3 +11,10 @@ shortcut_set_delete:
+    _permission: 'customize shortcut links'

You need an access checker defined for this route to implement the previous access logic.

Status:Needs work» Needs review
StatusFileSize
new5.84 KB
PASSED: [[SimpleTest]]: [MySQL] 56,024 pass(es).
[ View ]

Added access checker and other fixes.

+++ b/core/modules/shortcut/shortcut.routing.ymlundefined
@@ -18,3 +18,9 @@ shortcut_set_admin:
+shortcut_set_edit:
+  pattern: '/admin/config/user-interface/shortcut/manage/{shortcut}/edit'
+  defaults:
+    _form: 'Drupal\shortcut\Form\SetEdit'
+  requirements:
+    _access_shortcut_set_edit: 'TRUE'

Couldn't you actually use something like views is doing:

views_ui.add:
  pattern: '/admin/structure/views/add'
  defaults:
    _entity_form: 'view.add'
  requirements:
    _permission: 'administer views'

So basically you can explicit tell the system to use an entity form controller and all logic is handled in there. No need for an extra route controller

Looks like we are adding additional access controller to run following function:

<?php
function shortcut_set_edit_access($shortcut_set = NULL) {
 
// Sufficiently-privileged users can edit their currently displayed shortcut
  // set, but not other sets. Shortcut administrators can edit any set.
 
if (user_access('administer shortcuts')) {
    return
TRUE;
  }
  if (
user_access('customize shortcut links')) {
    return !isset(
$shortcut_set) || $shortcut_set == shortcut_current_displayed_set();
  }
  return
FALSE;
}
?>

It is called in several places, also it relies on shortcut_current_displayed_set().

Looks like access functions and controllers should be completely re factored. Any ideas how to do it or may be we can do it as a separate issue?

StatusFileSize
new3.26 KB
PASSED: [[SimpleTest]]: [MySQL] 55,635 pass(es).
[ View ]

Remove extra route and use default entity logic.

+++ b/core/modules/shortcut/shortcut.moduleundefined
@@ -586,3 +582,15 @@ function shortcut_library_info() {
+/**
+ * Implements hook_entity_info().
+ */
+function shortcut_entity_info(&$entity_info) {
+  $entity_info['shortcut']['controllers'] += array(
+    'list' => 'Drupal\shortcut\ShortcutListController',
+    'form' => array(
+      'edit' => 'Drupal\shortcut\ShortcutFormController',
+    ),
+  );
+}

You don't need this additional hook_entity_info anymore.

StatusFileSize
new2.88 KB
PASSED: [[SimpleTest]]: [MySQL] 55,474 pass(es).
[ View ]

Really, deleted.

Status:Needs review» Reviewed & tested by the community

Manually using the form still works.

+++ b/core/modules/shortcut/lib/Drupal/shortcut/ShortcutAccessController.phpundefined
@@ -20,7 +20,10 @@ class ShortcutAccessController extends EntityAccessController {
-    if ($operation == 'delete') {
+    if ($operation == 'edit') {
+      return user_access('administer shortcuts', $account);
+    }
+    else if ($operation == 'delete') {

Hmmm... it doesn't look like we've properly replaced the shortcut_set_edit_access() function... current function below

function shortcut_set_edit_access($shortcut_set = NULL) {
  // Sufficiently-privileged users can edit their currently displayed shortcut
  // set, but not other sets. Shortcut administrators can edit any set.
  if (user_access('administer shortcuts')) {
    return TRUE;
  }
  if (user_access('customize shortcut links')) {
    return !isset($shortcut_set) || $shortcut_set == shortcut_current_displayed_set();
  }
  return FALSE;
}

Status:Reviewed & tested by the community» Needs work

Updating status appropiately...

Assigned:Unassigned» sanchiz
Status:Needs work» Needs review
StatusFileSize
new2.82 KB
PASSED: [[SimpleTest]]: [MySQL] 55,663 pass(es).
[ View ]

But this logic controller uses to remove. You must use the shortcut_set_edit_access() in method checkAccess() or move shortcut_set_edit_access() in checkAccess()?

This patch use shortcut_set_edit_access() in access controller

Status:Needs review» Needs work

I think we should put the logic in the access controller. I.e. something like this...

  protected function checkAccess(EntityInterface $entity, $operation, $langcode, User $account) {
    switch ($operation) {
      case 'edit':
        if (user_access('administer shortcuts', $account)) {
          return TRUE;
        }
        if (user_access('customize shortcut links', $account)) {
          return !isset($entity) || $entity == shortcut_current_displayed_set($account);
        }
        return FALSE;
        break;
      case 'delete':
        if (!user_access('administer shortcuts', $account)) {
          return FALSE;
        }
        return $entity->id() != 'default';
        break;
    }
  }

StatusFileSize
new3.3 KB
PASSED: [[SimpleTest]]: [MySQL] 55,427 pass(es).
[ View ]

This solution works fine. Fixed.

Status:Needs work» Needs review

Status:Needs review» Reviewed & tested by the community

Manual testing worked fine.

Status:Reviewed & tested by the community» Fixed

Committed 62eebd6 and pushed to 8.x. Thanks!

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