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

vijaycs85’s picture

Component:rest.module» shortcut.module
vyasamit2007’s picture

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.

vijaycs85’s picture

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.

vyasamit2007’s picture

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.

dawehner’s picture

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

tim.plunkett’s picture

kim.pepper’s picture

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.

kim.pepper’s picture

Issue tags:+Needs reroll
vijaycs85’s picture

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

Re-rolling...

dawehner’s picture

Status:Needs review» Reviewed & tested by the community

Adding shortcuts still works fine. Code looks perfect.

jibran’s picture

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.

vijaycs85’s picture

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

Updating changes mentioned in #14.

ParisLiakos’s picture

Status:Needs review» Needs work
Issue tags:+Needs reroll
tim.plunkett’s picture

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

ParisLiakos’s picture

Status:Needs review» Reviewed & tested by the community

awesome, thanks

jibran’s picture

Everything in #14 is fixed.

YesCT’s picture

Issue tags:+RTBC July 1

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

alexpott’s picture

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
vijaycs85’s picture

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.

tim.plunkett’s picture

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

Status:Needs review» Reviewed & tested by the community

This has been ready to fly before.

alexpott’s picture

Status:Reviewed & tested by the community» Fixed

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

tim.plunkett’s picture

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

tim.plunkett’s picture

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.

vijaycs85’s picture

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

Berdir’s picture

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.

disasm’s picture

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

reroll!

disasm’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work

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

disasm’s picture

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.

tim.plunkett’s picture

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.

tim.plunkett’s picture

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

Status:Needs review» Needs work

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

jibran’s picture

Issue tags:+Needs reroll

Needs reroll.

tim.plunkett’s picture

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

Berdir’s picture

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.

tim.plunkett’s picture

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 :)

jibran’s picture

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

tim.plunkett’s picture

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!

jibran’s picture

Title:Convert shortcut_set_add to a Controller» Regression: 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.

jibran’s picture

jibran’s picture

Here is before and after screenshots @alexpott suggestion.

Before

Shortcuts_v.d8_20130906-153816.png

After

Shortcuts_v.d8_20130906-154318.png

alexpott’s picture

Title:Regression: Convert shortcut_set_add to a Controller» Convert 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.