Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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 |
---|---|---|---|
#26 | 1987810-25-follow-up.patch | 592 bytes | tstoeckler |
#26 | hide-descriptions.png | 42.58 KB | tstoeckler |
#22 | system-1987810-22.patch | 9.32 KB | tim.plunkett |
#22 | interdiff.txt | 746 bytes | tim.plunkett |
#20 | system-1987810-20.patch | 9.22 KB | tim.plunkett |
Comments
Comment #1
vijaycs85blocked by #1987830: Convert system_status() to a new style controller. However adding initial patch to start with after #1987830: Convert system_status() to a new style controller is in.
Comment #2
nick_schuch CreditAttribution: nick_schuch commentedComment #3
nick_schuch CreditAttribution: nick_schuch commentedI have rerolled this against the latest head now that system_status() is in. Ready for a review.
Comment #4
nick_schuch CreditAttribution: nick_schuch commentedAh dam, only half the patch. Here we go.
Comment #6
nick_schuch CreditAttribution: nick_schuch commentedErrors were these:
"exception 'Drupal\Core\Config\StorageException' with message 'Failed to write configuration file: sites/default/files/simpletest/98944/config_active/manifest.block.block.yml' in /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Config/FileStorage.php:102
Stack trace:"
Going to requeue.
Comment #7
nick_schuch CreditAttribution: nick_schuch commentedComment #8
nick_schuch CreditAttribution: nick_schuch commented#4: 1987810-route-system-config-4.patch queued for re-testing.
Comment #9
dawehnerThe documentation standard suggests to use "Contains \..."
Just drop the bit about the menu callback and maybe provide an @return
You can inject the menu link entity storage controller and load the entities from there.
Please inject the entity query into the controller.
What about converting it to a render array here, I guess this should be possible.
this should be _content here.
Comment #10
nick_schuch CreditAttribution: nick_schuch commentedOk, here is a patch that I believe addresses the items in #9. I also updated all the _controller references to _content.
Comment #11
dawehnerIt would be cool to have some comment what the hell is happening here.
We could just switch to a render array here.
Yes right these ones should be replacing _controller by _content, but ideally this should have been a new issue. It just saves kittens.
Comment #12
alexpottOkay based on #11 needs work...
Comment #13
dawehnerLet's just do that.
Comment #15
dawehnerurgs.
Comment #17
dawehnerSO using render arrays would need changes in more then one place.
Comment #18
vijaycs85Re-roll...
Comment #20
tim.plunkettRerolled, I took a slightly different approach.
Comment #21
jibranNothing big.
@todo here.
This is awesome. I completely missed it.
It is in the theme function I think it is fine.
PS: https://twitter.com/JibranIjaz/status/366753152608903169
Comment #22
tim.plunkettAdded the todo. This now blocks #1987814: Convert system_admin_menu_block_page() to a new style controller, let's get it in.
Comment #23
jibranThanks for the change. Code looks great let's get it in.
Comment #24
tim.plunkettAll of these are criticals, but I'm actually marking this one since it blocks another.
Comment #25
webchickMan, we so desperately need to rename that interface. :P
Committed and pushed to 8.x. Thanks!
Comment #26
tstoecklerThe route is in fact called
<front>
. This is currently broken in 8.x, which you can verify by clicking the "Hide descriptions" link on admin/config. You will get a nice helpful error that the 'front' route does not exist. (See also attached screenshot.) That's why I'm re-opening this instead of opening a follow-up. Setting to needs review so the attached patch gets a date with the bot, but the fact that 8.x is currently passing tests is that we're missing test coverage for this.Edit: Fixed HTML for < front >
Comment #27
tstoecklerNow back to needs work for tests.
Also this is no longer critical. It's sort of major though due to the current state in 8.x.
Comment #28
pfrenssenI'll write a test.
Comment #29
tim.plunkett#2076551: SystemController::compactPage uses an invalid route name for the front page was opened for this, I thought pwolanin had linked it here.