Support from Acquia helps fund testing for Drupal Acquia logo

Comments

michaellenahan’s picture

Assigned: Unassigned » michaellenahan
Issue tags: -WSCCI-conversion

Working on this as part of the London Drupal Sprint weekend.

alexpott’s picture

Tagging...

michaellenahan’s picture

Status: Needs review » Active
FileSize
5.69 KB

Here's the patch. I'm new to this, so many thanks Alex for your help over the weekend!

The title is static, so the form will show 'Edit shortcuts' as the title rather the correct title of the shortcut set which comes from the title callback. Is this the same issue that is listed here http://drupal.org/node/1981644 ?

michaellenahan’s picture

Status: Active » Needs review
Issue tags: -WSCCI-conversion, -LONDON_2013_APRIL
michaellenahan’s picture

Status: Active » Needs review
Issue tags: +WSCCI-conversion, +LONDON_2013_APRIL

reinstating tags

michaellenahan’s picture

Removed a rogue print_r() statement from the patch.

alexpott’s picture

+++ b/core/modules/shortcut/lib/Drupal/shortcut/Form/SetCustomize.phpundefined
@@ -0,0 +1,102 @@
+  /**
+   * Constructs a SetCustomize object.
+   */
+  public function __construct() {
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  public static function create(ContainerInterface $container) {
+    return new static();
+  }

Need to inject the plugin.manager.entity service from the container to use in the submit method.

+++ b/core/modules/shortcut/lib/Drupal/shortcut/Form/SetCustomize.phpundefined
@@ -0,0 +1,102 @@
+  /**
+   * {@inheritdoc}
+   */
+  public function submitForm(array &$form, array &$form_state) {
+  }

Need to move functionality from shortcut_set_customize_submit() into this method and instead of using menu_link_load/save use the methods on the entity and from the manager.

+++ b/core/modules/shortcut/lib/Drupal/shortcut/ShortcutAccessController.phpundefined
@@ -20,12 +20,31 @@ class ShortcutAccessController extends EntityAccessController {
+        return $entity->id() != 'default';
+        break;
+      ¶
+      case 'customize':
+        // Sufficiently-privileged users can edit their currently displayed shortcut

Unnecessary spaces...

+++ b/core/modules/shortcut/shortcut.moduleundefined
@@ -77,13 +77,7 @@ function shortcut_menu() {
-    'title callback' => 'entity_page_label',
-    'title arguments' => array(5),

We need to keep the title callback and arguments at the moment... unfortunately we then have issues with argument loading - see #1981644: Figure out how to deal with 'title/title callback' and #1974408: Convert aggregator_page_source() to a Controller

Also we can remove the shortcut_set_customize() and shortcut_set_customize_submit() functions from shotcut.admin.inc

dawehner’s picture

Status: Needs review » Needs work

Change status.

michaellenahan’s picture

Hi there, I'm stuck! :)

Need to inject the plugin.manager.entity service from the container to use in the submit method.

I'm not sure how to do this, or if I'm really honest I'm not sure what "inject the plugin.manager.entity service from the container" really means, I'm still quite fresh to D8. Any examples out there I can follow?

... instead of using menu_link_load/save use the methods on the entity and from the manager.

If you can give me a pointer for how to do this I'd be grateful.

Thank you!

dawehner’s picture

Hi!

There is a great blogpost about dependency injection: http://fabien.potencier.org/article/11/what-is-dependency-injection
To sum it up, an object should not rely on global functions, but the dependencies should be passed in as objects in the constructor,
so you can swap it out and test it easy.

The previous code looked like that:

function shortcut_set_customize_submit($form, &$form_state) {
  foreach ($form_state['values']['shortcuts']['links'] as $mlid => $data) {
    $link = menu_link_load($mlid);
    $link['weight'] = $data['weight'];
    menu_link_save($link);
  }
  drupal_set_message(t('The shortcut set has been updated.'));
}

It still relies on the global menu_link_load() method but you can replace it by using something like Drupal::entityManager()->getStorageController('menu_link')->load(array($id)); Instead of using Drupal::entityManager() you can also use an entity manager object,
which get injected into the constructor of the object. Therefore you can use the create() method to get the entity manager from the container.
The ShortcutController does something similar.

michaellenahan’s picture

Hello, here's another attempt at a patch.

I'm now adding the EntityManager in the constructor (lines 41-43 in the patch)

public function __construct(EntityManager $entity_manager) {
   $this->entity_manager = $entity_manager;
}

However, I'm still missing something because when I submit the form by hitting "Save changes" button on:
admin/config/user-interface/shortcut/manage/default

... I get this error.

Fatal error: Call to a member function getStorageController() on a non-object in /workspace/drupal-8/core/modules/shortcut/lib/Drupal/shortcut/Form/SetCustomize.php on line 116

In my submitForm function (line 119 in the patch) I am trying to do this.

+  public function submitForm(array &$form, array &$form_state) {
+    foreach ($form_state['values']['shortcuts']['links'] as $mlid => $data) {
+      $menu_link = $this->entityManager
+        ->getStorageController('menu_link');
+      $link = $menu_link->load(array($mlid));
+      $link['weight'] = $data['weight'];
+      $menu_link->save($link);
+    }
+    drupal_set_message(t('The shortcut set has been updated.'));
+  }

$this->entityManager is not set to anything though.

Any ideas what I'm missing? Thank you!

alexpott’s picture

+++ b/core/modules/shortcut/lib/Drupal/shortcut/Form/SetCustomize.phpundefined
@@ -0,0 +1,124 @@
+    $this->entity_manager = $entity_manager;

Here's why... should be $this->entityManager = $entity_manager;

michaellenahan’s picture

Thanks. Patch attached.

+  public function __construct(EntityManager $entity_manager) {
+    $this->entityManager = $entity_manager;
+  }

Also needed to adjust the $link variable in submitForm() with an array_shift.

+  public function submitForm(array &$form, array &$form_state) {
+    foreach ($form_state['values']['shortcuts']['links'] as $mlid => $data) {
+      $menu_link = $this->entityManager
+        ->getStorageController('menu_link');
+      $link = $menu_link->load(array($mlid));
+      $link = array_shift($link);
+      $link['weight'] = $data['weight'];
+      $menu_link->save($link);
+    }
+    drupal_set_message(t('The shortcut set has been updated.'));
+  }

michaellenahan’s picture

While testing this, I noticed that changing the weight of the links had no effect.

08.png

The changed weights are being saved correctly to the database, the problem is with the $shortcut variable which contains the shortcut set at buildForm time.

public function buildForm(array $form, array &$form_state, Shortcut $shortcut = NULL) {

This $shortcut variable contains the shortcut set to be rendered on-screen. The problem is that the links within the shortcut set are not in ascending weight order.

I noticed this same issue occurs in the pre-conversion version of the same form.
admin/config/user-interface/shortcut/manage/default

How to fix it? Should the $shortcut variable come with links ordered in the correct order? Or should we adjust the order in the buildForm function?

michaellenahan’s picture

Status: Needs work » Needs review

changed to needs review.

Status: Needs review » Needs work

The last submitted patch, convert_shortcut_set_customize-1978956-11.patch, failed testing.

michaellenahan’s picture

Attached the wrong patch last time. Here's the correct one (I hope).

dawehner’s picture

Status: Needs work » Needs review
FileSize
9.22 KB

I realized that the shortcut shorting is broken in general. This is not caused by your conversion, so this might be better solved in a follow up.
One way to solve this would be to order the $entity->links in ShortcutStorageController::preSave() by weight.

While debugging this issue I came up with the idea to not store the full entity manager.

michaellenahan’s picture

Assigned: michaellenahan » Unassigned
kim.pepper’s picture

#18: drupal-1978956-17.patch queued for re-testing.

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

The last submitted patch, drupal-1978956-17.patch, failed testing.

kim.pepper’s picture

Issue tags: +Needs reroll
vijaycs85’s picture

Status: Needs work » Needs review
FileSize
9.02 KB

Re-rolling...

tim.plunkett’s picture

+++ b/core/modules/shortcut/lib/Drupal/shortcut/ShortcutAccessController.phpundefined
@@ -36,6 +36,20 @@ protected function checkAccess(EntityInterface $entity, $operation, $langcode, U
+      case 'customize':
+        // 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($entity) || $entity == shortcut_current_displayed_set();
+        }
+        return FALSE;
+        break;

This logic is already encapsulated in case 'edit'.

+++ b/core/modules/shortcut/lib/Drupal/shortcut/ShortcutAccessController.phpundefined
@@ -36,6 +36,20 @@ protected function checkAccess(EntityInterface $entity, $operation, $langcode, U
+      default:
+        return FALSE;

I don't think this patch should add this.

dawehner’s picture

Status: Needs review » Needs work
star-szr’s picture

Issue tags: -Needs reroll

Tags

michaellenahan’s picture

Hello, I'm looking at rerolling this.

After applying the patch I get this Notice when I go to the following page:
admin/config/user-interface/shortcut/manage/default

Notice: Undefined index: #shortcut_set_name in theme_shortcut_set_customize() (line 213 of core/modules/shortcut/shortcut.admin.inc).

line 213 of core/modules/shortcut/shortcut.admin.inc -

$output = theme('table', array('header' => $header, 'rows' => $rows, 'attributes' => array('id' => 'shortcuts'), 'empty' => t('No shortcuts available. <a href="@link">Add a shortcut</a>.', array('@link' => url('admin/config/user-interface/shortcut/manage/' . $form['#shortcut_set_name'] . '/add-link')))));

it's true, $form['#shortcut_set_name'] doesn't exist.

the variable I'm looking for should read 'default' --- that's the shortcut set name --- but I can't see any variable with this string as content when I look at the $form array.

naxoc’s picture

Assigned: Unassigned » naxoc
naxoc’s picture

Status: Needs work » Needs review
FileSize
8.76 KB

Here is a reroll of the patch in #23.

I changed a couple of things as pr. Tim's comments in #24.

tim.plunkett’s picture

+++ b/core/modules/shortcut/lib/Drupal/shortcut/Form/SetCustomize.phpundefined
@@ -0,0 +1,125 @@
+class SetCustomize implements FormInterface, ControllerInterface {

After looking at this again, I think we could add a standard EntityFormController here. I can help with that if need be.

+++ b/core/modules/shortcut/lib/Drupal/shortcut/ShortcutAccessController.phpundefined
@@ -22,6 +22,7 @@ class ShortcutAccessController extends EntityAccessController {
+      case 'customize':

+++ b/core/modules/shortcut/shortcut.routing.ymlundefined
@@ -25,3 +25,10 @@ shortcut_set_edit:
+    _form: 'Drupal\shortcut\Form\SetCustomize'
+  requirements:
+    _entity_access: 'shortcut.customize'

I don't think we need to expand this to include 'customize', we can just use 'edit'.

naxoc’s picture

I took a stab at making the controller an EntityFormController. I ended up implementing a lot of the same things that are going on in #1938924: Convert shortcut theme tables to table #type which meant that I could kill even more code in shortcut.admin.inc. Not sure if that is a little too much going on though.

dawehner’s picture

It would be cool to have a screenshot of before and after, as there might be changed.

+++ b/core/modules/shortcut/lib/Drupal/shortcut/Form/SetCustomize.phpundefined
@@ -0,0 +1,97 @@
+   * Overrides Drupal\Core\Entity\EntityFormController::actions().

Should be "{@inheritdoc}"

+++ b/core/modules/shortcut/shortcut.moduleundefined
@@ -98,13 +98,7 @@ function shortcut_menu() {
-    'title callback' => 'entity_page_label',
-    'title arguments' => array(5),

We should not remove the title callback here, as it is still needed for menu links.

naxoc’s picture

There are no (visible) changes to the UI, so screenshots would be identical.

I put the title callback back in hook_menu and corrected the comment.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me, thanks @naxoc!

dawehner’s picture

Even prooving that nothing changes is a win.

tim.plunkett’s picture

Issue tags: +MENU_LOCAL_ACTION

This blocks removal of MENU_LOCAL_ACTION.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: -MENU_LOCAL_ACTION

Needs a reroll

git applyc https://drupal.org/files/1978956-shortcut_set_customize-33.patch
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 10942  100 10942    0     0   5231      0  0:00:02  0:00:02 --:--:--  5979
error: patch failed: core/modules/shortcut/shortcut.routing.yml:32
error: core/modules/shortcut/shortcut.routing.yml: patch does not apply
alexpott’s picture

+++ b/core/modules/shortcut/lib/Drupal/shortcut/Form/SetCustomize.phpundefined
@@ -0,0 +1,97 @@
+      '#empty' => t('No shortcuts available. @link', array('@link' => l(t('Add a shortcut'), 'admin/config/user-interface/shortcut/' . $this->entity->id() . '/add-link'))),

We need to inject the url generator here...

tim.plunkett’s picture

Status: Needs work » Needs review
Issue tags: +MENU_LOCAL_ACTION
FileSize
10.01 KB

That calls l(), not url(), that's not injectable yet.
Straight reroll.

ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community
alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 8469e98 and pushed to 8.x. Thanks!

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