Support from Acquia helps fund testing for Drupal Acquia logo

Comments

vijaycs85’s picture

sanchiz’s picture

Status: Active » Needs review
FileSize
1.64 KB

Initial patch

dawehner’s picture

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.

sanchiz’s picture

Status: Needs work » Needs review
FileSize
5.84 KB

Added access checker and other fixes.

dawehner’s picture

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

dealancer’s picture

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

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?

sanchiz’s picture

Remove extra route and use default entity logic.

dawehner’s picture

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

sanchiz’s picture

Really, deleted.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Manually using the form still works.

alexpott’s picture

+++ 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;
}
alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Updating status appropiately...

sanchiz’s picture

Assigned: Unassigned » sanchiz
Status: Needs work » Needs review
FileSize
2.82 KB

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

alexpott’s picture

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;
    }
  }
sanchiz’s picture

This solution works fine. Fixed.

alexpott’s picture

Status: Needs work » Needs review
dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Manual testing worked fine.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

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

andypost’s picture

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