Convert shortcut_set_customize to a Controller

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

Tagging...

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 ?

reinstating tags

Removed a rogue print_r() statement from the patch.

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

Change status.

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!

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.

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!

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

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.'));
+  }

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

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?

changed to needs review.

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

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.

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

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

Tags

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.

Here is a reroll of the patch in #23.

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

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

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.

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.

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.

Looks good to me, thanks @naxoc!

Even prooving that nothing changes is a win.

This blocks removal of 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

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

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

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