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.

CommentFileSizeAuthor
#177 1978976-177.patch1.88 KBlussoluca
#173 interdiff.txt721 byteskim.pepper
#173 1978976-shortcut-set-controller-173.patch24.61 KBkim.pepper
#171 interdiff.txt1.3 KBkim.pepper
#171 1978976-shortcut-set-controller-171.patch24.46 KBkim.pepper
#169 interdiff.txt4.83 KBkim.pepper
#169 1978976-shortcut-set-controller-169.patch24.3 KBkim.pepper
#165 1978976-diff-162-165.txt1.55 KBvijaycs85
#165 1978976-shortcut_set_controller-165.patch25.39 KBvijaycs85
#162 1978976-shortcut_set_controller-162.patch25.53 KBshivachevva
#158 1978976-diff-156-158.txt968 bytesvijaycs85
#158 1978976-shortcut_set_controller-158.patch25.86 KBvijaycs85
#156 interdiff.txt692 bytesdawehner
#156 shortcut_set-1978976-156.patch26 KBdawehner
#154 interdiff.txt5.67 KBdawehner
#154 shortcut-1978976-154.patch26.01 KBdawehner
#149 interdiff.txt568 bytesXano
#149 drupal_1978976_149.patch26.06 KBXano
#147 interdiff.txt1.45 KBXano
#147 drupal_1978976_147.patch26.1 KBXano
#145 1978976-shortcut_set_switch-145.patch26.12 KBpcambra
#139 1978976-shortcut_set_switch-137.patch26.09 KBlongwave
#137 1978976-shortcut_set_switch-137.patch26.09 KBlongwave
#134 drupal-convert_shortcut_set_to_controller-1978976-134.patch26.11 KBAlbert Volkman
#133 drupal-convert_shortcut_set_to_controller-1978976-133.patch26.05 KBInternetDevels
#127 shortcut_set-1978976-127.patch26.02 KBpfrenssen
#126 interdiff.txt3.37 KBdawehner
#126 shortcut_set-1978976.patch26.05 KBdawehner
#124 shortcut-1978976-124.patch25.92 KBtim.plunkett
#122 shortcut-1978976-122.patch24.31 KBtim.plunkett
#113 interdiff.txt1.21 KBjibran
#113 1978976-113.patch24.97 KBjibran
#108 drupal_1978976_108.patch25.07 KBXano
#104 shortcut-1978976-104.patch24.82 KBtim.plunkett
#102 shortcut-1978976-102.patch24.79 KBdawehner
#99 shortcut-1978976-99.patch23.93 KBtim.plunkett
#99 interdiff.txt6.08 KBtim.plunkett
#93 shortcut-1978976-91.patch23.59 KBdawehner
#91 shortcut-1978976-91.patch15.85 KBDavid Hernández
#87 shortcut-1978976-87.patch23.79 KBdawehner
#87 interdiff.txt1.26 KBdawehner
#85 shortcut-1978976-85.patch23.89 KBdawehner
#75 shortcut_set-1978976-75.patch22.61 KBdawehner
#73 shortcut_set-1978976-73.patch22.58 KBdawehner
#73 interdiff.txt6.3 KBdawehner
#70 shortcut-1978976-70.patch22.6 KBdawehner
#70 interdiff.txt5.96 KBdawehner
#65 shortcut-1978976-65.patch22.66 KBdawehner
#65 interdiff.txt420 bytesdawehner
#63 shortcut-1978976-63.patch22.25 KBdawehner
#63 interdiff.txt2.42 KBdawehner
#61 shortcut-1978976-61.patch21.9 KBdawehner
#56 drupal-1978976-56.patch23.43 KBdawehner
#56 interdiff.txt8.8 KBdawehner
#54 shortcut-1978976-54.patch22.99 KBdawehner
#54 interdiff.txt2.69 KBdawehner
#52 drupal-1978976-52.patch22.75 KBdawehner
#52 interdiff.txt1 KBdawehner
#50 drupal-1978976-50.patch22.71 KBdawehner
#50 interdiff.txt1.32 KBdawehner
#48 drupal-1978976-48.patch23.38 KBdawehner
#45 drupal-1978976-45.patch15.51 KBdawehner
#45 interdiff.txt1.04 KBdawehner
#42 interdiff.txt1.31 KBdawehner
#42 drupal-1978976-42.patch14.47 KBdawehner
#36 shortcut-1978976-36.patch14.41 KBkgoel
#36 interdiff.txt450 byteskgoel
#34 shortcut-1978976-34.patch14.41 KBtim.plunkett
#34 interdiff.txt9.15 KBtim.plunkett
#31 convert_shortcut_set_switch-31.patch13.8 KBkgoel
#31 interdiff.txt877 byteskgoel
#26 convert_shortcut_set_switch-26.patch13.79 KBkgoel
#26 interdiff.txt1.71 KBkgoel
#20 convert_shortcut_set_switch-20.patch13.74 KBkgoel
#20 interdiff.txt1.24 KBkgoel
#19 convert_shortcut_set_switch-19.patch13.77 KBkgoel
#19 interdiff.txt2.47 KBkgoel
#16 convert_shortcut_set_switch-1978976-16.patch13.62 KBkgoel
#16 interdiff.txt652 byteskgoel
#14 convert_shortcut_set_switch-1978976-14.patch13.61 KBkgoel
#14 interdiff.txt1.38 KBkgoel
#12 convert_shortcut_set_switch-1978976-12.patch13.63 KBkgoel
#9 convert_shortcut_set_switch-1978976-9.patch13.53 KBkgoel
#6 convert_shortcut_set_switch-1978976-6.patch13.42 KBkgoel
#4 convert_shortcut_set_switch-1978976-4.patch13.42 KBkgoel
#4 interdiff.txt13.59 KBkgoel
#2 convert_shortcut_set_switch-7396768-2.patch13.42 KBkgoel
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

kgoel’s picture

Assigned: Unassigned » kgoel
kgoel’s picture

Status: Active » Needs review
FileSize
13.42 KB

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
FileSize
13.59 KB
13.42 KB

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

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

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

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
FileSize
1.38 KB
13.61 KB

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
FileSize
652 bytes
13.62 KB

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
FileSize
2.47 KB
13.77 KB

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

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
FileSize
1.71 KB
13.79 KB
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
dawehner’s picture

Status: Needs review » Needs work
kgoel’s picture

Status: Needs work » Needs review
FileSize
877 bytes
13.8 KB
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
FileSize
9.15 KB
14.41 KB

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

FileSize
450 bytes
14.41 KB

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
FileSize
14.47 KB
1.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
FileSize
1.04 KB
15.51 KB

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

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
FileSize
1.32 KB
22.71 KB

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
FileSize
1 KB
22.75 KB

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
FileSize
2.69 KB
22.99 KB
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
FileSize
8.8 KB
23.43 KB

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

FileSize
21.9 KB

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

FileSize
2.42 KB
22.25 KB

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
FileSize
420 bytes
22.66 KB

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
FileSize
5.96 KB
22.6 KB

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
FileSize
6.3 KB
22.58 KB

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

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

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

FileSize
1.26 KB
23.79 KB

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

Quick reroll

Status: Needs review » Needs work

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

dawehner’s picture

Issue tags: +Needs reroll
FileSize
23.59 KB

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
FileSize
6.08 KB
23.93 KB
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
FileSize
24.79 KB

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

FileSize
24.82 KB

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

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
FileSize
24.97 KB
1.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

FileSize
24.31 KB

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

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
FileSize
26.05 KB
3.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

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

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

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

New reroll.

Albert Volkman’s picture

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

Status: Needs review » Needs work

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

longwave’s picture

FileSize
26.09 KB
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
FileSize
26.12 KB

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
FileSize
26.1 KB
1.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
FileSize
26.06 KB
568 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
FileSize
26.01 KB
5.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
FileSize
26 KB
692 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
FileSize
25.86 KB
968 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
FileSize
25.53 KB

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
FileSize
25.39 KB
1.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
FileSize
24.3 KB
4.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
FileSize
24.46 KB
1.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
FileSize
24.61 KB
721 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

FileSize
1.88 KB

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.