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
#49 Shortcuts_v.d8_20130906-153816.png14.62 KBjibran
#49 Shortcuts_v.d8_20130906-154318.png16.42 KBjibran
#44 shortcut-1978952-44.patch3.54 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,832 pass(es).
[ View ]
#44 interdiff.txt1.23 KBtim.plunkett
#42 shortcut-1978952-42.patch3.65 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,268 pass(es).
[ View ]
#38 shortcut-1978952-38.patch4.03 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch shortcut-1978952-38.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#38 interdiff.txt1.85 KBtim.plunkett
#36 drupal8.shortcut-module.1978952-36.patch2.18 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] 58,350 pass(es), 6 fail(s), and 1 exception(s).
[ View ]
#36 interdiff.txt1011 bytesdisasm
#33 drupal8.shortcut-module.1978952-33.patch2.19 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] 57,998 pass(es), 6 fail(s), and 1 exception(s).
[ View ]
#28 shortcut-1978952-28.patch2.23 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch shortcut-1978952-28.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#27 shortcut-1978952-27-FAIL.patch1.46 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 56,660 pass(es), 5 fail(s), and 0 exception(s).
[ View ]
#27 shortcut-1978952-27-PASS.patch2.2 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 56,552 pass(es).
[ View ]
#22 1978952-convert_shortcut_set_add-controller-22.patch2.96 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 56,536 pass(es).
[ View ]
#17 shortcut-1978952-17.patch2.96 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 56,541 pass(es).
[ View ]
#17 interdiff.txt1.18 KBtim.plunkett
#15 1978952-Convert_shortcut_set_add_to_Controller-15.patch2.43 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 55,530 pass(es).
[ View ]
#15 1978952-diff-12-15.txt960 bytesvijaycs85
#12 1978952-Convert_shortcut_set_add_to_Controller-12.patch2.03 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 56,969 pass(es).
[ View ]
#6 1978952-Convert_shortcut_set_add_to_Controller-6.patch2.91 KBvyasamit2007
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1978952-Convert_shortcut_set_add_to_Controller-6.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#2 1978952-Convert shortcut_set_add-to Controller-2.patch2.91 KBvyasamit2007
FAILED: [[SimpleTest]]: [MySQL] Fetch test patch: failed to retrieve [1978952-Convert shortcut_set_add-to Controller-2.patch] from [drupal.org].
[ View ]

Comments

Component:rest.module» shortcut.module

Assigned:Unassigned» vyasamit2007
Status:Active» Needs review
StatusFileSize
new2.91 KB
FAILED: [[SimpleTest]]: [MySQL] Fetch test patch: failed to retrieve [1978952-Convert shortcut_set_add-to Controller-2.patch] from [drupal.org].
[ View ]

PFA. The patch file with the changes as per the link provided.

Status:Needs review» Needs work
Issue tags:-WSCCI-conversion

The last submitted patch, 1978952-Convert shortcut_set_add-to Controller-2.patch, failed testing.

Status:Needs work» Needs review

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

The last submitted patch, 1978952-Convert shortcut_set_add-to Controller-2.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new2.91 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1978952-Convert_shortcut_set_add_to_Controller-6.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Renamed the patch file and queued for re-testing.

+++ b/core/modules/shortcut/lib/Drupal/shortcut/Controller/ShortcutController.phpundefined
@@ -0,0 +1,38 @@
+ /**

Don't use tabs here but spaces :)

This patch should probably wait until #1946454: Convert all confirm_form() in system.module and system.admin.inc to the new form interface and convert route is in, so you can inject the entity manager in there.

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

The last submitted patch, 1978952-Convert_shortcut_set_add_to_Controller-6.patch, failed testing.

Issue tags:+Needs reroll

Status:Needs work» Needs review
StatusFileSize
new2.03 KB
PASSED: [[SimpleTest]]: [MySQL] 56,969 pass(es).
[ View ]

Re-rolling...

Status:Needs review» Reviewed & tested by the community

Adding shortcuts still works fine. Code looks perfect.

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

Sorry hook_local_actions() is missing see Action links are provided by hook_local_actions() instead of MENU_LOCAL_ACTION in hook_menu() for further details.

Status:Needs work» Needs review
StatusFileSize
new960 bytes
new2.43 KB
PASSED: [[SimpleTest]]: [MySQL] 55,530 pass(es).
[ View ]

Updating changes mentioned in #14.

Status:Needs review» Needs work
Issue tags:+Needs reroll

Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new1.18 KB
new2.96 KB
PASSED: [[SimpleTest]]: [MySQL] 56,541 pass(es).
[ View ]

Rerolled

Status:Needs review» Reviewed & tested by the community

awesome, thanks

Everything in #14 is fixed.

Issue tags:+RTBC July 1

This issue was RTBC and passing tests on July 1, the beginning of API freeze.

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs reroll

Needs a reroll

git ac https://drupal.org/files/shortcut-1978952-17.patch
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  3030  100  3030    0     0   3628      0 --:--:-- --:--:-- --:--:--  4604
error: patch failed: core/modules/shortcut/shortcut.admin.inc:172
error: core/modules/shortcut/shortcut.admin.inc: patch does not apply

Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new2.96 KB
PASSED: [[SimpleTest]]: [MySQL] 56,536 pass(es).
[ View ]

Re-rolling...

Status:Needs review» Needs work
Issue tags:-WSCCI-conversion, -MENU_LOCAL_ACTION, -RTBC July 1

The last submitted patch, 1978952-convert_shortcut_set_add-controller-22.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+WSCCI-conversion, +MENU_LOCAL_ACTION, +RTBC July 1

Status:Needs review» Reviewed & tested by the community

This has been ready to fly before.

Status:Reviewed & tested by the community» Fixed

Committed 522c787 and pushed to 8.x. Thanks!

Category:task» bug
Priority:Normal» Major
Status:Fixed» Needs review
StatusFileSize
new2.2 KB
PASSED: [[SimpleTest]]: [MySQL] 56,552 pass(es).
[ View ]
new1.46 KB
FAILED: [[SimpleTest]]: [MySQL] 56,660 pass(es), 5 fail(s), and 0 exception(s).
[ View ]

This doesn't actually work, we just had no test coverage.

The test right now is only covering itself, not the UI...

StatusFileSize
new2.23 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch shortcut-1978952-28.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Rerolled.

Status:Needs review» Needs work

The last submitted patch, shortcut-1978952-28.patch, failed testing.

This patch still valid and getting error because of 'Add shortcut set' missing in admin/config/user-interface/shortcut. Seems LOCAL_TASK implementation broken between #27 and #28

Status:Needs work» Needs review
Issue tags:-WSCCI-conversion, -MENU_LOCAL_ACTION, -RTBC July 1

#28: shortcut-1978952-28.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+WSCCI-conversion, +MENU_LOCAL_ACTION, +RTBC July 1

The last submitted patch, shortcut-1978952-28.patch, failed testing.

StatusFileSize
new2.19 KB
FAILED: [[SimpleTest]]: [MySQL] 57,998 pass(es), 6 fail(s), and 1 exception(s).
[ View ]

reroll!

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, drupal8.shortcut-module.1978952-33.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1011 bytes
new2.18 KB
FAILED: [[SimpleTest]]: [MySQL] 58,350 pass(es), 6 fail(s), and 1 exception(s).
[ View ]

Missed an entity manager change in the test.

Status:Needs review» Needs work

The last submitted patch, drupal8.shortcut-module.1978952-36.patch, failed testing.

StatusFileSize
new1.85 KB
new4.03 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch shortcut-1978952-38.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

ShortcutSetAccessController was really out of date.

Status:Needs work» Needs review
Issue tags:-WSCCI-conversion

Status:Needs review» Needs work

The last submitted patch, shortcut-1978952-38.patch, failed testing.

Issue tags:+Needs reroll

Needs reroll.

Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new3.65 KB
PASSED: [[SimpleTest]]: [MySQL] 58,268 pass(es).
[ View ]

Reroll, #2062021: Replace user_access() calls with $account->hasPermission() in shortcut module decided to blindly update the access checker without manually testing anything

Status:Needs review» Needs work
  1. +++ b/core/modules/shortcut/lib/Drupal/shortcut/ShortcutSetAccessController.php
    @@ -20,14 +20,14 @@ class ShortcutSetAccessController extends EntityAccessController {
       protected function checkAccess(EntityInterface $entity, $operation, $langcode, AccountInterface $account) {
    +    $account = $this->prepareUser($account);
         switch ($operation) {

    This is not necessary, $account is always provided, prepareUser() is called in access().

  2. +++ b/core/modules/shortcut/lib/Drupal/shortcut/ShortcutSetAccessController.php
    @@ -41,4 +41,12 @@ protected function checkAccess(EntityInterface $entity, $operation, $langcode, A
    +  public function createAccess($entity_bundle = NULL, AccountInterface $account = NULL, array $context = array()) {
    +    $account = $this->prepareUser($account);
    +    return $account->hasPermission('administer shortcuts') || $account->hasPermission('customize shortcut links');
    +  }

    This should implement checkCreateAccess() instead (just like the other one is checkAccess() and not access()), then you don't need the prepare stuff but always get an account object.

Status:Needs work» Needs review
StatusFileSize
new1.23 KB
new3.54 KB
PASSED: [[SimpleTest]]: [MySQL] 58,832 pass(es).
[ View ]

Duh, thanks @Berdir. I should have known better :)

+++ b/core/modules/shortcut/lib/Drupal/shortcut/ShortcutSetAccessController.php
@@ -21,13 +21,12 @@ class ShortcutSetAccessController extends EntityAccessController {
+          return $entity == shortcut_current_displayed_set($account);

ahh can we add ShortcutManager? and move shortcut_current_displayed_set to ShortcutManager.

Not in this issue, no. This is a regression, we need to fix it ASAP.
I will be glad to join you in a follow-up!

Title:Convert shortcut_set_add to a ControllerRegression: Convert shortcut_set_add to a Controller
Status:Needs review» Reviewed & tested by the community
Issue tags:+Needs followup

Ok then RTBC for a green green patch with tests.

Issue tags:-Needs followup+WSCCI

Created on public demand #2083123: Create ShortcutManager.

Here is before and after screenshots @alexpott suggestion.

Before

Shortcuts_v.d8_20130906-153816.png

After

Shortcuts_v.d8_20130906-154318.png

Title:Regression: Convert shortcut_set_add to a ControllerConvert shortcut_set_add to a Controller
Priority:Major» Normal
Status:Reviewed & tested by the community» Fixed

Committed b84bec5 and pushed to 8.x. Thanks!

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