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 |
---|---|---|---|
#39 | shortcut-1978956-39.patch | 10.01 KB | tim.plunkett |
#33 | 1978956-shortcut_set_customize-33.patch | 10.69 KB | naxoc |
#33 | interdiff.txt | 1.23 KB | naxoc |
#31 | 1978956-shortcut_set_customize-31.patch | 10.67 KB | naxoc |
#31 | interdiff.txt | 9.5 KB | naxoc |
Comments
Comment #1
michaellenahan CreditAttribution: michaellenahan commentedWorking on this as part of the London Drupal Sprint weekend.
Comment #2
alexpottTagging...
Comment #3
michaellenahan CreditAttribution: michaellenahan commentedHere's the patch. I'm new to this, so many thanks Alex for your help over the weekend!
The title is static, so the form will show 'Edit shortcuts' as the title rather the correct title of the shortcut set which comes from the title callback. Is this the same issue that is listed here http://drupal.org/node/1981644 ?
Comment #4
michaellenahan CreditAttribution: michaellenahan commentedComment #5
michaellenahan CreditAttribution: michaellenahan commentedreinstating tags
Comment #6
michaellenahan CreditAttribution: michaellenahan commentedRemoved a rogue print_r() statement from the patch.
Comment #7
alexpottNeed to inject the plugin.manager.entity service from the container to use in the submit method.
Need to move functionality from shortcut_set_customize_submit() into this method and instead of using menu_link_load/save use the methods on the entity and from the manager.
Unnecessary spaces...
We need to keep the title callback and arguments at the moment... unfortunately we then have issues with argument loading - see #1981644: Figure out how to deal with 'title/title callback' and #1974408: Convert aggregator_page_source() to a Controller
Also we can remove the shortcut_set_customize() and shortcut_set_customize_submit() functions from shotcut.admin.inc
Comment #8
dawehnerChange status.
Comment #9
michaellenahan CreditAttribution: michaellenahan commentedHi there, I'm stuck! :)
I'm not sure how to do this, or if I'm really honest I'm not sure what "inject the plugin.manager.entity service from the container" really means, I'm still quite fresh to D8. Any examples out there I can follow?
If you can give me a pointer for how to do this I'd be grateful.
Thank you!
Comment #10
dawehnerHi!
There is a great blogpost about dependency injection: http://fabien.potencier.org/article/11/what-is-dependency-injection
To sum it up, an object should not rely on global functions, but the dependencies should be passed in as objects in the constructor,
so you can swap it out and test it easy.
The previous code looked like that:
It still relies on the global menu_link_load() method but you can replace it by using something like Drupal::entityManager()->getStorageController('menu_link')->load(array($id)); Instead of using Drupal::entityManager() you can also use an entity manager object,
which get injected into the constructor of the object. Therefore you can use the create() method to get the entity manager from the container.
The ShortcutController does something similar.
Comment #11
michaellenahan CreditAttribution: michaellenahan commentedHello, here's another attempt at a patch.
I'm now adding the EntityManager in the constructor (lines 41-43 in the patch)
However, I'm still missing something because when I submit the form by hitting "Save changes" button on:
admin/config/user-interface/shortcut/manage/default
... I get this error.
Fatal error: Call to a member function getStorageController() on a non-object in /workspace/drupal-8/core/modules/shortcut/lib/Drupal/shortcut/Form/SetCustomize.php on line 116
In my submitForm function (line 119 in the patch) I am trying to do this.
$this->entityManager is not set to anything though.
Any ideas what I'm missing? Thank you!
Comment #12
alexpottHere's why... should be
$this->entityManager = $entity_manager;
Comment #13
michaellenahan CreditAttribution: michaellenahan commentedThanks. Patch attached.
Also needed to adjust the $link variable in submitForm() with an array_shift.
Comment #14
michaellenahan CreditAttribution: michaellenahan commentedWhile testing this, I noticed that changing the weight of the links had no effect.
The changed weights are being saved correctly to the database, the problem is with the $shortcut variable which contains the shortcut set at buildForm time.
This $shortcut variable contains the shortcut set to be rendered on-screen. The problem is that the links within the shortcut set are not in ascending weight order.
I noticed this same issue occurs in the pre-conversion version of the same form.
admin/config/user-interface/shortcut/manage/default
How to fix it? Should the $shortcut variable come with links ordered in the correct order? Or should we adjust the order in the buildForm function?
Comment #15
michaellenahan CreditAttribution: michaellenahan commentedchanged to needs review.
Comment #17
michaellenahan CreditAttribution: michaellenahan commentedAttached the wrong patch last time. Here's the correct one (I hope).
Comment #18
dawehnerI realized that the shortcut shorting is broken in general. This is not caused by your conversion, so this might be better solved in a follow up.
One way to solve this would be to order the $entity->links in ShortcutStorageController::preSave() by weight.
While debugging this issue I came up with the idea to not store the full entity manager.
Comment #19
michaellenahan CreditAttribution: michaellenahan commentedComment #20
kim.pepper#18: drupal-1978956-17.patch queued for re-testing.
Comment #22
kim.pepperComment #23
vijaycs85Re-rolling...
Comment #24
tim.plunkettThis logic is already encapsulated in case 'edit'.
I don't think this patch should add this.
Comment #25
dawehnerComment #26
star-szrTags
Comment #27
michaellenahan CreditAttribution: michaellenahan commentedHello, I'm looking at rerolling this.
After applying the patch I get this Notice when I go to the following page:
admin/config/user-interface/shortcut/manage/default
Notice: Undefined index: #shortcut_set_name in theme_shortcut_set_customize() (line 213 of core/modules/shortcut/shortcut.admin.inc).
line 213 of core/modules/shortcut/shortcut.admin.inc -
it's true, $form['#shortcut_set_name'] doesn't exist.
the variable I'm looking for should read 'default' --- that's the shortcut set name --- but I can't see any variable with this string as content when I look at the $form array.
Comment #28
naxoc CreditAttribution: naxoc commentedComment #29
naxoc CreditAttribution: naxoc commentedHere is a reroll of the patch in #23.
I changed a couple of things as pr. Tim's comments in #24.
Comment #30
tim.plunkettAfter looking at this again, I think we could add a standard EntityFormController here. I can help with that if need be.
I don't think we need to expand this to include 'customize', we can just use 'edit'.
Comment #31
naxoc CreditAttribution: naxoc commentedI took a stab at making the controller an EntityFormController. I ended up implementing a lot of the same things that are going on in #1938924: Convert shortcut theme tables to table #type which meant that I could kill even more code in shortcut.admin.inc. Not sure if that is a little too much going on though.
Comment #32
dawehnerIt would be cool to have a screenshot of before and after, as there might be changed.
Should be "{@inheritdoc}"
We should not remove the title callback here, as it is still needed for menu links.
Comment #33
naxoc CreditAttribution: naxoc commentedThere are no (visible) changes to the UI, so screenshots would be identical.
I put the title callback back in hook_menu and corrected the comment.
Comment #34
tim.plunkettLooks good to me, thanks @naxoc!
Comment #35
dawehnerEven prooving that nothing changes is a win.
Comment #36
tim.plunkettThis blocks removal of MENU_LOCAL_ACTION.
Comment #37
alexpottNeeds a reroll
Comment #38
alexpottWe need to inject the url generator here...
Comment #39
tim.plunkettThat calls l(), not url(), that's not injectable yet.
Straight reroll.
Comment #40
ParisLiakos CreditAttribution: ParisLiakos commentedComment #41
alexpottCommitted 8469e98 and pushed to 8.x. Thanks!