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 |
---|---|---|---|
#42 | 1987638-convert-block-admin-display-42.patch | 9.37 KB | vijaycs85 |
#39 | 1987638-convert-block-admin-display-37.patch | 9.39 KB | vijaycs85 |
#39 | 1987638-diff-35-37.txt | 2.55 KB | vijaycs85 |
#35 | 1987638-convert-block-admin-display-35.patch | 9.38 KB | vijaycs85 |
#35 | 1987638-diff-32-35.txt | 1.42 KB | vijaycs85 |
Comments
Comment #1
tim.plunkettWill work on after #1983844: Add EntityListController and convert picture.module callbacks to routes/controllers to provide a use-case
Comment #2
manu4543 CreditAttribution: manu4543 commentedIntial patch..
Comment #3
manu4543 CreditAttribution: manu4543 commentedInitial patch..
Comment #5
manu4543 CreditAttribution: manu4543 commentedComment #6
manu4543 CreditAttribution: manu4543 commentedComment #8
tim.plunkettSo the problem is this line. It passes a variable to the page callback, which the EntityListController cannot handle.
This will need something more custom.
Comment #9
dawehnerIt would be at least possible to extend the default EntityListController so there is just a single override needed for this function.
Comment #10
vijaycs85Will it help?
Comment #12
tim.plunkettYou'll also need to convert the foreach loop at the bottom into a RouteSubscriber. It currently is trying to use the same page callback.
Comment #13
vijaycs85Thanks for the quick reply @tim.plunkett. I have updated to routeSubscriber. I can see a TODO for D8 in foreach,
but not sure what that means. Hopefully this patch makes the patch green at least.
Comment #14
tim.plunkettYou can have the plugin UI manager injected via the services.yml
use
statements don't need leading \Otherwise, this looks great!
Comment #15
vijaycs85Updating patch to reflect comments in #14
Comment #16
tim.plunkettInstead of getting the container, you can just get that single service injected. Sorry for the confusion
Comment #17
vijaycs85sorry, my mistake. injected just plugin manager.
Comment #19
vijaycs85#17: 1987638-convert-block-admin-display-17.patch queued for re-testing.
Comment #21
vijaycs85#17: 1987638-convert-block-admin-display-17.patch queued for re-testing.
Comment #22
manu4543 CreditAttribution: manu4543 commentedIs this
same as
because both works or more specifically does it have anything to do with failed testing?
Comment #24
vijaycs85#17: 1987638-convert-block-admin-display-17.patch queued for re-testing.
Comment #26
tim.plunkettBlockStorageUnitTest needs to add 'system' to the public static $modules array.
Comment #27
vijaycs85Thanks @tim.plunkett. That solves the issue (Can confirm here).
Comment #28
dawehnerThis should use _content instead of _controller.
Please inject the config settings into the controller.
Comment #29
vijaycs85thanks @dawehner. Here is the fix for #28.
Comment #30
tim.plunkettThis looks perfect, commit if green!
Thanks @vijaycs85.
Comment #31
alexpottAFAICS this should remove the
block_admin_display()
function as once the patch has been applied it is not used.Comment #32
vijaycs85alexpott++. just missed the basic purpose of moving stuffs :) Did a code look up for any other reference to block_admin_display() and seems we are good to remove it.
Comment #34
dawehnerSo why should this be a string?
Has some missing docs.
Comment #35
vijaycs85Will it help?
Comment #36
xtfer CreditAttribution: xtfer commentedJust a few code standards issues... nothing major.
Last item in an array should have a comma.
This should be $camelCase.
Needs a scope modifier (i.e. public).
Space after the comma required.
Space before comma should be removed.
Comment #37
xtfer CreditAttribution: xtfer commentedWith those changes, this would be RTBC, in my opinion.
Comment #38
xtfer CreditAttribution: xtfer commentedCurrently blocking #2014191: Implement the block-by-theme listing page.
Comment #39
vijaycs85Thanks for your review @xtfer. Here is the patch that fixes all review comments in #36.
Comment #40
xtfer CreditAttribution: xtfer commentedI've reviewed and tested this as part of #2014191: Implement the block-by-theme listing page, and I can't see any further issues with it. Looking good.
Comment #41
alexpottthere is a new _entity_list key :) added in #2008356: Provide a _entity_list route default to replace Controller\EntityListController and mimic _entity_form
Comment #42
vijaycs85Just re-roll as we can't use _entity_list in blockListContoller.
Comment #43
tim.plunkettCorrect, since the block list needs $theme passed along, it cannot use _entity_list.
Comment #44
alexpottCommitted 40471c8 and pushed to 8.x. Thanks!