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

Assigned:Unassigned» kgoel

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.

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.

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.

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

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.

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.

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.

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.

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.

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

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.

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.

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

Status:Needs review» Needs work

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.

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.

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 ]

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

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

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

Status:Needs review» Needs work

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 ]

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.

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

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.

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.

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.

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.

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.

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.

Assigned:kgoel» Unassigned

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

There we go.

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

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

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.

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.

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.

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

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.

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.

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

No, it is known, see menu_item_route_access().

The account object should maybe copied from the parent request.

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

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

Rerolled.

  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.

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.

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.

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.

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.

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.

  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.

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.

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.

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

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

Status:Needs review» Reviewed & tested by the community

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

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

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

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.

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

Issue tags:+Needs reroll

Needs reroll

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

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?

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.

Status:Needs review» Reviewed & tested by the community

This looks good to me.

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

This can't go in as part of this.

Status:Reviewed & tested by the community» Needs work

Patch no longer applies.

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.

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

Just did a reroll.

Issue tags:-Needs reroll

... sorry but #91 seems wrong anyway.

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.

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.

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

Status:Needs review» Reviewed & tested by the community

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

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

Patch no longer applies.

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

There we go.

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

Back to RTBC

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.

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

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.

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.

Status:Needs work» Needs review

108: drupal_1978976_108.patch queued for re-testing.

Status:Needs review» Reviewed & tested by the community

Rollin' rollin' rollin'...

Status:Reviewed & tested by the community» Needs work

Sorry, no longer applies.

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.

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

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

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

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

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

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

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

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.

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.

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.

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.

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.

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.

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

New reroll.

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

Reroll

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.

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

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.

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

Status:Needs work» Needs review

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.

Status:Needs work» Needs review

Status:Needs review» Needs work

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

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.

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.

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.

149: drupal_1978976_149.patch queued for re-testing.

  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?

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.

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.

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.