Support from Acquia helps fund testing for Drupal Acquia logo

Comments

vijaycs85’s picture

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

Assigned: Unassigned » vyasamit2007
Status: Active » Needs review
FileSize
2.91 KB

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
FileSize
2.91 KB

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
FileSize
2.03 KB

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
FileSize
960 bytes
2.43 KB

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
FileSize
1.18 KB
2.96 KB

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
FileSize
2.96 KB

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
FileSize
2.2 KB
1.46 KB

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

FileSize
2.23 KB

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

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
FileSize
1011 bytes
2.18 KB

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

FileSize
1.85 KB
4.03 KB

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
FileSize
3.65 KB

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
FileSize
1.23 KB
3.54 KB

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.