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
#39 shortcut-1978956-39.patch10.01 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 56,419 pass(es).
[ View ]
#33 1978956-shortcut_set_customize-33.patch10.69 KBnaxoc
PASSED: [[SimpleTest]]: [MySQL] 58,022 pass(es).
[ View ]
#33 interdiff.txt1.23 KBnaxoc
#31 1978956-shortcut_set_customize-31.patch10.67 KBnaxoc
PASSED: [[SimpleTest]]: [MySQL] 58,520 pass(es).
[ View ]
#31 interdiff.txt9.5 KBnaxoc
#29 1978956-shortcut_set_customize-29.patch8.76 KBnaxoc
PASSED: [[SimpleTest]]: [MySQL] 58,066 pass(es).
[ View ]
#23 1978956-shortcut_set_customize-23.patch9.02 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 56,072 pass(es).
[ View ]
#18 drupal-1978956-17.patch9.22 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-1978956-17.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#17 convert_shortcut_set_customize-1978956-17.patch9.04 KBmichaellenahan
PASSED: [[SimpleTest]]: [MySQL] 55,565 pass(es).
[ View ]
#14 Screenshot from 2013-05-06 19:49:08.png41.43 KBmichaellenahan
#13 convert_shortcut_set_customize-1978956-11.patch8.98 KBmichaellenahan
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch convert_shortcut_set_customize-1978956-11_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#11 convert_shortcut_set_customize-1978956-11.patch8.98 KBmichaellenahan
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch convert_shortcut_set_customize-1978956-11.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#6 convert_shortcut_set_customize-1978956-6.patch5.69 KBmichaellenahan
PASSED: [[SimpleTest]]: [MySQL] 55,301 pass(es).
[ View ]
#3 convert_shortcut_set_customize-1978956-2.patch5.69 KBmichaellenahan
PASSED: [[SimpleTest]]: [MySQL] 55,314 pass(es).
[ View ]

Comments

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

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

Tagging...

Status:Needs review» Active
StatusFileSize
new5.69 KB
PASSED: [[SimpleTest]]: [MySQL] 55,314 pass(es).
[ View ]

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 ?

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

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

reinstating tags

StatusFileSize
new5.69 KB
PASSED: [[SimpleTest]]: [MySQL] 55,301 pass(es).
[ View ]

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

Status:Needs review» Needs work

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:

<?php
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.

StatusFileSize
new8.98 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch convert_shortcut_set_customize-1978956-11.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Hello, here's another attempt at a patch.

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

<?php
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.

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

StatusFileSize
new8.98 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch convert_shortcut_set_customize-1978956-11_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Thanks. Patch attached.

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

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

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

StatusFileSize
new41.43 KB

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.

<?php
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?

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.

StatusFileSize
new9.04 KB
PASSED: [[SimpleTest]]: [MySQL] 55,565 pass(es).
[ View ]

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

Status:Needs work» Needs review
StatusFileSize
new9.22 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-1978956-17.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

Assigned:michaellenahan» Unassigned

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

Issue tags:+Needs reroll

Status:Needs work» Needs review
StatusFileSize
new9.02 KB
PASSED: [[SimpleTest]]: [MySQL] 56,072 pass(es).
[ View ]

Re-rolling...

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

Status:Needs review» Needs work

Issue tags:-Needs reroll

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 -

<?php
$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.

Assigned:Unassigned» naxoc

Status:Needs work» Needs review
StatusFileSize
new8.76 KB
PASSED: [[SimpleTest]]: [MySQL] 58,066 pass(es).
[ View ]

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

StatusFileSize
new9.5 KB
new10.67 KB
PASSED: [[SimpleTest]]: [MySQL] 58,520 pass(es).
[ View ]

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.

StatusFileSize
new1.23 KB
new10.69 KB
PASSED: [[SimpleTest]]: [MySQL] 58,022 pass(es).
[ View ]

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.

Status:Needs review» Reviewed & tested by the community

Looks good to me, thanks @naxoc!

Even prooving that nothing changes is a win.

Issue tags:+MENU_LOCAL_ACTION

This blocks removal of MENU_LOCAL_ACTION.

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

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

Status:Needs work» Needs review
Issue tags:+MENU_LOCAL_ACTION
StatusFileSize
new10.01 KB
PASSED: [[SimpleTest]]: [MySQL] 56,419 pass(es).
[ View ]

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

Status:Needs review» Reviewed & tested by the community

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.