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
#177 1978976-177.patch1.88 KBlussoluca
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,393 pass(es).
[ View ]
#173 interdiff.txt721 byteskim.pepper
#173 1978976-shortcut-set-controller-173.patch24.61 KBkim.pepper
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,041 pass(es).
[ View ]
#171 interdiff.txt1.3 KBkim.pepper
#171 1978976-shortcut-set-controller-171.patch24.46 KBkim.pepper
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,923 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#169 interdiff.txt4.83 KBkim.pepper
#169 1978976-shortcut-set-controller-169.patch24.3 KBkim.pepper
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,926 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#165 1978976-diff-162-165.txt1.55 KBvijaycs85
#165 1978976-shortcut_set_controller-165.patch25.39 KBvijaycs85
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,082 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#162 1978976-shortcut_set_controller-162.patch25.53 KBshivachevva
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,013 pass(es).
[ View ]
#158 1978976-diff-156-158.txt968 bytesvijaycs85
#158 1978976-shortcut_set_controller-158.patch25.86 KBvijaycs85
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 1978976-shortcut_set_controller-158.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#156 interdiff.txt692 bytesdawehner
#156 shortcut_set-1978976-156.patch26 KBdawehner
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,652 pass(es), 141 fail(s), and 1 exception(s).
[ View ]
#154 interdiff.txt5.67 KBdawehner
#154 shortcut-1978976-154.patch26.01 KBdawehner
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,625 pass(es), 145 fail(s), and 32 exception(s).
[ View ]
#149 interdiff.txt568 bytesXano
#149 drupal_1978976_149.patch26.06 KBXano
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,728 pass(es).
[ View ]

Comments

kgoel’s picture

Assigned:Unassigned» kgoel
kgoel’s picture

Status:Active» Needs review
StatusFileSize
new13.42 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/shortcut/lib/Drupal/shortcut/Form/SetSwitch.php.
[ View ]

I am not sure about construct method but uploading patch so someone can review the patch and guide me in the right direction.

Thanks

Status:Needs review» Needs work

The last submitted patch, convert_shortcut_set_switch-7396768-2.patch, failed testing.

kgoel’s picture

Status:Needs work» Needs review
StatusFileSize
new13.59 KB
new13.42 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/shortcut/lib/Drupal/shortcut/Form/SetSwitch.php.
[ View ]

Status:Needs review» Needs work

The last submitted patch, convert_shortcut_set_switch-1978976-4.patch, failed testing.

kgoel’s picture

Status:Needs work» Needs review
StatusFileSize
new13.42 KB
FAILED: [[SimpleTest]]: [MySQL] 55,349 pass(es), 19 fail(s), and 0 exception(s).
[ View ]

Status:Needs review» Needs work

The last submitted patch, convert_shortcut_set_switch-1978976-6.patch, failed testing.

Crell’s picture

+++ b/core/modules/shortcut/lib/Drupal/shortcut/Form/SetSwitch.php
@@ -0,0 +1,191 @@
+    $sets = entity_load_multiple('shortcut');

I'm pretty sure we can inject the entity manager via create/construct so we don't need this function anymore.

+++ b/core/modules/shortcut/lib/Drupal/shortcut/Form/SetSwitch.php
@@ -0,0 +1,191 @@
+        '@switch-url' => url(current_path()),

current_path() shouldn't be used anymore. Instead, buildForm() should take Request $request as an additional parameter, and save that object to a property. Then here you can do $this->request->attributes->get('system_path').

(Note: Tim Plunkett may disagree with me here; there's some weirdness around forms and caching that limits their injectability at the moment.)

kgoel’s picture

Status:Needs work» Needs review
StatusFileSize
new13.53 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/shortcut/lib/Drupal/shortcut/Form/SetSwitch.php.
[ View ]

I have added entity manager but not sure if I need to remove this function -

$sets = entity_load_multiple('shortcut');

Also, not sure how to do this. Is there any example, I can follow or documentation will help.

current_path() shouldn't be used anymore. Instead, buildForm() should take Request $request as an additional parameter, and save that object to a property. Then here you can do $this->request->attributes->get('system_path').

Status:Needs review» Needs work

The last submitted patch, convert_shortcut_set_switch-1978976-9.patch, failed testing.

Crell’s picture

Issue tags:+WSCCI-conversion
+++ b/core/modules/shortcut/lib/Drupal/shortcut/Form/SetSwitch.php
@@ -0,0 +1,201 @@
+   * Constructs a SetSwitch object.
+   * @param \Drupal\Core\Entity\EntityManager $entity_manager
+   *   The entity manager.

Line break before the @param.

+++ b/core/modules/shortcut/lib/Drupal/shortcut/Form/SetSwitch.php
@@ -0,0 +1,201 @@
+  public function __construct(EntityManager $entity_manager) {
+    $this->entityManager = $entity_manager;

Modify this to take a Request $request parameter (you'll need another use statement above). Then $this->currentPath = $request->attributes->get('system_path');

+++ b/core/modules/shortcut/lib/Drupal/shortcut/Form/SetSwitch.php
@@ -0,0 +1,201 @@
+  public static function create(ContainerInterface $container) {
+    return new static (
+      $container->get('plugin.manager.entity')
+    );

Add a second parameter to this call, $container->get('request'). (This is not a great way of doing it, but the alternative doesn't work on forms yet.)

Also, no space after static.

+++ b/core/modules/shortcut/lib/Drupal/shortcut/Form/SetSwitch.php
@@ -0,0 +1,201 @@
+      $options[$name] = check_plain($set->label());

I think check_plain() now has a static method you can call instead, on the String:: class. (I don't recall if that landed yet or not.)

+++ b/core/modules/shortcut/lib/Drupal/shortcut/Form/SetSwitch.php
@@ -0,0 +1,201 @@
+      '@switch-url' => url(current_path()),

Now replace this line with url($this->currentPath) instead. (The generator isn't quite here yet for the injected version, so url() is still necessary.)

Also, there's some trailing whitespace to clean up. dreditor will highlight it for you.

kgoel’s picture

Status:Needs work» Needs review
StatusFileSize
new13.63 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/shortcut/lib/Drupal/shortcut/Form/SetSwitch.php.
[ View ]

Thanks Crell!

I think check_plain() now has a static method you can call instead, on the String:: class. (I don't recall if that landed yet or not.)

I have checked core/modules/action/lib/Drupal/action/Controller/ActionController.php and it seems check_plain is still being used.

Status:Needs review» Needs work

The last submitted patch, convert_shortcut_set_switch-1978976-12.patch, failed testing.

kgoel’s picture

Status:Needs work» Needs review
StatusFileSize
new1.38 KB
new13.61 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/shortcut/lib/Drupal/shortcut/Form/SetSwitch.php.
[ View ]

Status:Needs review» Needs work

The last submitted patch, convert_shortcut_set_switch-1978976-14.patch, failed testing.

kgoel’s picture

Status:Needs work» Needs review
StatusFileSize
new652 bytes
new13.62 KB
FAILED: [[SimpleTest]]: [MySQL] 55,811 pass(es), 18 fail(s), and 0 exception(s).
[ View ]

Status:Needs review» Needs work

The last submitted patch, convert_shortcut_set_switch-1978976-16.patch, failed testing.

dawehner’s picture

+++ b/core/modules/shortcut/lib/Drupal/shortcut/Form/SetSwitch.phpundefined
@@ -0,0 +1,204 @@
+  public function __construct(EntityManager $entity_manager) {
+    $this->currentPath = $request->attributes->get('system_path');
+  }

You probably want to store the actual manager here :) $this->entityManger = $entity_manager

+++ b/core/modules/shortcut/lib/Drupal/shortcut/Form/SetSwitch.phpundefined
@@ -0,0 +1,204 @@
+    $sets = entity_load_multiple('shortcut');

Then use $this->entityManager->getStorageController('shortcut')->load() .. Personally I prefer to just store the storage controller and not the full entity manager

+++ b/core/modules/shortcut/lib/Drupal/shortcut/Form/SetSwitch.phpundefined
@@ -0,0 +1,204 @@
+function shortcut_set_switch_validate($form, &$form_state) {

This should by on the validateForm and submitForm method instead.

+++ b/core/modules/shortcut/lib/Drupal/shortcut/Form/SetSwitch.phpundefined
@@ -0,0 +1,204 @@
+    $set = entity_create('shortcut', array(
...
+    $set = shortcut_set_load($form_state['values']['set']);

entity_create/shortcut_set_load could also just use the entity storage controller.

kgoel’s picture

Status:Needs work» Needs review
StatusFileSize
new2.47 KB
new13.77 KB
FAILED: [[SimpleTest]]: [MySQL] Repository checkout: failed to checkout from [git://git.drupal.org/project/drupal.git].
[ View ]

I have addressed some of the issues under comment #18. Need to look at some doc as how to add validate and submit form method.

kgoel’s picture

StatusFileSize
new1.24 KB
new13.74 KB
Test request sent.
[ View ]

Need to look at some doc as how to add validate and submit form method.

Took care of this. This patch addressed all the issues.

kgoel’s picture

Crell’s picture

+++ b/core/modules/shortcut/lib/Drupal/shortcut/Form/SetSwitch.php
@@ -0,0 +1,206 @@
+    $this->currentPath = $request->attributes->get('system_path');

There is no $request in this scope, so this line will fail.

For a controller, you never need the request in the constructor. Instead, you can get it passed into the controller method itself.

+++ b/core/modules/shortcut/lib/Drupal/shortcut/Form/SetSwitch.php
@@ -0,0 +1,206 @@
+      $container->get('plugin.manager.entity')->get('request')

I don't think this line is right... There is no request entity for you to get...

+++ b/core/modules/shortcut/lib/Drupal/shortcut/Form/SetSwitch.php
@@ -0,0 +1,206 @@
+  public function buildForm(array $form, array &$form_state, $account = NULL) {

If you put Request $request = NULL at the end of this method's parameters, you'll get the request object passed to you directly. No need for it in the constructor.

Crell’s picture

Status:Needs review» Needs work
kgoel’s picture

Need some clarification on the following -
$this->currentPath = $request->attributes->get('system_path');

There is no $request in this scope, so this line will fail.
For a controller, you never need the request in the constructor. Instead, you can get it passed into the controller method itself.

Looking at form interface api (submit form) it takes only two arguments and so not sure how can I pass request argument in the controller method. That's why I passed request argument in the constructor method.

Crell’s picture

Actually what I said before is blocked on #1998166: Use the controller resolver to inject parameters into HtmlFormController.

Once that's in, the buildForm() method will work just like any other controller method. As long as parameters you add after the first two are nominally optional (ie, have a default defined), they still will pass the interface and will get parameters passed to them by name just like controllers, including the Request object.

kgoel’s picture

Status:Needs work» Needs review
StatusFileSize
new1.71 KB
new13.79 KB
FAILED: [[SimpleTest]]: [MySQL] 57,359 pass(es), 18 fail(s), and 0 exception(s).
[ View ]
dawehner’s picture

+++ b/core/modules/shortcut/lib/Drupal/shortcut/Form/SetSwitch.phpundefined
@@ -0,0 +1,207 @@
+    $this->entityManager->getStorageController('shortcut')->load();
+    $sets = entity_load_multiple('shortcut');

it should be $sets = $this->entityManager ....

+++ b/core/modules/shortcut/lib/Drupal/shortcut/Form/SetSwitch.phpundefined
@@ -0,0 +1,207 @@
+    $set = shortcut_set_load($form_state['values']['set']);

You could also load a single shortcut if you use the storage controller.

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

The last submitted patch, convert_shortcut_set_switch-26.patch, failed testing.

Crell’s picture

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

#26: convert_shortcut_set_switch-26.patch queued for re-testing.

dawehner’s picture

Status:Needs review» Needs work
kgoel’s picture

Status:Needs work» Needs review
StatusFileSize
new877 bytes
new13.8 KB
FAILED: [[SimpleTest]]: [MySQL] 57,137 pass(es), 20 fail(s), and 2 exception(s).
[ View ]
dawehner’s picture

Status:Needs review» Needs work
+++ b/core/modules/shortcut/lib/Drupal/shortcut/Form/SetSwitch.phpundefined
@@ -0,0 +1,207 @@
+    $sets = entity_load_multiple('shortcut');

no need for this line.

+++ b/core/modules/shortcut/lib/Drupal/shortcut/Form/SetSwitch.phpundefined
@@ -0,0 +1,207 @@
+    $set = shortcut_set_load($form_state['values']['set']);

... should also use the storage controller method.

kgoel’s picture

+++ b/core/modules/shortcut/lib/Drupal/shortcut/Form/SetSwitch.phpundefined
@@ -0,0 +1,207 @@
+    $set = shortcut_set_load($form_state['values']['set']);

... should also use the storage controller method.

I am not sure how to use storage controller here.

tim.plunkett’s picture

Status:Needs work» Needs review
StatusFileSize
new9.15 KB
new14.41 KB
PASSED: [[SimpleTest]]: [MySQL] 56,520 pass(es).
[ View ]

This needs a custom access checker, "switch" is not a standard entity operation. For now I've made it _access: 'TRUE'.

The interdiff is with whitespace changes ignored, the validate and submit methods were indented wrong.

dawehner’s picture

Manual testing works pretty fine. So here is incredible nitpick.

+++ b/core/modules/shortcut/lib/Drupal/shortcut/Form/SetSwitch.phpundefined
@@ -0,0 +1,223 @@
+*/

There is a space missing here.

kgoel’s picture

StatusFileSize
new450 bytes
new14.41 KB
FAILED: [[SimpleTest]]: [MySQL] 57,571 pass(es), 4 fail(s), and 40 exception(s).
[ View ]

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

The last submitted patch, shortcut-1978976-36.patch, failed testing.

kgoel’s picture

Status:Needs work» Needs review

#36: shortcut-1978976-36.patch queued for re-testing.

Status:Needs review» Needs work

The last submitted patch, shortcut-1978976-36.patch, failed testing.

Crell’s picture

Status:Needs work» Needs review

#36: shortcut-1978976-36.patch queued for re-testing.

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

The last submitted patch, shortcut-1978976-36.patch, failed testing.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new14.47 KB
FAILED: [[SimpleTest]]: [MySQL] 57,970 pass(es), 4 fail(s), and 8 exception(s).
[ View ]
new1.31 KB

This should fix most of the errors.

Status:Needs review» Needs work

The last submitted patch, drupal-1978976-42.patch, failed testing.

kgoel’s picture

Assigned:kgoel» Unassigned
dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new1.04 KB
new15.51 KB
PASSED: [[SimpleTest]]: [MySQL] 57,894 pass(es).
[ View ]

There we go.

tim.plunkett’s picture

+++ b/core/modules/shortcut/lib/Drupal/shortcut/Form/SetSwitch.phpundefined
@@ -0,0 +1,225 @@
+      $default_set = shortcut_default_set($this->account);

Unrelated to this issue, should this be a method on the storage controller? Or a static method on the entity class?

+++ b/core/modules/shortcut/lib/Drupal/shortcut/Form/SetSwitch.phpundefined
@@ -0,0 +1,225 @@
+    shortcut_set_assign_user($set, $this->account);

Now that the storage controller has an assignUser() method, we should use that

+++ b/core/modules/shortcut/shortcut.moduleundefined
@@ -152,12 +152,8 @@ function shortcut_menu() {
-    'access callback' => 'shortcut_set_switch_access',
-    'access arguments' => array(1),

+++ b/core/modules/shortcut/shortcut.routing.ymlundefined
@@ -24,3 +24,13 @@ shortcut_set_edit:
+  requirements:
+    _access: 'TRUE'

This needs to be fixed

dawehner’s picture

Status:Needs review» Needs work

Unrelated to this issue, should this be a method on the storage controller? Or a static method on the entity class?

Yeah either on the entity class or some other service (the storage controller should have different logic).

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new23.38 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site.
[ View ]

Replaced these two methods on the storage controller but we could put even more on the longrun.

Status:Needs review» Needs work

The last submitted patch, drupal-1978976-48.patch, failed testing.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new1.32 KB
new22.71 KB
FAILED: [[SimpleTest]]: [MySQL] 56,927 pass(es), 10 fail(s), and 8 exception(s).
[ View ]

Let's not remove the used method.

Status:Needs review» Needs work

The last submitted patch, drupal-1978976-50.patch, failed testing.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new1 KB
new22.75 KB
FAILED: [[SimpleTest]]: [MySQL] 56,481 pass(es), 10 fail(s), and 3 exception(s).
[ View ]

This should work now.

Status:Needs review» Needs work

The last submitted patch, drupal-1978976-52.patch, failed testing.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new2.69 KB
new22.99 KB
PASSED: [[SimpleTest]]: [MySQL] 56,667 pass(es).
[ View ]
Crell’s picture

Status:Needs review» Needs work
+++ b/core/modules/shortcut/lib/Drupal/shortcut/Access/SetSwitchAccessCheck.php
@@ -0,0 +1,52 @@
+  /**
+   * {@inheritdoc}
+   */
+  public function access(Route $route, Request $request) {
+    $global_account = $request->attributes->get('account');
+    $account = $request->attributes->get('_account');
+
+    if (user_access('administer shortcuts', $global_account)) {
+      // Administrators can switch anyone's shortcut set.
+      return static::ALLOW;
+    }
+
+    if (!user_access('switch shortcut sets', $global_account)) {
+      // The user has no permission to switch anyone's shortcut set.
+      return static::DENY;
+    }
+
+    if (!isset($account) || $global_account->id() == $account->id()) {
+      // Users with the 'switch shortcut sets' permission can switch their own
+      // shortcuts sets.
+      return static::ALLOW;
+    }
+
+    return static::DENY;
+  }
+

I think this can be simplified. The administer marker can be moved to its own _permission check. Then this checker can just check "does this user have a permission AND is editing their own account" ? ALLOW : DENY.

Then we switch the access mode to ANY. That makes it a good example of how "admin override permissions" can work for other routes.

+++ b/core/modules/shortcut/lib/Drupal/shortcut/Form/SetSwitch.php
@@ -0,0 +1,233 @@
+    global $user;

If we're accepting $_account as a parameter then we don't need global $user. They're the same object. (===)

+++ b/core/modules/shortcut/lib/Drupal/shortcut/Form/SetSwitch.php
@@ -0,0 +1,233 @@
+    $this->account = $this->userStorageController->load($_account->id())->getBCEntity();

Oh this is so confusing. $_account vs. $account? I have no idea which is which here. These need better names.

+++ b/core/modules/shortcut/lib/Drupal/shortcut/Form/SetSwitch.php
@@ -0,0 +1,233 @@
+    $sets = $this->storageController->loadMultiple(array($id));
+    return !empty($sets[$id]);

We don't need loadMultiple() here. A single load() will do since we're only looking for one item.

+++ b/core/modules/shortcut/shortcut.routing.yml
@@ -26,6 +26,16 @@ shortcut_set_edit:
+  pattern: '/user/{_account}/shortcuts'

Oh yikes. That's right, $account is what we call the active account, not $_account. This is again seriously confusing. Honestly right now I don't even know which variable is which anymore in this patch, which is a bad sign. :-) All of these need better names.

An underscored variable in the pattern is rarely appropriate.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new8.8 KB
new23.43 KB
PASSED: [[SimpleTest]]: [MySQL] 56,853 pass(es).
[ View ]

Okay, let's rename _account to user, as that is what it is. It is a user entity.

Crell’s picture

+++ b/core/modules/shortcut/lib/Drupal/shortcut/Access/SetSwitchAccessCheck.php
@@ -27,26 +27,20 @@ public function applies(Route $route) {
+    // @todo For some reasons account might not exist when checking menu link
+    //   access.
+    if (!isset($account)) {

I'm a little concerned about this todo. Should we investigate further? When might it not exist?

Otherwise I think we're good.

dawehner’s picture

No, it is known, see menu_item_route_access().

The account object should maybe copied from the parent request.

Crell’s picture

For a mocked request for menu access checking, yes, we should pass the account down through. If that resolves that todo, great. If not, we should note in the todo what circumstances that would be, even if it's just a @see menu_item_route_access().

dawehner’s picture

dawehner’s picture

StatusFileSize
new21.9 KB
PASSED: [[SimpleTest]]: [MySQL] 58,292 pass(es).
[ View ]

Rerolled.

Crell’s picture

  1. +++ b/core/modules/shortcut/lib/Drupal/shortcut/Access/SetSwitchAccessCheck.php
    @@ -0,0 +1,40 @@
    +class SetSwitchAccessCheck implements AccessCheckInterface {
    +
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function applies(Route $route) {
    +    return array_key_exists('_shortcut_set_switch', $route->getRequirements());
    +  }

    Should this be using the static check then?

  2. +++ b/core/modules/shortcut/lib/Drupal/shortcut/Access/SetSwitchAccessCheck.php
    @@ -0,0 +1,40 @@
    +    $account = $request->attributes->get('_account') ?: $GLOBALS['user'];

    $GLOBALS why?

    (Maybe we should let #2062151: Create a current user service to ensure that current account is always available get committed, then use that.)

Seems OK otherwise.

dawehner’s picture

StatusFileSize
new2.42 KB
new22.25 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Yeah, we can do it properly now.

Status:Needs review» Needs work

The last submitted patch, shortcut-1978976-63.patch, failed testing.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new420 bytes
new22.66 KB
FAILED: [[SimpleTest]]: [MySQL] 58,510 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

There we go

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

The last submitted patch, shortcut-1978976-65.patch, failed testing.

dawehner’s picture

Status:Needs work» Needs review

#65: shortcut-1978976-65.patch queued for re-testing.

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

The last submitted patch, shortcut-1978976-65.patch, failed testing.

pfrenssen’s picture

Found some nitpicks:

+++ b/core/modules/shortcut/lib/Drupal/shortcut/Access/SetSwitchAccessCheck.phpundefined
@@ -0,0 +1,57 @@
+class SetSwitchAccessCheck implements StaticAccessCheckInterface {

Why is this called SetSwitchAccessCheck instead of ShortcutSetSwitchAccessCheck? What is a setswitch?

+++ b/core/modules/shortcut/lib/Drupal/shortcut/Access/SetSwitchAccessCheck.phpundefined
@@ -0,0 +1,57 @@
+   * @param \Drupal\Core\Session\AccountInterface $account

Parameter is not documented.

+++ b/core/modules/shortcut/lib/Drupal/shortcut/Access/SetSwitchAccessCheck.phpundefined
@@ -0,0 +1,57 @@
+    // TODO: Implement appliesTo() method.

This todo may be removed.

+++ b/core/modules/shortcut/lib/Drupal/shortcut/Form/SetSwitch.phpundefined
@@ -0,0 +1,238 @@
+class SetSwitch extends FormBase implements ControllerInterface, FormInterface {

ShortcutSetSwitch better describes what this is about.

+++ b/core/modules/shortcut/lib/Drupal/shortcut/Form/SetSwitch.phpundefined
@@ -0,0 +1,238 @@
+   *   The url generator

Missing period.

+++ b/core/modules/shortcut/lib/Drupal/shortcut/ShortcutSetStorageController.phpundefined
@@ -16,6 +21,49 @@
+   *   The module handler;
+   */

Ends in a semicolon instead of a period.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new5.96 KB
new22.6 KB
FAILED: [[SimpleTest]]: [MySQL] 58,525 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

Why is this called SetSwitchAccessCheck instead of ShortcutSetSwitchAccessCheck? What is a setswitch?

Renamed it to SwitchShortcutSetAccessCheck which for sure gives way more context.

This todo may be removed.

Ups!

ShortcutSetSwitch better describes what this is about.

Renamed to SwitchShortcutSet

Some additional changes here and there.

tim.plunkett’s picture

  1. +++ b/core/modules/shortcut/lib/Drupal/shortcut/Access/SwitchShortcutSetAccessCheck.php
    @@ -0,0 +1,57 @@
    +  /**
    +   * Constructs a new SwitchShortcutSetAccessCheck.
    +   *
    +   * @param \Drupal\Core\Session\AccountInterface $account
    +   *   The current user.
    +   */
    +  public function __construct($account) {
    +    $this->account = $account;
    +  }

    Are we actually doing this now? I know most other access checkers still do $request->attributes->get('_account'), I was thinking we'd start passing $account to access checkers when we want to phase out _account.

  2. +++ b/core/modules/shortcut/lib/Drupal/shortcut/Form/SwitchShortcutSet.php
    @@ -0,0 +1,231 @@
    +  /**
    +   * The url generator.
    +   *
    +   * @var \Drupal\Core\Routing\UrlGeneratorInterface
    +   */
    +  protected $urlGenerator;
    ...
    +   *   The url generator.

    #2073813: Add a UrlGenerator helper to FormBase and ControllerBase is close, keep that in mind. Also this should really be "The URL generator."

  3. +++ b/core/modules/shortcut/lib/Drupal/shortcut/Form/SwitchShortcutSet.php
    @@ -0,0 +1,231 @@
    +    $account = $this->getRequest()->attributes->get('_account');
    ...
    +    $account = $this->getRequest()->attributes->get('_account');

    $account = $this->currentUser();

  4. +++ b/core/modules/shortcut/lib/Drupal/shortcut/Form/SwitchShortcutSet.php
    @@ -0,0 +1,231 @@
    +        '@switch-url' => $this->urlGenerator->generateFromPath($this->request->attributes->get('_system_path')),

    $this->getRequest(), we should never rely on $this->request

  5. +++ b/core/modules/shortcut/lib/Drupal/shortcut/Form/SwitchShortcutSet.php
    @@ -0,0 +1,231 @@
    +    $sets = $this->storageController->load($id);
    +    return !empty($sets);

    EntityDisplayModeFormBase, FilterFormatFormControllerBase, and DateFormatFormBase all use entity.query for this, since we don't want the loading.

Status:Needs review» Needs work

The last submitted patch, shortcut-1978976-70.patch, failed testing.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new6.3 KB
new22.58 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch shortcut_set-1978976-73.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Are we actually doing this now? I know most other access checkers still do $request->attributes->get('_account'), I was thinking we'd start passing $account to access checkers when we want to phase out _account.

Whatever you think is the way to get this patch in.

#2073813: Add a UrlGenerator helper to FormBase and ControllerBase is close, keep that in mind. Also this should really be "The URL generator."

So let's not deal with injection then.

I don't even get these failures and for sure they pass locally as every good test.

Status:Needs review» Needs work

The last submitted patch, shortcut_set-1978976-73.patch, failed testing.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new22.61 KB
FAILED: [[SimpleTest]]: [MySQL] 59,149 pass(es), 12 fail(s), and 3 exception(s).
[ View ]

Just yet another rerole.

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

The last submitted patch, shortcut_set-1978976-75.patch, failed testing.

dawehner’s picture

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

#75: shortcut_set-1978976-75.patch queued for re-testing.

Crell’s picture

Status:Needs review» Reviewed & tested by the community

Let's just get this in; we need to get the conversions out of the way.

alexpott’s picture

Status:Reviewed & tested by the community» Needs review
+++ b/core/core.services.yml
@@ -613,7 +613,6 @@ services:
     arguments: ['@request']
-    scope: request
   asset.css.collection_renderer:

@crell are you sure? You are rejecting this change over in #2076411: Remove the request scope from the current user service

tim.plunkett’s picture

Heh. Yeah, this and many other conversions are blocked until that change is made.

xjm’s picture

Thanks for your work on this issue! Please see #1971384-43: [META] Convert page callbacks to controllers for an update on the routing system conversion process.

jibran’s picture

#75: shortcut_set-1978976-75.patch queued for re-testing.

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

The last submitted patch, shortcut_set-1978976-75.patch, failed testing.

h3rj4n’s picture

Issue tags:+Needs reroll

Needs reroll

dawehner’s picture

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

Rerolled ... I moved some of the existing services around as

tim.plunkett’s picture

Issue tags:+Needs reroll
  1. +++ b/core/modules/shortcut/lib/Drupal/shortcut/Access/SwitchShortcutSetAccessCheck.php
    @@ -2,25 +2,25 @@
    -    return array('_access_shortcut_set_switch');
    +    return array('_shortcut_set_switch');

    We use _access_* in most other places, why the switch?

  2. +++ b/core/modules/shortcut/shortcut.routing.yml
    @@ -34,6 +34,15 @@ shortcut.set_edit:
    +  requirements:
    +    _permission: 'administer shortcuts'
    +    _shortcut_set_switch: 'TRUE'

    So this is ALL, is it worth specifying for when the default switch happens?

dawehner’s picture

StatusFileSize
new1.26 KB
new23.79 KB
PASSED: [[SimpleTest]]: [MySQL] 58,449 pass(es).
[ View ]

We use _access_* in most other places, why the switch?

I don't care ... all I did was to rerole the already RTBC patch :) I do agree that for every special case where it is not obvious _access is helpful.
Some examples where the behavior is obvious: _permission, _role.

So this is ALL, is it worth specifying for when the default switch happens?

Good catch.

googletorp’s picture

Status:Needs review» Reviewed & tested by the community

This looks good to me.

tim.plunkett’s picture

+++ b/core/core.services.yml
@@ -616,7 +616,6 @@ services:
-    scope: request

This can't go in as part of this.

alexpott’s picture

Status:Reviewed & tested by the community» Needs work

Patch no longer applies.

David Hernández’s picture

Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new15.85 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Quick reroll

Status:Needs review» Needs work

The last submitted patch, shortcut-1978976-91.patch, failed testing.

dawehner’s picture

Issue tags:+Needs reroll
StatusFileSize
new23.59 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Just did a reroll.

dawehner’s picture

Issue tags:-Needs reroll

... sorry but #91 seems wrong anyway.

vijaycs85’s picture

Status:Needs work» Needs review

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

The last submitted patch, shortcut-1978976-91.patch, failed testing.

David Hernández’s picture

Status:Needs work» Needs review

#93: shortcut-1978976-91.patch queued for re-testing.

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

The last submitted patch, shortcut-1978976-91.patch, failed testing.

tim.plunkett’s picture

Status:Needs work» Needs review
StatusFileSize
new6.08 KB
new23.93 KB
PASSED: [[SimpleTest]]: [MySQL] 59,047 pass(es).
[ View ]
jibran’s picture

Status:Needs review» Reviewed & tested by the community

I don't think anything left here. Coding review is fine so RTBC.

alexpott’s picture

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

Patch no longer applies.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new24.79 KB
PASSED: [[SimpleTest]]: [MySQL] 59,694 pass(es).
[ View ]

There we go.

jibran’s picture

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

Back to RTBC

tim.plunkett’s picture

StatusFileSize
new24.82 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch shortcut-1978976-104.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Reroll.

Xano’s picture

104: shortcut-1978976-104.patch queued for re-testing.

Xano’s picture

104: shortcut-1978976-104.patch queued for re-testing.

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 104: shortcut-1978976-104.patch, failed testing.

Xano’s picture

Issue summary:View changes
Status:Needs work» Needs review
StatusFileSize
new25.07 KB
PASSED: [[SimpleTest]]: [MySQL] 58,046 pass(es).
[ View ]

Re-roll.

Status:Needs review» Needs work

The last submitted patch, 108: drupal_1978976_108.patch, failed testing.

Xano’s picture

Status:Needs work» Needs review

108: drupal_1978976_108.patch queued for re-testing.

Crell’s picture

Status:Needs review» Reviewed & tested by the community

Rollin' rollin' rollin'...

webchick’s picture

Status:Reviewed & tested by the community» Needs work

Sorry, no longer applies.

jibran’s picture

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new24.97 KB
PASSED: [[SimpleTest]]: [MySQL] 59,353 pass(es).
[ View ]
new1.21 KB

The last submitted patch, 113: 1978976-113.patch, failed testing.

jibran’s picture

113: 1978976-113.patch queued for re-testing.

The last submitted patch, 113: 1978976-113.patch, failed testing.

jibran’s picture

Non-pass

Test name Pass Fail Exception
CollapsedDrupal\node\Tests\Views\FilterUidRevisionTest 3 1 0
Message Group Filename Line Function Status
Make sure that the view only returns nodes which match either the node or the revision author. Other FilterUidRevisionTest.php 61 Drupal\node\Tests\Views\FilterUidRevisionTest->testFilter()
jibran’s picture

113: 1978976-113.patch queued for re-testing.

The last submitted patch, 113: 1978976-113.patch, failed testing.

jibran’s picture

113: 1978976-113.patch queued for re-testing.

webchick’s picture

113: 1978976-113.patch queued for re-testing.

tim.plunkett’s picture

StatusFileSize
new24.31 KB
FAILED: [[SimpleTest]]: [MySQL] 59,724 pass(es), 0 fail(s), and 1 exception(s).
[ View ]

Rerolled.

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 122: shortcut-1978976-122.patch, failed testing.

tim.plunkett’s picture

Status:Needs work» Needs review
StatusFileSize
new25.92 KB
FAILED: [[SimpleTest]]: [MySQL] 59,557 pass(es), 0 fail(s), and 1 exception(s).
[ View ]

Didn't reroll properly after #2021779: Decouple shortcuts from menu links.

Status:Needs review» Needs work

The last submitted patch, 124: shortcut-1978976-124.patch, failed testing.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new26.05 KB
PASSED: [[SimpleTest]]: [MySQL] 59,728 pass(es).
[ View ]
new3.37 KB

I realized that core is currently broken in the sense that creating a new shortcut set for a user does not add the default shortcut links.

pfrenssen’s picture

StatusFileSize
new26.02 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Straight reroll.

Status:Needs review» Needs work

The last submitted patch, 127: shortcut_set-1978976-127.patch, failed testing.

mr.baileys’s picture

Status:Needs work» Needs review

127: shortcut_set-1978976-127.patch queued for re-testing.

Status:Needs review» Needs work

The last submitted patch, 127: shortcut_set-1978976-127.patch, failed testing.

Xano’s picture

Status:Needs work» Needs review

127: shortcut_set-1978976-127.patch queued for re-testing.

Status:Needs review» Needs work

The last submitted patch, 127: shortcut_set-1978976-127.patch, failed testing.

InternetDevels’s picture

Status:Needs work» Needs review
StatusFileSize
new26.05 KB
PASSED: [[SimpleTest]]: [MySQL] 63,306 pass(es).
[ View ]

New reroll.

Albert Volkman’s picture

StatusFileSize
new26.11 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,231 pass(es).
[ View ]

Reroll

dawehner’s picture

Status:Needs review» Reviewed & tested by the community
+++ b/core/modules/shortcut/lib/Drupal/shortcut/Form/SwitchShortcutSet.php
@@ -0,0 +1,252 @@
+  /**
+   * Checks access for the shortcut set switch form.
+   *
+   * @param \Drupal\user\UserInterface $user
+   *   The user whose shortcut sets are being switched.
+   *
+   * @return bool|null
+   *   AccessInterface::ALLOW, AccessInterface::DENY, or AccessInterface::KILL.
+   */
+  public function checkAccess(UserInterface $user) {
+    $account = $this->currentUser();
+    if ($account->hasPermission('administer shortcuts')) {
+      // Administrators can switch anyone's shortcut set.
+      return AccessInterface::ALLOW;
+    }
+
+    if (!$account->hasPermission('switch shortcut sets')) {
+      // The user has no permission to switch anyone's shortcut set.
+      return AccessInterface::DENY;
+    }
+
+    if ($account->id() == $user->id()) {
+      // Users with the 'switch shortcut sets' permission can switch their own
+      // shortcuts sets.
+      return AccessInterface::ALLOW;
+    }
+    return AccessInterface::DENY;
+  }

I don't really like to move that away from its own access check service what yeah this is fine. Note: Technically these values aren't booleans.

alexpott’s picture

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

drupal-convert_shortcut_set_to_controller-1978976-134.patch no longer applies.

error: patch failed: core/modules/shortcut/shortcut.admin.inc:1
error: core/modules/shortcut/shortcut.admin.inc: patch does not apply
longwave’s picture

Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new26.09 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,186 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Status:Needs review» Needs work

The last submitted patch, 137: 1978976-shortcut_set_switch-137.patch, failed testing.

longwave’s picture

StatusFileSize
new26.09 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed.
[ View ]
longwave’s picture

Status:Needs work» Needs review
andypost’s picture

Status:Needs review» Reviewed & tested by the community

back to rtbc

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 139: 1978976-shortcut_set_switch-137.patch, failed testing.

longwave’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 139: 1978976-shortcut_set_switch-137.patch, failed testing.

pcambra’s picture

Status:Needs work» Needs review
StatusFileSize
new26.12 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed.
[ View ]

Just a plain re roll on the yaml files, hopefully the testbot would allow us to RTBC this.

Status:Needs review» Needs work

The last submitted patch, 145: 1978976-shortcut_set_switch-145.patch, failed testing.

Xano’s picture

Status:Needs work» Needs review
StatusFileSize
new26.1 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed.
[ View ]
new1.45 KB

Status:Needs review» Needs work

The last submitted patch, 147: drupal_1978976_147.patch, failed testing.

Xano’s picture

Status:Needs work» Needs review
StatusFileSize
new26.06 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,728 pass(es).
[ View ]
new568 bytes

Status:Needs review» Needs work

The last submitted patch, 149: drupal_1978976_149.patch, failed testing.

Xano’s picture

149: drupal_1978976_149.patch queued for re-testing.

Crell’s picture

  1.     foo

    Is this random patch flotsam?

  2. +++ b/core/modules/shortcut/lib/Drupal/shortcut/Form/SwitchShortcutSet.php
    @@ -0,0 +1,252 @@
    +  /**
    +   * Determines if a shortcut set exists already.
    +   *
    +   * @param string $id
    +   *   The set ID to check.
    +   *
    +   * @return bool
    +   *   TRUE if the shortcut set exists, FALSE otherwise.
    +   */
    +  public function exists($id) {
    +    return (bool) $this->storageController->getQuery()
    +      ->condition('id', $id)
    +      ->execute();
    +  }

    This feels like it belongs on the shortcut storage controller instead. There's nothing form-specific about it.

  3. +++ b/core/modules/shortcut/lib/Drupal/shortcut/Form/SwitchShortcutSet.php
    @@ -0,0 +1,252 @@
    +   * @param \Drupal\user\UserInterface $user
    +   *   The user whose shortcut sets are being switched.

    Are you sure? I thought the current user was passed in, not the contextual user.

  4. +++ b/core/modules/shortcut/lib/Drupal/shortcut/Form/SwitchShortcutSet.php
    @@ -0,0 +1,252 @@
    +   * @return bool|null

    I don't think null is ever allowed anymore.

  5. +++ b/core/modules/shortcut/lib/Drupal/shortcut/Form/SwitchShortcutSet.php
    @@ -0,0 +1,252 @@
    +    if (!$account->hasPermission('switch shortcut sets')) {
    +      // The user has no permission to switch anyone's shortcut set.
    +      return AccessInterface::DENY;
    +    }

    Shouldn't this be an AccessInterface::KILL?

tim.plunkett’s picture

152.1 that's if you use git show/git format-patch and not git diff.
152.2 Consider opening a follow-up, but this is the same pattern used for all exists callbacks

152.5 Generally no, deny is fine I think.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new26.01 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,625 pass(es), 145 fail(s), and 32 exception(s).
[ View ]
new5.67 KB

This feels like it belongs on the shortcut storage controller instead. There's nothing form-specific about it.

While it is TRUE that we do things wrong all over the place thought here it is used for the machine name validation BS.

Status:Needs review» Needs work

The last submitted patch, 154: shortcut-1978976-154.patch, failed testing.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new26 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,652 pass(es), 141 fail(s), and 1 exception(s).
[ View ]
new692 bytes

This could be it.

Status:Needs review» Needs work

The last submitted patch, 156: shortcut_set-1978976-156.patch, failed testing.

vijaycs85’s picture

Status:Needs work» Needs review
StatusFileSize
new25.86 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 1978976-shortcut_set_controller-158.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new968 bytes

\Drupal\shortcut\Form\SwitchShortcutSet::checkAccess is not normal access callback and doesn't have $account argument.

dawehner’s picture

Status:Needs review» Reviewed & tested by the community

This is certainly good to go,

shivachevva’s picture

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 158: 1978976-shortcut_set_controller-158.patch, failed testing.

shivachevva’s picture

Status:Needs work» Needs review
StatusFileSize
new25.53 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,013 pass(es).
[ View ]

Updating with re-rolled patch. (shortcut files were moved.)

David Hernández’s picture

Status:Needs review» Reviewed & tested by the community

Good to go, AFAIK.

alexpott’s picture

Status:Reviewed & tested by the community» Needs work
  1. +++ b/core/modules/shortcut/lib/Drupal/shortcut/Form/SwitchShortcutSet.php
    @@ -0,0 +1,250 @@
    +use Drupal\Core\Session\AccountInterface;

    Not used.

  2. +++ b/core/modules/shortcut/lib/Drupal/shortcut/Form/SwitchShortcutSet.php
    @@ -0,0 +1,250 @@
    +        // @todo Convert once https://drupal.org/node/2073813 is in.
    +        '@switch-url' => $this->urlGenerator()->generateFromPath($this->getRequest()->attributes->get('_system_path')),

    https://drupal.org/node/2073813 is in

vijaycs85’s picture

Status:Needs work» Needs review
StatusFileSize
new25.39 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,082 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
new1.55 KB

Updating...

alexpott’s picture

+++ b/core/modules/shortcut/lib/Drupal/shortcut/Form/SwitchShortcutSet.php
@@ -239,7 +237,7 @@ public function checkAccess() {
-    if ($account->id() == $account->id()) {
+    if ($this->user->id() == $account->id()) {
       // Users with the 'switch shortcut sets' permission can switch their own
       // shortcuts sets.
       return AccessInterface::ALLOW;

Doesn't the change in the interdiff mean we are missing test coverage for this?

Status:Needs review» Needs work

The last submitted patch, 165: 1978976-shortcut_set_controller-165.patch, failed testing.

dawehner’s picture

+++ b/core/modules/shortcut/lib/Drupal/shortcut/Form/SwitchShortcutSet.php
@@ -189,8 +188,7 @@ public function submitForm(array &$form, array &$form_state) {
+        '@switch-url' => $this->url($this->getRequest()->attributes->get('_system_path')),

This can't work, given that _system_path is a path and not a route name.

kim.pepper’s picture

Status:Needs work» Needs review
StatusFileSize
new24.3 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,926 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
new4.83 KB
+++ b/core/modules/shortcut/lib/Drupal/shortcut/Form/SwitchShortcutSet.php
@@ -0,0 +1,248 @@
+      $default_set = $this->storageController->getDefaultSet($this->user);
+      $set = $this->storageController->create(array(
+        'id' => $form_state['values']['id'],
+        'label' => $form_state['values']['label'],
+        'links' => $default_set->getShortcuts(),
+      ));

This isn't necessary as the ShortcutSetStorage does this for us already.

In addition, I've made a number of changes including:

  • Moving to PSR4
  • Rename $storageController to $shortcutSetStorage
  • Using RouteMatchInterface to get the switch url

Status:Needs review» Needs work

The last submitted patch, 169: 1978976-shortcut-set-controller-169.patch, failed testing.

kim.pepper’s picture

Status:Needs work» Needs review
StatusFileSize
new24.46 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,923 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new1.3 KB

The access check needs the shortcut set user passed in too.

Status:Needs review» Needs work

The last submitted patch, 171: 1978976-shortcut-set-controller-171.patch, failed testing.

kim.pepper’s picture

Status:Needs work» Needs review
StatusFileSize
new24.61 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,041 pass(es).
[ View ]
new721 bytes

Somewhere along the way, the check for $account->hasPermission('access shortcuts') got dropped. Re-adding.

Crell’s picture

Status:Needs review» Reviewed & tested by the community

This was RTBC before, so "it compiles, ship it!"

(Yes I read the patch over and didn't notice anything obviously wrong, but didn't fine-tooth-comb it.)

alexpott’s picture

Status:Reviewed & tested by the community» Fixed

Committed a8a5f10 and pushed to 8.x. Thanks!

  • alexpott committed a8a5f10 on
    Issue #1978976 by dawehner, kgoel, tim.plunkett, pcambra, longwave,...
lussoluca’s picture

StatusFileSize
new1.88 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,393 pass(es).
[ View ]

The constructor of ShortcutSetStorage should use ConfigFactoryInterface type hint instead of ConfigFactory otherwise no contrib can override the config.factory service (as webprofiler does).

vijaycs85’s picture

Status:Fixed» Needs review

Valid point. let's test it. Hope it is OK to commit as part of this issue as it is a minor change.

Crell’s picture

Status:Needs review» Reviewed & tested by the community
alexpott’s picture

Status:Reviewed & tested by the community» Fixed

Committed 41c6402 and pushed to 8.x. Thanks!

  • alexpott committed 41c6402 on 8.x
    Issue #1978976 followup by lussoluca: Convert shortcut_set_switch to a...

Status:Fixed» Closed (fixed)

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