Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#49 | Shortcuts_v.d8_20130906-153816.png | 14.62 KB | jibran |
#49 | Shortcuts_v.d8_20130906-154318.png | 16.42 KB | jibran |
#44 | shortcut-1978952-44.patch | 3.54 KB | tim.plunkett |
#44 | interdiff.txt | 1.23 KB | tim.plunkett |
#42 | shortcut-1978952-42.patch | 3.65 KB | tim.plunkett |
Comments
Comment #1
vijaycs85Comment #2
vyasamit2007 CreditAttribution: vyasamit2007 commentedPFA. The patch file with the changes as per the link provided.
Comment #4
vijaycs85#2: 1978952-Convert shortcut_set_add-to Controller-2.patch queued for re-testing.
Comment #6
vyasamit2007 CreditAttribution: vyasamit2007 commentedRenamed the patch file and queued for re-testing.
Comment #7
dawehnerDon't use tabs here but spaces :)
This patch should probably wait until #1946454: Convert all confirm_form() in system.module and system.admin.inc to the new form interface and convert route is in, so you can inject the entity manager in there.
Comment #8
tim.plunkettBlocks #1908756: Separate Action Links (MENU_LOCAL_ACTION) from hook_menu()
Comment #9
kim.pepper#6: 1978952-Convert_shortcut_set_add_to_Controller-6.patch queued for re-testing.
Comment #11
kim.pepperComment #12
vijaycs85Re-rolling...
Comment #13
dawehnerAdding shortcuts still works fine. Code looks perfect.
Comment #14
jibranSorry
hook_local_actions()
is missing see Action links are provided by hook_local_actions() instead of MENU_LOCAL_ACTION in hook_menu() for further details.Comment #15
vijaycs85Updating changes mentioned in #14.
Comment #16
ParisLiakos CreditAttribution: ParisLiakos commentedComment #17
tim.plunkettRerolled
Comment #18
ParisLiakos CreditAttribution: ParisLiakos commentedawesome, thanks
Comment #19
jibranEverything in #14 is fixed.
Comment #20
YesCT CreditAttribution: YesCT commentedThis issue was RTBC and passing tests on July 1, the beginning of API freeze.
Comment #21
alexpottNeeds a reroll
Comment #22
vijaycs85Re-rolling...
Comment #24
tim.plunkett#22: 1978952-convert_shortcut_set_add-controller-22.patch queued for re-testing.
Comment #25
dawehnerThis has been ready to fly before.
Comment #26
alexpottCommitted 522c787 and pushed to 8.x. Thanks!
Comment #27
tim.plunkettThis doesn't actually work, we just had no test coverage.
The test right now is only covering itself, not the UI...
Comment #28
tim.plunkettRerolled.
Comment #30
vijaycs85This patch still valid and getting error because of 'Add shortcut set' missing in admin/config/user-interface/shortcut. Seems LOCAL_TASK implementation broken between #27 and #28
Comment #31
Berdir#28: shortcut-1978952-28.patch queued for re-testing.
Comment #33
disasm CreditAttribution: disasm commentedreroll!
Comment #34
disasm CreditAttribution: disasm commentedComment #36
disasm CreditAttribution: disasm commentedMissed an entity manager change in the test.
Comment #38
tim.plunkettShortcutSetAccessController was really out of date.
Comment #39
tim.plunkettComment #41
jibranNeeds reroll.
Comment #42
tim.plunkettReroll, #2062021: Replace user_access() calls with $account->hasPermission() in shortcut module decided to blindly update the access checker without manually testing anything
Comment #43
BerdirThis is not necessary, $account is always provided, prepareUser() is called in access().
This should implement checkCreateAccess() instead (just like the other one is checkAccess() and not access()), then you don't need the prepare stuff but always get an account object.
Comment #44
tim.plunkettDuh, thanks @Berdir. I should have known better :)
Comment #45
jibranahh can we add
ShortcutManager
? and moveshortcut_current_displayed_set
toShortcutManager
.Comment #46
tim.plunkettNot in this issue, no. This is a regression, we need to fix it ASAP.
I will be glad to join you in a follow-up!
Comment #47
jibranOk then RTBC for a green green patch with tests.
Comment #48
jibranCreated on public demand #2083123: Shortcut cleanup: Remove duplicated code and deprecate legacy functions.
Comment #49
jibranHere is before and after screenshots @alexpott suggestion.
Before
After
Comment #50
alexpottCommitted b84bec5 and pushed to 8.x. Thanks!