Shortcut cleanup: Remove duplicated code and deprecate legacy functions

First patch attempt. This is my first time with this stuff, so feel free to add links I may read on the subject along with your comments.

shortcut_set_load should be removed, no reason for to to exist

@tim.plunkett I didn't see the way to remove shortcut_set_load since we need the callback for the 'exists' checks on machin_name form elements. I removed explicit calls to it anywhere else.

@jibran I had to do a bit more work than described on #4 to make tests pass. I also created the ShortcutManagerInterface.php as a placeholder in order to open discussion on what methods should make it there.

Marking as needs review to trigger tests.

Crosslinking: #2021779: Decouple shortcuts from menu links

First up - great job!

1. +++ b/core/modules/shortcut/lib/Drupal/shortcut/Access/LinkAccessCheck.php
   @@ -8,6 +8,7 @@
   +use Drupal\shortcut\ShortcutManager;
   
   @@ -17,6 +18,23 @@
   +   * @var \Drupal\shortcut\ShortcutManager
   ...
   +  function __construct(ShortcutManager $shortcut_manager) {
   
   +++ b/core/modules/shortcut/lib/Drupal/shortcut/Access/ShortcutSetAccessController.php
   @@ -0,0 +1,79 @@
   +use Drupal\shortcut\ShortcutManager;
   ...
   +   * @var \Drupal\shortcut\ShortcutManager
   ...
   +   * @param \Drupal\shortcut\ShortcutManager $shortcut_manager
   ...
   +  function __construct(ShortcutManager $shortcut_manager) {
   
   +++ b/core/modules/shortcut/lib/Drupal/shortcut/Access/ShortcutSetEditAccessCheck.php
   @@ -10,6 +10,7 @@
   +use Drupal\shortcut\ShortcutManager;
   
   @@ -17,6 +18,23 @@
   +   * @var \Drupal\shortcut\ShortcutManager
   ...
   +  function __construct(ShortcutManager $shortcut_manager) {
   
   +++ b/core/modules/shortcut/lib/Drupal/shortcut/Access/ShortcutSetSwitchAccessCheck.php
   @@ -10,6 +10,7 @@
   +use Drupal\shortcut\ShortcutManager;
   
   @@ -17,6 +18,23 @@
   +   * @var \Drupal\shortcut\ShortcutManager
   ...
   +   * @param \Drupal\shortcut\ShortcutManager $shortcut_manager
   ...
   +  function __construct(ShortcutManager $shortcut_manager) {
   
   +++ b/core/modules/shortcut/lib/Drupal/shortcut/Controller/ShortcutSetController.php
   @@ -8,7 +8,9 @@
   +use Drupal\shortcut\ShortcutManager;
   
   @@ -20,6 +22,43 @@
   +   * @var \Drupal\shortcut\ShortcutManager
   ...
   +   * @param \Drupal\shortcut\ShortcutManager $shortcut_manager
   ...
   +  function __construct(CsrfTokenGenerator $csrf_token, ShortcutManager $shortcut_manager) {
   

   Can we type hint on the interface

2. +++ b/core/modules/shortcut/lib/Drupal/shortcut/Access/LinkAccessCheck.php
   @@ -29,8 +47,8 @@ public function appliesTo() {
   +    if ($shortcut_set = entity_load('shortcut_set', $set_name)) {
   

   Can we inject the shortcut storage controller (may need to inject entity.manager and then store the shortcut storage controller on the object)

3. +++ b/core/modules/shortcut/lib/Drupal/shortcut/Controller/ShortcutSetController.php
   @@ -43,7 +82,7 @@ public function addShortcutLinkInline(ShortcutSetInterface $shortcut_set, Reques
   +      \Drupal::service('shortcut.manager')->adminAddLink($link, $shortcut_set);
   

   Can this be injected?

4. +++ b/core/modules/shortcut/lib/Drupal/shortcut/Plugin/Block/ShortcutsBlock.php
   @@ -25,8 +25,9 @@ class ShortcutsBlock extends BlockBase {
   +    $shortcutManager = \Drupal::service('shortcut.manager');
   

   We can implement ContainerFactoryPluginInterface and inject the shortcut manager

5. +++ b/core/modules/shortcut/lib/Drupal/shortcut/ShortcutManager.php
   @@ -0,0 +1,257 @@
   + * Contains \Drupal\book\ShortcutManager.
   

   book => shortcut

6. +++ b/core/modules/shortcut/lib/Drupal/shortcut/ShortcutManager.php
   @@ -0,0 +1,257 @@
   +   * @return
   

   Missing type

7. +++ b/core/modules/shortcut/lib/Drupal/shortcut/ShortcutManager.php
   @@ -0,0 +1,257 @@
   +    $shortcut_sets = &drupal_static(__FUNCTION__, array());
   

   Should this be a public static property on the object instead?

8. +++ b/core/modules/shortcut/lib/Drupal/shortcut/ShortcutManager.php
   @@ -0,0 +1,257 @@
   +      $account = \Drupal::currentUser();
   

   We can inject this from the services.yml file

9. +++ b/core/modules/shortcut/lib/Drupal/shortcut/ShortcutManager.php
   @@ -0,0 +1,257 @@
   +      $shortcut_set = entity_load('shortcut_set', $shortcut_set_name);
   

   We have the storage controller, we should use it to load.

10. +++ b/core/modules/shortcut/lib/Drupal/shortcut/ShortcutManager.php
    @@ -0,0 +1,257 @@
    +      $shortcut_set = shortcut_default_set($account);
    

    Can this become a method on the manager too? We would retain and mark the old function as @deprecated, and just make it wrap a call to the manager.

11. +++ b/core/modules/shortcut/lib/Drupal/shortcut/ShortcutManager.php
    @@ -0,0 +1,257 @@
    +   * @return
    

    Missing type (bool)

12. +++ b/core/modules/shortcut/lib/Drupal/shortcut/ShortcutManager.php
    @@ -0,0 +1,257 @@
    +  function shortcutSetTitleExists($title) {
    ...
    +  function validLink($path) {
    ...
    +  function adminAddLink($shortcut_link, &$shortcut_set) {
    ...
    +  function renderableLinks($shortcut_set = NULL) {
    ...
    +  function shortcutSetEditAccess($shortcut_set = NULL) {
    ...
    +  function shortcutSetSwitchAccess($account = NULL) {
    

    missing public|private

13. +++ b/core/modules/shortcut/lib/Drupal/shortcut/ShortcutManager.php
    @@ -0,0 +1,257 @@
    +    $sets = $this->entityManager->getStorageController('shortcut_set')->loadMultiple();
    

    Can we use the load by properties method, passing the title instead of loading them all?

14. +++ b/core/modules/shortcut/lib/Drupal/shortcut/ShortcutManager.php
    @@ -0,0 +1,257 @@
    +    return (!url_is_external($path) && ($this->routeProvider->getRoutesByPattern('/' . $path)->count() > 0 || menu_get_item($path))) || empty($path) || $path == '<front>';
    

    Url::isExternal($path); instead of url_is_external()

15. +++ b/core/modules/shortcut/lib/Drupal/shortcut/ShortcutManager.php
    @@ -0,0 +1,257 @@
    +   * Adds a link to the end of a shortcut set, keeping within a prescribed limit.
    ...
    +   * This function can be used, for example, when a new shortcut link is added to
    

    > 80 chars

16. +++ b/core/modules/shortcut/lib/Drupal/shortcut/ShortcutManager.php
    @@ -0,0 +1,257 @@
    +    $menu_link = entity_create('menu_link', $shortcut_link);
    

    We have the entityManager, we can use it to get the storage controller for menu links and call ::create on that.

17. +++ b/core/modules/shortcut/lib/Drupal/shortcut/ShortcutManager.php
    @@ -0,0 +1,257 @@
    +    $weight = -50;
    

    objects are always passed as reference - do we need the & in the function signature any more?

18. +++ b/core/modules/shortcut/lib/Drupal/shortcut/ShortcutManager.php
    @@ -0,0 +1,257 @@
    +   * @return
    

    Missing type (array)

19. +++ b/core/modules/shortcut/lib/Drupal/shortcut/ShortcutManager.php
    @@ -0,0 +1,257 @@
    +    return menu_tree('shortcut-' . $shortcut_set->id());
    

    In order to make this object unit-testable, the call to menu_tree (which has no OO equivalent) should be inside a wrapper method, that way we can mock that method. Thoughts?

20. +++ b/core/modules/shortcut/lib/Drupal/shortcut/ShortcutManager.php
    @@ -0,0 +1,257 @@
    +   * @param \Drupal\shortcut\Entity\ShortcutSet $shortcut_set
    

    The function signature doesn't contain a type-hint to match this

21. +++ b/core/modules/shortcut/lib/Drupal/shortcut/ShortcutManager.php
    @@ -0,0 +1,257 @@
    +   * @return
    ...
    +   * @return
    

    Missing type (bool)

22. +++ b/core/modules/shortcut/lib/Drupal/shortcut/ShortcutManager.php
    @@ -0,0 +1,257 @@
    +    $account = \Drupal::currentUser();
    

    Can we inject this?

23. +++ b/core/modules/shortcut/lib/Drupal/shortcut/ShortcutManager.php
    @@ -0,0 +1,257 @@
    +   * @param object $account
    

    Is it just an object? Or is it an AccountInterface? or perhaps a UserInterface?. Same story in function signature.

24. +++ b/core/modules/shortcut/lib/Drupal/shortcut/ShortcutManagerInterface.php
    @@ -0,0 +1,13 @@
    +  // TODO: Discuss what should be the signature of this interface.
    

    This should contain any public method found in the default implementation. All of the function docblocks can be moved here and replaced with {@inheritdoc} in the default implementation.

25. +++ b/core/modules/shortcut/lib/Drupal/shortcut/ShortcutManagerInterface.php
    @@ -0,0 +1,13 @@
    \ No newline at end of file
    
    +++ b/core/modules/shortcut/shortcut.services.yml
    @@ -1,15 +1,22 @@
    \ No newline at end of file
    

    Whitespace

26. +++ b/core/modules/shortcut/lib/Drupal/shortcut/ShortcutSetFormController.php
    @@ -64,7 +64,7 @@ public function validate(array $form, array &$form_state) {
    +    if ($form_state['values']['label'] != $entity->label() && \Drupal::service('shortcut.manager')->shortcutSetTitleExists($form_state['values']['label'])) {
    

    We can inject the shortcut manager by implementing ContainerInjectionInterface.

27. +++ b/core/modules/shortcut/lib/Drupal/shortcut/ShortcutSetStorageController.php
    @@ -48,7 +48,7 @@ public function assignUser(ShortcutSetInterface $shortcut_set, $account) {
    +    drupal_static_reset('currentDisplayedSet');
    

    Does this work? I think you might need a 'resetCache' method on the manager. Inject the manager (by implementing EntityControllerInterface) and call the resetCache method here instead.

28. +++ b/core/modules/shortcut/shortcut.admin.inc
    @@ -322,33 +322,8 @@ function shortcut_link_add_submit($form, &$form_state) {
    -function shortcut_admin_add_link($shortcut_link, &$shortcut_set) {
    
    +++ b/core/modules/shortcut/shortcut.module
    @@ -144,77 +144,6 @@ function shortcut_admin_paths() {
    -function shortcut_set_edit_access($shortcut_set = NULL) {
    ...
    -function shortcut_set_switch_access($account = NULL) {
    ...
    -function shortcut_link_access($menu_link) {
    
    @@ -244,24 +173,6 @@ function shortcut_set_load($id) {
    -function shortcut_set_reset_link_weights(&$shortcut_set) {
    
    @@ -295,45 +206,6 @@ function shortcut_set_unassign_user($account) {
    -function shortcut_current_displayed_set($account = NULL) {
    
    @@ -366,66 +238,6 @@ function shortcut_default_set($account = NULL) {
    -function shortcut_set_title_exists($title) {
    ...
    -function shortcut_valid_link($path) {
    

    This needs to be retained as just a wrapper to the service and marked @deprecated

29. +++ b/core/modules/shortcut/shortcut.module
    @@ -466,13 +279,13 @@ function shortcut_preprocess_page(&$variables) {
    -      $query['token'] = drupal_get_token('shortcut-add-link');
    ...
    +      $query['token'] = \Drupal::csrfToken()->get('shortcut-add-link');
    

    Out of scope?

Finally can we get a follow-up to add some phpunit tests for the manager?

@larowlan thank you for your comments. I implemented your suggestions, but I have some comments about 27.

Does this work? I think you might need a 'resetCache' method on the manager. Inject the manager (by implementing EntityControllerInterface) and call the resetCache method here instead.

I modified the implementation adding the complete class namespace and adding the method name to avoid cache clashes. I checked and found that CacheBackendInterface is using the same approach.

Also I didn't wrap

shortcut_link_access

because is not used anywhere in the current 8.x branch and polluted unnecessarily the manager.

Tests turned green in my local.

@larowlan thank you for your comments. I implemented your suggestions, but I have some comments about 27.

Does this work? I think you might need a 'resetCache' method on the manager. Inject the manager (by implementing EntityControllerInterface) and call the resetCache method here instead.

I modified the implementation adding the complete class namespace and adding the method name to avoid cache clashes. I checked and found that CacheBackendInterface is using the same approach.

Also I didn't wrap shortcut_link_access because is not used anywhere in the current 8.x branch and polluted unnecessarily the manager.

Tests turned green in my local.

Ooops, there were some changes pending to commit.

Adding feedback from #12.

Also, @jibran asked me in IRC to clarify the injection of the current_user service. I could not find an example in all of our codebase on how to inject it. If you pay attention to core.services.yml you'll see that the current_user service has scope: request in its declaration. This makes services of scope wider than request to not be able to inject it. Also, if you see the implementation of \Drupal::currentUser() you'll see that we are using the dependency injection container to get the current_user. What I did was mimic that behavior and inject the dependency injection container to the manager to be able to inject the current_user service in the constructor (see the constructor of ShortcutManager.php to make sense out of this).

A part from that, I updated the constructor documentation à la BookManager.php.

Read #2076411: Remove the request scope from the current user service for more details about the current_user injection problem. The solution there was to remove the request scope from the current_user service to unblock these issues and deal with it later in #2102027: Add back the request scope to the current user service.

I guess that I'll need to rework the issue to inject the current_user service as all of the others.

+++ b/core/modules/shortcut/lib/Drupal/shortcut/Access/ShortcutSetAccessController.php
@@ -27,7 +27,7 @@ class ShortcutSetAccessController extends EntityAccessController implements Enti
+   * Constructs ShortcutSetAccessController objects.

+++ b/core/modules/shortcut/lib/Drupal/shortcut/Access/ShortcutSetEditAccessCheck.php
@@ -25,7 +25,7 @@ class ShortcutSetEditAccessCheck implements StaticAccessCheckInterface {
+   * Constructs ShortcutSetEditAccessCheck objects.

+++ b/core/modules/shortcut/lib/Drupal/shortcut/Access/ShortcutSetSwitchAccessCheck.php
@@ -25,7 +25,7 @@ class ShortcutSetSwitchAccessCheck implements StaticAccessCheckInterface {
+   * Constructs ShortcutSetSwitchAccessCheck objects.

+++ b/core/modules/shortcut/lib/Drupal/shortcut/Controller/ShortcutSetController.php
@@ -36,7 +36,7 @@ class ShortcutSetController extends ControllerBase {
+   * Constructs ShortcutSetController objects.

object

Yeah we've seen issues with the current_user service on other issues.

Related bug in simpletest #2102539: Current user service should be updated in drupalLogin() and drupalLogout()

Reworking this to (hopefully) get it back to green.

Rerolled #25.

Implemenation of shortcutSetEditAccess() and shortcutSetSwitchAccess() in manager looks strange suppose they should be access checkrs, also it's not clear the reason of moving other methods to manager because not sure there's any sense to make them overridable

1. +++ b/core/modules/shortcut/lib/Drupal/shortcut/ShortcutManager.php
   @@ -0,0 +1,265 @@
   +  public function shortcutSetTitleExists($title) {
   

   suppose this function should live in controller where used

2. +++ b/core/modules/shortcut/lib/Drupal/shortcut/ShortcutManager.php
   @@ -0,0 +1,265 @@
   +  public function validLink($path) {
   

   same

3. +++ b/core/modules/shortcut/lib/Drupal/shortcut/ShortcutManager.php
   @@ -0,0 +1,265 @@
   +  public function shortcutSetEditAccess(ShortcutSetInterface $shortcut_set = NULL, AccountInterface $user = NULL) {
   ...
   +  public function shortcutSetSwitchAccess(AccountInterface $account = NULL, AccountInterface $user = NULL) {
   

   Suppose better leave this ones as access controllers

4. +++ b/core/modules/shortcut/lib/Drupal/shortcut/Tests/ShortcutLinksTest.php
   @@ -60,7 +60,7 @@ public function testShortcutLinkAdd() {
   -      $saved_set = shortcut_set_load($set->id());
   +      $saved_set = entity_load('shortcut_set', $set->id());
   
   @@ -94,7 +94,7 @@ public function testShortcutLinkRename() {
   +    $saved_set = entity_load('shortcut_set', $set->id());
   

   Better to file new issue to get rid of shortcut_set_load()

Filed #2161371: Deprecate shortcut_set_load() in favour of entity_load()

1. +++ b/core/modules/shortcut/lib/Drupal/shortcut/Plugin/Block/ShortcutsBlock.php
   @@ -20,14 +23,37 @@
   +  /**
   +   * @param \Drupal\shortcut\ShortcutManagerInterface $shortcut_manager
   +   *   Injected shortcut manager.
   +   */
   

   Missing short description.

2. +++ b/core/modules/shortcut/lib/Drupal/shortcut/ShortcutManager.php
   @@ -0,0 +1,265 @@
   +   * @param \Drupal\Core\Extension\ModuleHandler $module_handler
   ...
   +   * @param \Drupal\Core\Routing\RouteProvider $route_provider
   ...
   +   * @param \Drupal\Core\Path\AliasManager $alias_manager
   ...
   +   * @param \Drupal\Core\Entity\EntityManager $entity_manager
   

   Should typehint ModuleHandlerInterface, AliasManagerInterface, RouteProviderInterface and EntityManagerInterface?

3. +++ b/core/modules/shortcut/lib/Drupal/shortcut/ShortcutManager.php
   @@ -0,0 +1,265 @@
   +    $shortcut_sets = &drupal_static('Drupal\shortcut\ShortcutManager::currentDisplayedSet', array());
   

   should use a static class property?

   static::$currentDisplayedSet
   

   and

   static protected $currentDisplayedSet;
   

4. +++ b/core/modules/shortcut/lib/Drupal/shortcut/ShortcutManager.php
   @@ -0,0 +1,265 @@
   +    return (!Url::isExternal($path) && ($this->routeProvider->getRoutesByPattern('/' . $path)->count() > 0 || menu_get_item($path))) || empty($path) || $path == '<front>';
   

   Do we still support menu_get_item()? pretty sure there's an effort to remove that.

5. +++ b/core/modules/shortcut/lib/Drupal/shortcut/ShortcutManager.php
   @@ -0,0 +1,265 @@
   +    foreach ($suggestions as $name) {
   +      if ($shortcut_set = $this->shortcutSetStorageController->load($name)) {
   +        break;
   +      }
   +    }
   

   would this be better off to just use reset() to load the first item from the suggestions?

6. +++ b/core/modules/shortcut/lib/Drupal/shortcut/ShortcutManager.php
   @@ -0,0 +1,265 @@
   +   * This is an OO wrapper for menu_tree since this function has no OO variant.
   

   Short description > 80 chars.

7. +++ b/core/modules/shortcut/lib/Drupal/shortcut/ShortcutManagerInterface.php
   @@ -0,0 +1,129 @@
   + * Contains \Drupal\book\ShortcutManager.
   

   %s/ShortcutManager/ShortcutManagerInterface

8. +++ b/core/modules/shortcut/lib/Drupal/shortcut/ShortcutManager.php
   @@ -0,0 +1,265 @@
   +    $menu_link = $this->entityManager->getStorageController('menu_link')->create($shortcut_link);
   

   Should this be saving menu_link entities now? I thought shortcut entities were decoupled form menu_links? If so, we are missing test coverage.

9. +++ b/core/modules/shortcut/lib/Drupal/shortcut/ShortcutManager.php
   @@ -0,0 +1,265 @@
   +    $shortcuts = $this->entityManager->getStorageController('shortcut')->loadByProperties(array('shortcut_set' => $shortcut_set->id()));
   

   This would seem to confirm my suspicions that the entity type should be shortcut and not menu_link

10. +++ b/core/modules/shortcut/lib/Drupal/shortcut/ShortcutManagerInterface.php
    @@ -0,0 +1,129 @@
    +   * Adds a link to the end of a shortcut set, keeping within a prescribed limit.
    

    >80 chars

11. +++ b/core/modules/shortcut/lib/Drupal/shortcut/ShortcutSetFormController.php
    @@ -64,8 +92,8 @@ public function validate(array $form, array &$form_state) {
    +      $this->setFormError('label', $form_state, t('The shortcut set %name already exists. Choose another name.', array('%name' => $form_state['values']['label'])));
    

    this->t()?

12. +++ b/core/modules/shortcut/lib/Drupal/shortcut/ShortcutSetStorageController.php
    @@ -33,7 +33,7 @@ public function assignUser(ShortcutSetInterface $shortcut_set, $account) {
    +    drupal_static_reset('Drupal\shortcut\ShortcutManager::currentDisplayedSet');
    

    ah, that's why you used drupal_static instead of a static property, although you could make the property public or add a public static reset method, increasing unit-testability. (drupal_static isn't unit testable)

13. +++ b/core/modules/shortcut/shortcut.module
    @@ -128,37 +119,21 @@ function shortcut_set_edit_access(ShortcutSetInterface $shortcut_set = NULL) {
    + *   The machinename of the shortcut set to load.
    

    unrelated change? machinename is not a word so I would say that machine-name or machine name is more correct? But could be a standard I'm not aware of?

So because we don't actually remove the functions here but only deprecate them the only API changes are:

1. Moving
   • ShortcutSetAccessController
   • ShortcutAccessController

   into the Access sub-namespace

2. Changing the constructor of
   • ShortcutSetFormController
   • ShortcutFormController
   • ShortcutsBlock
   • ShortcutSetController
   • ShortcutSetSwitchAccessCheck
   • ShortcutSetEditAccessCheck
   • ShortcutSetAccessController
   • ShortcutAccessController

   to inject the shortcut manager

The first can just be reverted and the second one we generally don't consider disruptive. In this particular case there's not really much of a use-case of subclassing these, so I think these should still be acceptable in beta. Will add a beta evaluation, but I think we're still good to go here.

If we're super adament about BC and don't want to change the constructors we could wrap add calls to \Drupal::service('shortcut.manager') in those classes, but for IMO that would be a bit silly.

Will review this now.

Didn't get all the way through, but here goes:

1. similarity index 69%
   rename from core/modules/shortcut/lib/Drupal/shortcut/ShortcutAccessController.php
   
   rename from core/modules/shortcut/lib/Drupal/shortcut/ShortcutAccessController.php
   rename to core/modules/shortcut/lib/Drupal/shortcut/Access/ShortcutAccessController.php
   
   @@ -2,15 +2,17 @@
   - * Contains \Drupal\shortcut\ShortcutAccessController.
   + * Contains \Drupal\shortcut\Access\ShortcutAccessController.
   ...
   -namespace Drupal\shortcut;
   +namespace Drupal\shortcut\Access;
   
   similarity index 52%
   rename from core/modules/shortcut/lib/Drupal/shortcut/ShortcutSetAccessController.php
   
   rename from core/modules/shortcut/lib/Drupal/shortcut/ShortcutSetAccessController.php
   rename to core/modules/shortcut/lib/Drupal/shortcut/Access/ShortcutSetAccessController.php
   
   @@ -2,19 +2,48 @@
   - * Contains \Drupal\shortcut\ShortcutSetAccessController.
   + * Contains \Drupal\shortcut\Access\ShortcutSetAccessController.
   ...
   -namespace Drupal\shortcut;
   +namespace Drupal\shortcut\Access;
   
   @@ -22,7 +22,7 @@
   - *     "access" = "Drupal\shortcut\ShortcutAccessController",
   + *     "access" = "Drupal\shortcut\Access\ShortcutAccessController",
   
   @@ -20,7 +20,7 @@
   - *     "access" = "Drupal\shortcut\ShortcutSetAccessController",
   + *     "access" = "Drupal\shortcut\Access\ShortcutSetAccessController",
   

   As mentioned above let's revert this.

2. +++ b/core/modules/shortcut/lib/Drupal/shortcut/Access/ShortcutAccessController.php
   @@ -26,6 +28,13 @@ class ShortcutAccessController extends EntityAccessController implements EntityC
   +   * Stores the shortcut Manager.
   
   @@ -34,10 +43,13 @@ class ShortcutAccessController extends EntityAccessController implements EntityC
   +   *   Injected ShortcutManager service.
   

   "The shortcut manager" would be more standard I think.

3. +++ b/core/modules/shortcut/lib/Drupal/shortcut/Access/ShortcutAccessController.php
   @@ -56,7 +69,7 @@ public static function createInstance(ContainerInterface $container, $entity_typ
      protected function checkAccess(EntityInterface $entity, $operation, $langcode, AccountInterface $account) {
        if ($shortcut_set = $this->shortcutSetStorage->load($entity->bundle())) {
   -      return shortcut_set_edit_access($shortcut_set, $account);
   +      return $this->shortcutManager->shortcutSetEditAccess($shortcut_set, $account);
        }
      }
   
   @@ -65,7 +78,7 @@ protected function checkAccess(EntityInterface $entity, $operation, $langcode, A
      protected function checkCreateAccess(AccountInterface $account, array $context, $entity_bundle = NULL) {
        if ($shortcut_set = $this->shortcutSetStorage->load($entity_bundle)) {
   -      return shortcut_set_edit_access($shortcut_set, $account);
   +      return $this->shortcutManager->shortcutSetEditAccess($shortcut_set, $account);
        }
      }
   
   +++ b/core/modules/shortcut/lib/Drupal/shortcut/Access/ShortcutSetEditAccessCheck.php
   @@ -28,17 +46,8 @@ public function appliesTo() {
      public function access(Route $route, Request $request, AccountInterface $account) {
   -    $account = \Drupal::currentUser();
        $shortcut_set = $request->attributes->get('shortcut_set');
   -    // Sufficiently-privileged users can edit their currently displayed shortcut
   -    // set, but not other sets. Shortcut administrators can edit any set.
   -    if ($account->hasPermission('administer shortcuts')) {
   -      return static::ALLOW;
   -    }
   -    if ($account->hasPermission('customize shortcut links')) {
   -      return !isset($shortcut_set) || $shortcut_set == shortcut_current_displayed_set() ? static::ALLOW : static::DENY;
   -    }
   -    return static::DENY;
   +    return $this->shortcutManager->shortcutSetEditAccess($shortcut_set, $account) ? static::ALLOW : static::DENY;
      }
   
   +++ b/core/modules/shortcut/shortcut.module
   @@ -103,21 +101,14 @@ function shortcut_admin_paths() {
   + * @deprecated Use \Drupal\shortcut\ShortcutManager::shortcutSetEditAccess()
   + *
   ...
    function shortcut_set_edit_access(ShortcutSetInterface $shortcut_set = NULL) {
   -  $account = \Drupal::currentUser();
   -  // Sufficiently-privileged users can edit their currently displayed shortcut
   -  // set, but not other sets. Shortcut administrators can edit any set.
   -  if ($account->hasPermission('administer shortcuts')) {
   -    return TRUE;
   -  }
   -  if ($account->hasPermission('customize shortcut links')) {
   -    return !isset($shortcut_set) || $shortcut_set == shortcut_current_displayed_set();
   -  }
   -  return FALSE;
   +  return \Drupal::service('shortcut.manager')->shortcutSetEditAccess($shortcut_set);
    }
   

   This indirection is awkward in my opinion. The ShortcutSetAccessController should be the canonical source for controlling access to shortcut sets. So if the shortcut access depends on the shortcut set edit access, ShortcutAccessController should get ShortcutSetAccessController injected. ShortcutManager has no business in all of this. shortcut_set_edit_access() should then just call \Drupal::entityManager()->getAccessControlHandler('shortcut_set')->edit($shortcut_set, 'edit')

working on this

This is still WIP patch and it's nowhere ready to be reviewed.

Reroll of https://www.drupal.org/node/2083123#comment-8278245 was impossible since the patch is more than year old and core changed significantly in a year :)

It's interesting that PhpStorm reformatting doesn't add new line at the EOF.

I am an idiot - moved shortcut.services.yml to src folder :(

WIP patch

still WIP

WIP

@jibran thanks for the review.

Let's revert this change.

I am not sure about this. please see shortcutSetEditAccess?

Why do we have $account and $user both here?

Fixed

I'm sure we can add some property in ShortcutManger and unset it here.

Can you please be specific?

hopefully this patch will fix some fails.

Correcting non standard tag 'Need tests' to 'Needs tests'

Re-roll.

I'm already working on another version of the patch to fix a few more things.

One of the recurring errors in previous patches was that class dependencies were referred to as services. This is wrong. Services are an application-level concept. Classes don't care if their dependencies are also defined as services somewhere else, or are mocks, or are neither.
TLDR; when writing classes, don't use the terminology service. It's irrelevant and misleading documentation.

This patch fixes the documentation, injects the current user into the shortcut manager, replaces drupal_static() usage in the manager, and makes more use of default method arguments.

Its good to see some progress here but there was a concern if there is a need to rewrite shortcut module in 8.0.x that resulted me to stop working on this issue.

I haven't reviewed the patch but looked at it briefly - I am under the impression that DI can't be added to the constructor as this is consider as API change and we are in beta right now. (I was told not to and thats why more service).

I haven't reviewed the patch but looked at it briefly - I am under the impression that DI can't be added to the constructor as this is consider as API change and we are in beta right now. (I was told not to and thats why more service).

Which constructors are we talking about here? The shortcut manager is new and we can build that whichever way we want. The block is a final class for all intents and purposes, so we can change that too.

+++ b/core/modules/shortcut/src/ShortcutAccessControlHandler.php
@@ -30,16 +30,26 @@ class ShortcutAccessControlHandler extends EntityAccessControlHandler implements
-  public function __construct(EntityTypeInterface $entity_type, ShortcutSetStorageInterface $shortcut_set_storage) {
+  public function __construct(EntityTypeInterface $entity_type, ShortcutSetStorageInterface $shortcut_set_storage, ShortcutManagerInterface $shortcut_manager) {

I was talking about this class but correct me if I am wrong.

Ah yeah. I would say the same logic applies there: the class is more or less final (it's definitely not designed to be extended), so I think adding an extra constructor argument there is fine. If any child classes happen to exist (small chance), it's easier for them to copy the extra constructor argument rather than to set up \Drupal during unit tests.

Ah yeah. I would say the same logic applies there: the class is more or less final (it's definitely not designed to be extended), so I think adding an extra constructor argument there is fine. If any child classes happen to exist (small chance), it's easier for them to copy the extra constructor argument rather than to set up \Drupal during unit tests.

why would you say that class is more or less final - I don't see the word final in the class (sorry, I am trying to understand your point).

unrelated - I don't understand why in some issues, it's ok to add parameters in the constructor. Is it based on the priority of the issue?

A final class cannot be extended. These classes aren't technically final, but were never designed to be extended. Assuming they are indeed never extended, adding constructor arguments does not break anything.

unrelated - I don't understand why in some issues, it's ok to add parameters in the constructor. Is it based on the priority of the issue?

Adding parameters is not a bad thing per se. It will however break child classes outside of core, because we cannot update them in the same patch. Depending on whether there are child classes (how big the disruption is), the BC break may or may not be a problem.

A final class cannot be extended. These classes aren't technically final, but were never designed to be extended. Assuming they are indeed never extended, adding constructor arguments does not break anything.

I know that final class cannot be extended. In this case, ShortcutAccessControlHandler.php, you think that this won't be extended. I guess I need to understand design pattern to convince myself.

This needs a beta evaluation. Shouldn't the old functions be marked deprecated in this issue or in a follow-up issue?

I wouldn't view a constructor as part of an API by default. Only if specifically called out as such.

In addition to the below the current patch is still work in progress, so undoubtedly "needs work".

1. +++ b/core/modules/shortcut/src/ShortcutManager.php
   @@ -0,0 +1,230 @@
   +  /**
   +   * The entity manager.
   +   *
   +   * @var \Drupal\Core\Entity\EntityManager
   +   */
   +  protected $entityManager;
   ...
   +  /**
   +   * The shortcut set storage.
   +   *
   +   * @var \Drupal\shortcut\ShortcutSetStorageInterface
   +   */
   +  protected $shortcutSetStorage;
   

   Either we should inject the shortcut storage (which is what the entity manager is used for) instead of the entity manager itself just like we currently do the shortcut set storage - or we should use the entity manager to fetch the shortcut set storage and not inject the shortcut set storage. (I.e. either inject both shortcut storage and shortcut set storage or neither)

   I would prefer the former but I don't really care.

2. +++ b/core/modules/shortcut/src/ShortcutManager.php
   @@ -0,0 +1,230 @@
   +  /**
   +   * The alias manager.
   +   *
   +   * @var \Drupal\Core\Path\AliasManager
   +   */
   +  protected $aliasManager;
   +
   +  /**
   +   * The route provider.
   +   *
   +   * @var \Drupal\Core\Routing\RouteProvider
   +   */
   +  protected $routeProvider;
   

   It seems both of these are unnecessary dependencies, or am I missing something?

3. +++ b/core/modules/shortcut/src/ShortcutManagerInterface.php
   @@ -0,0 +1,101 @@
   +  public function shortcutSetTitleExists($title);
   

   As shortcut_set_title_exists() has no usages left and only exists for BC purposes we should not introduce a method for this here. Let's just leave the function as is, and then kill it without replacement in D9.

4. +++ b/core/modules/shortcut/src/ShortcutManagerInterface.php
   @@ -0,0 +1,101 @@
   +  public function renderableLinks(ShortcutSetInterface $shortcut_set = NULL);
   

   As this is just a different way to display the links (i.e. it is conceptually similar to ShortcutSet::getShortcuts() IMO we should move this onto ShortcutSetInterface instead.

5. +++ b/core/modules/shortcut/src/ShortcutManagerInterface.php
   @@ -0,0 +1,101 @@
   +  public function shortcutSetEditAccess(ShortcutSetInterface $shortcut_set = NULL, AccountInterface $account = NULL);
   ...
   +  public function shortcutSetSwitchAccess(AccountInterface $account = NULL, AccountInterface $user = NULL);
   

   These two strike me as rather odd. We already have a ShortcutSetAccessControlHandler so I think we should put the logic in there. We should document that a dedicated 'switch' operation is supported and then put everything in ShortcutSetAccessControllerHandler::checkAccess(). Then the current methods can look like

   function shortcut_set_edit_access($shortcut_set = NULL) {
     if (!$shortcut_set) {
       $shortcut_set = shortcut_current_displayed_set();
     }
     return $shortcut_set->access('edit', NULL, TRUE);
   }
   
   function shortcut_set_switch_access($account = NULL) {
     $shortcut_set = shortcut_current_displayed_set();
     return $shortcut_set->access('switch', $account, TRUE);
   }
   

6. +++ b/core/modules/shortcut/src/ShortcutSetStorage.php
   @@ -82,7 +82,7 @@ public function assignUser(ShortcutSetInterface $shortcut_set, $account) {
   -    drupal_static_reset('shortcut_current_displayed_set');
   +    drupal_static_reset('Drupal\shortcut\ShortcutManager::currentDisplayedSet');
   

   The specified variable doesn't actually exist. More importantly, drupal_static_reset() (by design) does not work with class variables. We could add a reset() method to ShortcutManager that does this, but that would still mask the larger problem IMO: We have circular dependency here, between ShortcutManager and ShortcutSetStorage. ShortcutManager calls ShortcutSetStorage to load the assigned shortcut set and ShortcuSetStorage wants to reset the static in ShortcutManager when a new assignment is made. We should fix that properly. I see two possible ways out:

   1. Move ShortcutManager::currentDisplayedSet() into ShortcutSetStorage
   2. Add a assignUser() method to ShortcutManager which proxies to the same method in ShortcutSetStorage and resets its own storage.

   I prefer the second option because that keeps ShortcutSetStorage strictly about storage.

Taking a stab at this

Here we go.

This finishes the clean-up:
- Adds @deprecated to all legacy functions
- Removes usages of them in favor of ShortcutManager et al.

Also addressed #84
I was wrong in #84.5: Switching shortcut set is not actually an operation on shortcut sets themselves, it is an operation on user entities. So I implemented the access logic according to that, so it's now possible to do $user->access('switch_shortcut_set'). #2446195: Move the {shortcut_set_users} table to a proper field on the user entity will clarify this problem-space further, hopefully in D8. It will also deprecate the assignUser() and unassignUser() methods alltogether so if you're not too fond of them now (additionally) living on ShortcutManager see you there! ;-)

In addition I took the liberty of modernizing the new API a bit:
- Renamed functions (currentDisplayedSet() -> getCurrentSet(), defaultSet() -> getDefaultSet()
- Removed the optionality of various $account parameters. It should be upon the caller to decide which account should be checked.

Let's see what I broke :-)

Sorry for the 2 interdiffs, I had to merge in between. It's a bit unfortunate because the second interdiff in some places changes things I did in the 1 one, so it might actually make sense to take a look at the whole patch.

Sorry.

+++ b/core/modules/shortcut/src/ShortcutAccessControlHandler.php
@@ -57,7 +57,7 @@ public static function createInstance(ContainerInterface $container, EntityTypeI
-      return shortcut_set_edit_access($shortcut_set, $account);
+      return $shortcut_set->access('update', $account);

@@ -69,7 +69,7 @@ protected function checkAccess(EntityInterface $entity, $operation, $langcode, A
-      return shortcut_set_edit_access($shortcut_set, $account);
+      return $shortcut_set->access('update', $account);

These should pass TRUE as the third parameter.

Will fix in a minute.

(Sorry for the d.o debugging... :-/)

Next try.

So the very rare condition that ShortcutManager::getDefaultSet() return NULL does actually happen. This was just masked by coincidence (and lack of docs) before. In fixing this I found ShortcutSetStorage::getDefaultSet() which was introduced way back in #1978976: Convert shortcut_set_switch to a Controller. It is a direct copy of shortcut_default_set() now ShortcutManager::getDefaultSet(). In other words, the latter is completely unnecessary. So since the assignUser()/unassignUser() methods are really just proxies, the only "real" method left on ShortcutManager is getCurrentSet(). At that point I think it's overkill to introduce a whole service for a single (not very complicated) method, that only acts as a static cache for ShortcutSetStorage in the first place. Will try that out now.

...and also make ShortcutManager::getDefaultSet() always return a shortcut set.

Here we go. No more ShortcutManager. This patch feels a lot better to me personally, but I won't tackle the title and issue summary update until someone else chimes in on the recent direction, maybe @jibran?

Let's see if this passes.

X-Post with #94. I'm fine with moving that to a follow-up, will fix that in the next patch.

Thanks for the review, #98 should address it, and hopefully fix the tests. I also rolled in the fix for #94.

Will try to tackle the test fails, but this no longer needs to be assigned to me.

+++ b/core/modules/shortcut/src/Plugin/Block/ShortcutsBlock.php
@@ -20,14 +23,61 @@
+  function __construct(array $configuration, $plugin_id, $plugin_definition, EntityManagerInterface $entity_manager, AccountInterface $account) {
+    parent::__construct($configuration, $plugin_id, $plugin_definition);
+    $this->shortcutSetStorage = $entity_manager;
+    $this->account = $account;
+  }

should be $this->shortcutSetStorage = $entity_manager->getStorage('shortcut_set');
With this fix the tests should pass

Added check for the very rare case that Shortcut module has been enabled but its configuration has not yet been installed.

Web services != services

I was really not a fan of adjusting our API for this extreme edge case. In particular because A) The |null type-hint would have to be added to getDefaultSet() as well (and have to be documented there as well) and in theory we would have to adapt all usages of those two methods to account for a NULL return value. I don't think that is a very sane API. Instead in this patch we create a stub shortcut set in the very rare edge case that the default one doesn't exist.

I have some very strange failures locally but I sincerely hope they are specific to my setup, we'll see.

Updated title + issue summary and added beta evaluation. Also marked as bug, because of the shortcut_set_default() issue. It was only triggered as part of the other changes here, but it is a bug nonetheless, just happens to be not caught by our current test coverage.

This should be green.

+++ b/core/modules/shortcut/shortcut.module
@@ -53,56 +55,47 @@ function shortcut_help($route_name, RouteMatchInterface $route_match) {
+ *   Deprecated in 8.x, will be removed in 9.x. Use

@@ -111,6 +104,30 @@ function shortcut_set_switch_access($account = NULL) {
+ *   Deprecated in 8.x, will be removed in 9.x. Use

@@ -160,32 +177,20 @@ function shortcut_set_unassign_user($account) {
+ *   Deprecated in 8.x, will be removed in 9.x. Use

@@ -198,26 +203,20 @@ function shortcut_current_displayed_set($account = NULL) {
+ *   Deprecated in 8.x, will be removed in 9.x. Use

@@ -250,42 +249,19 @@ function shortcut_set_title_exists($title) {
+ *   Deprecated in 8.x, will be removed in 9.x. Use

8.0.x, 9.0.x

Yeah, that's reasonable. Working on tests now.

Here we go. This fixes #111 and adds tests.

Hat tip to @tim.plunkett because after I had seen #2544746: Rewrite EntityManagerTest to use phpspec/prophecy I took this as an opportunity to learn phpspec/prophecy. Spoiler alert: It's pretty awesome!

As is often the case, I had to fix up a couple things to be able to write proper unit tests, most notably injecting the database connection in ShortcutSetStorage. I also noticed that we were missing a resetCache() override to reset the userShortcutSets member variable, that was a bug in the previous patches that is now fixed and tested.

1. +++ b/core/modules/shortcut/shortcut.module
   @@ -53,56 +55,47 @@ function shortcut_help($route_name, RouteMatchInterface $route_match) {
   + *
   + * @deprecated
   + *   Deprecated in 8.0.x, will be removed in 9.0.x. Use
   + *   $shortcut_set->access('update') directly instead.
     */
    function shortcut_set_edit_access(ShortcutSetInterface $shortcut_set = NULL) {
   -  $account = \Drupal::currentUser();
   -
   -  // Shortcut administrators can edit any set.
   -  if ($account->hasPermission('administer shortcuts')) {
   -    return AccessResult::allowed()->cachePerPermissions();
   +  if (!isset($shortcut_set)) {
   +    /** @var \Drupal\shortcut\ShortcutSetStorageInterface $shortcut_set_storage */
   +    $shortcut_set_storage = \Drupal::entityManager()->getStorage('shortcut_set');
   +    $shortcut_set = $shortcut_set_storage->getCurrentSet(\Drupal::currentUser());
      }
   -
   -  // Sufficiently-privileged users can edit their currently displayed shortcut
   -  // set, but not other sets. They must also be able to access shortcuts.
   -  $may_edit_current_shortcut_set = $account->hasPermission('customize shortcut links') && (!isset($shortcut_set) || $shortcut_set == shortcut_current_displayed_set()) && $account->hasPermission('access shortcuts');
   -  return AccessResult::allowedIf($may_edit_current_shortcut_set)->cachePerPermissions();
   +  return $shortcut_set->access('update', NULL, TRUE);
    }
   

   YAY!

2. +++ b/core/modules/shortcut/shortcut.module
   @@ -53,56 +55,47 @@ function shortcut_help($route_name, RouteMatchInterface $route_match) {
   -function shortcut_set_switch_access($account = NULL) {
   
   @@ -111,6 +104,30 @@ function shortcut_set_switch_access($account = NULL) {
   + * @deprecated
   + *   Deprecated in 8.0.x, will be removed in 9.0.x. Use
   + *   $user->access('switch_shortcut_set') with a user entity instead.
   + */
   +function shortcut_set_switch_access(AccountInterface $account = NULL) {
   +  if (!$account) {
   +    $account = \Drupal::currentUser();
   +  }
   +  /** @var \Drupal\user\UserInterface $user */
   +  $user = User::load($account->id());
   +  return $user->access('switch_shortcut_set', NULL, TRUE);
   +}
   

   YAY!

These are the main concerns I opened #2339903: Shortcut module's access checking is insane for, so marked that as a duplicate now :)

I realized that I hadn't introduced tests for the newly introduced shortcut_user_access(), so I wrote that as a shiny new KernelTestBaseTNG (á la #2304461: KernelTestBaseTNG™) but then I hit #2553661: KernelTestBase fails to set up FileCache and specifically saw #2553661-12: KernelTestBase fails to set up FileCache by @alexpott:

Before we add any more KTBTNG's we need to fix https://www.drupal.org/node/2553533 - each new test added increases the chance of random fail due to test id reuse.

So not adding the new test to the patch just yet, but just providing it here as a separate patch for reference. If #2553661: KernelTestBase fails to set up FileCache lands before this I will add it to the patch (note that the overridden setUp() method is no longer necessary then, hat tip to @klausi for that by the way for that, see #2304461-252: KernelTestBaseTNG™), otherwise I will open a follow-up for the added test coverage.

This really should be ready now. Does someone feel bold enough to pull the trigger and RTBC?

#117.1: I disagree.
When testing the following code, asserting that b() will be called is sensible, but I disagree that asserting that a() is called is sensible.

if (a()) {
  b();
}

The code in question *looks* quite a lot different, but in terms of actual code flow it's the same pattern.

#117.2: This is just a unit test with a mocked user ID, so 1 has no special meaning.

#118.1 Since we were already using \Drupal::currentUser() above moved that to the top and used the result in both places.

#118.2 Well, we already have an $account variable in scope, so $user would be pretty much the only sensible choice here. Collapsing it is fine, though, so I did that.

#118.3 Fixed

#118.4 Well load() is documented as sometimes returning NULL, so...

#118.5 No += should only ever be used with keyed (= named) arrays.

$array = ['foo'];
$array += ['bar'];

yields an array with just 'foo', 'bar' is not added because both entries have the same key (0). But in this case we always want to add 'default'.

#118.6 Fixed

#118.7 In theory agree, but that would be an unnecessary API break, so I would like confirmation by a maintainer before proceeding with that change.

This should fix it.

Re #123: The patch in #119 already included the test.

Interdiff was correct, but I rolled the wrong patch.

#fail

Added a draft change notice.

+++ b/core/modules/shortcut/src/ShortcutSetAccessControlHandler.php
@@ -10,14 +10,49 @@
+use Drupal\node\Plugin\views\filter\Access;

unintended changed?

Can be fixed on commit.

Great work @tstoeckler and @jibran

Lol, yeah, sorry about that. In case of re-roll will remove but leaving at RTBC for now. Thanks for the review!

Shortcut cleanup:

• Fix bug during config import
• remove duplicated code
• deprecate legacy functions

Can this be split out into three issues please - makes it easier to review and way less risky.