Problem/Motivation

Shortcut module is using shortcut_set_assign_user and shortcut_set_unassign_user global functions.

As per tstoeckler for removing global functions -

it is a good idea. it's the new world order

Proposed resolution

Remove global functions
Fix the test coverage

Remaining tasks

User interface changes

None

API changes

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue priority Not critical because Drupal is fully functional with these two global functions
Unfrozen changes Not unfrozen
Prioritized changes This is not a prioritized change, however we are only deprecating functions for drupal 9.x and removing the usages.
Disruption No disruption.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

kgoel’s picture

Status: Active » Needs review
FileSize
4.07 KB

Removed some unused classes as well...

kgoel’s picture

FileSize
4.46 KB
1.98 KB

The last submitted patch, 1: 2427323-1.patch, failed testing.

tstoeckler’s picture

Status: Needs review » Needs work
-    shortcut_set_assign_user($this->set, $this->adminUser);
+    \Drupal::entityManager()->getStorage('shortcut_set')->assignUser($this->set, $this->shortcutUser);

Should be $this->adminUser instead of $this->shortcutUser.

kgoel’s picture

Status: Needs work » Needs review
FileSize
4.45 KB
685 bytes
kgoel’s picture

Issue summary: View changes
kgoel’s picture

Issue summary: View changes
tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community

Awesome, thanks!

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Time has passed for this type of change. We can deprecate for drupal 9.x and remove usages but there is no need to break the API.

kgoel’s picture

Status: Needs work » Needs review
FileSize
4.06 KB
1.68 KB
dawehner’s picture

Let's nitpick a bit :)

+++ b/core/modules/shortcut/shortcut.module
@@ -110,6 +110,45 @@ function shortcut_set_switch_access($account = NULL) {
+ *
+ * @deprecated in Drupal 8.x, will be removed before Drupal 9.0.
+ *   Use \Drupal::entityManager()->getStorage('shortcut_set')->assignUser().
...
+ *
+ * @deprecated in Drupal 8.x, will be removed before Drupal 9.0.
+ *   Use \Drupal::entityManager()->getStorage('shortcut_set')->unassignUser().
+ *

Afaik we place those @deprecated tags onto the bottom of the doc block.

kgoel’s picture

In that case, this must be wrong in core...

/**
 * Return the taxonomy vocabulary entity matching a vocabulary ID.
 *
 * @deprecated in Drupal 8.x, will be removed before Drupal 9.0.
 *   Use \Drupal\taxonomy\Entity\Vocabulary::load().
 *
 * @param int $vid
 *   The vocabulary's ID.
 *
 * @return \Drupal\taxonomy\Entity\Vocabulary|null
 *   The taxonomy vocabulary entity, if exists, NULL otherwise. Results are
 *   statically cached.
 */
function taxonomy_vocabulary_load($vid) {
  return Vocabulary::load($vid);
}
kgoel’s picture

FileSize
4.19 KB
1.78 KB
tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me, thanks for sticking with this!

kgoel’s picture

Assigned: kgoel » Unassigned
alexpott’s picture

Title: Remove shortcut_set_assign_user and shortcut_set_unassign_user from shortcut » Deprecate shortcut_set_assign_user and shortcut_set_unassign_user
Issue summary: View changes
Status: Reviewed & tested by the community » Fixed

Committed 4211782 and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.

  • alexpott committed 4211782 on 8.0.x
    Issue #2427323 by kgoel: Deprecate shortcut_set_assign_user and...

Status: Fixed » Closed (fixed)

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