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.
For instructions on how to convert a page callback into a controller, see the WSCCI Conversion Guide.
Part of #1971384: [META] Convert page callbacks to controllers
Related Issues
#1974394: Convert aggregator_page_category() to a Controller
#2043581: Move aggregator_load_feed_items to the ItemStorageController, retain procedural wrapper
Blocked by
#2040199: Convert aggregator_page_category_form and aggregator_page_source_form to a Controller
Comment | File | Size | Author |
---|---|---|---|
#90 | 1974408-aggregator-source-controller-90.patch | 23.85 KB | kim.pepper |
#90 | interdiff.txt | 1.73 KB | kim.pepper |
#87 | 1974408-aggregator-source-controller-87.patch | 23.84 KB | kim.pepper |
#87 | interdiff.txt | 3.99 KB | kim.pepper |
#81 | drupal8.aggregator-module.1974408-80.patch | 24.82 KB | disasm |
Comments
Comment #1
kshama_deshmukh CreditAttribution: kshama_deshmukh commentedComment #2
kshama_deshmukh CreditAttribution: kshama_deshmukh commentedComment #3
kshama_deshmukh CreditAttribution: kshama_deshmukh commentedComment #4
kshama_deshmukh CreditAttribution: kshama_deshmukh commentedaggregator_page_source() is also used by aggregator_page_source_form(). Hence, needs to remove aggregator_page_source() from aggregator.pages.inc while conversion of aggregator_page_source_form() to WSCCI.
Comment #5
alexpottTagging
Comment #6
alexpottOnce we converted the hook_menu to use the route %aggregator_feed was no longer autoloaded by aggregator_feed_load. So kshama has introduced a new title callback to handle this. This (to me) appears to be a regression. Adding the Stalking Crell tag to get an opinion :)
Comment #7
dawehnerOpened an issue for that #1981644: Figure out how to deal with 'title/title callback'
Comment #9
alexpottFixed up patch in #2 to investigate fixes for #1981644 ... none found so far
Comment #11
kshama_deshmukh CreditAttribution: kshama_deshmukh commentedFixed failed patch. Also moved used functions from aggregator.pages.inc to controller and removed usage of module handler.
Comment #12
ParisLiakos CreditAttribution: ParisLiakos commentedhere is i think a better way to deal with page_callbacks for now? #1978166: Convert block/add/{%custom_block_type} to Controller
no need for _ prefix, it is already marked protected
Comment #13
ParisLiakos CreditAttribution: ParisLiakos commenteddrupal_set_title solution is the way to go for now. see the issue linked above for example
also
lets use the procedural function here, this method is definitely not a method for a controller
Comment #14
kim.pepper#11: aggregator-page-source-controller-1974408-11.patch queued for re-testing.
Comment #16
kim.pepperComment #17
theMusician CreditAttribution: theMusician commentedWorking on re-rolling this right now.
Comment #18
theMusician CreditAttribution: theMusician commentedHere is a re-roll of the last patch. I needed to include the _ prefix mentioned in comment #12 or the aggregator module whitescreens. The PHP log reports:
PHP Fatal error: Call to undefined method Drupal\aggregator\Routing\AggregatorController::buildPageList() in /Applications/MAMP/htdocs/drupal8/core/modules/aggregator/lib/Drupal/aggregator/Routing/AggregatorController.php on line 117
Keeping the _ prefix prevents the issue.
Comment #19
theMusician CreditAttribution: theMusician commentedUgh...forgot to set for review.
Comment #20
dawehnerCan you explain why entity_page_label does not work here?
No need to write _ in front of it.
Couldn't you use entity query for that instead of manually querying the database.
Comment #21
ParisLiakos CreditAttribution: ParisLiakos commentedlets just do, what we did with the pageLast() method here. most of those functions in aggregator.pages.inc will look very different or die once #15266: Replace aggregator category system with taxonomy is in..so just add a @todo above them
Comment #22
Gaelan CreditAttribution: Gaelan commentedComment #23
star-szrNeeds another reroll, AggregatorController got moved.
Comment #24
pwieck CreditAttribution: pwieck commentedReroll to current head.
Only changed:
+ return $this->_buildPageList($items, $feed_source);
TO
+ return $this->buildPageList($items, $feed_source);
Refer to #20 and #21 for further needed changes
Comment #26
kim.pepper@pwieck you forgot to change the actual method name in #24.
Comment #28
dawehnerIt doesn't seem helpful to have this method to be public as this is just the controller. On the longrun there should be a separate service for it?
Comment #29
kim.pepperThanks for the review @dawehner.
I've fixed the broken tests in #26 and moved
loadFeedItems()
to theItemStorageController
interface and class.Comment #30
dawehnerSo as we don't have the load callbacks anymore the $fid is not upcasted anymore? Maybe just remove the placeholder in the uri of the hook_menu entry
This documentation is missed up a bit.
Let's use the FeedInterface instead.
ItemInterface
Should be just a render array as well.
should/could we not just make this a entity query?
Needs types.
Comment #32
kim.pepper#29: 1974408-aggregator-source-controller-29.patch queued for re-testing.
Comment #33
kim.pepperFixed the documentation issues, but have a few more comments.
@dawehner
Sorry, I'm not entirely clear on what you mean here.
Discussed this with @dawehner in IRC and agreed to leave it in ItemStorageController as is for now.
Not sure what needs to be done to convert that.
Comment #34
kim.pepperTagging
Comment #35
ParisLiakos CreditAttribution: ParisLiakos commentedabout the pager thing:
it should be
instead of
Comment #36
ParisLiakos CreditAttribution: ParisLiakos commentedsigh
Comment #37
dawehnerThis change might be needed because of #2023795: REGRESSION: hook_local_actions doesn't use title callback
FeedInterface could have the full namespace, ... mentioning as you already have to do something.
Comment #38
kim.pepperFixed #35 and doc change in #37.
I'm still not sure if I can do anything about the title callback just yet? Is this a follow up?
Kim
Comment #40
kim.pepperLooks like I messed up the render array stuff.
Comment #41
kim.pepperIs this right?
Comment #42
kim.pepperDouble-post
Comment #43
kim.pepperI think I have the changes @dawehner pointed out in #30 including the title callback and replacing theme().
Comment #44
ParisLiakos CreditAttribution: ParisLiakos commentedso if the pager does not work in the #type => 'container', maybe add one more level to it?
eg
havent tested, but i dont like the drupal_render there tbh
Comment #45
kim.pepperRe-roll, and removed drupal_render as per #44.
Comment #46
ParisLiakos CreditAttribution: ParisLiakos commentedi moved the whole thing to drupal_set_title() as originally proposed in #13
Also, since we are adding the buildPageList and the method in Item storage controller, lets cleanup the controller:)
Comment #47
kim.pepperLooks good to me!
Comment #48
YesCT CreditAttribution: YesCT commentedThis issue was RTBC and passing tests on July 1, the beginning of API freeze.
Comment #49
ParisLiakos CreditAttribution: ParisLiakos commentedlets fix the use statements too
Comment #51
tim.plunkett'title callback' was broken for routes for a while but is fixed again, this can be reverted
Also, is this still needed?
I know we already have the entity manager injected, but can we please at least inject the storage controller, and maybe the render controller as well? Better typehints++
Same here with the render controller. Also, wouldn't we already know the entity type?
Comment #52
ParisLiakos CreditAttribution: ParisLiakos commentedYeap, it is called by aggregator_page_source_form()..
i fixed the title callbacks..also failures..i have no clue how this passed in the first time.
About the typehints, while i agree, and i started doing it, the constructor became unreadable (4 extra arguments)..
this is a result of falsely putting everything in one controller..i would rather split the controller in 2: one for feeds one for items, one for categories and fix the typehints/arguments..but probably not in this issue? right?edit: this is actually a mess, we cant split it, all of them would need more or less the same things:/. i ll have to think a bit more about this, but, not in this issue anyway
Comment #53
kim.pepperLooking good. One minor nitpick.
Missing UrlGenerator docs.
Comment #54
ParisLiakos CreditAttribution: ParisLiakos commentedadded url generator docs
Comment #55
kim.pepperAssuming the bot comes back green. This has been through a number of reviews, so putting back to RTBC
Comment #56
kim.pepperThis is blocking #1974394: Convert aggregator_page_category() to a Controller
Comment #57
alexpottSo let's convert aggregator_page_source_form here as well... so we can get rid of aggregator_page_source() it makes no sense to be maintaining duplicate code...
Also considering this patch is copying the code from aggregator_load_feed_items() to the storage controller we have the same problem here... we should have duplicate methods that do the same thing - that's a recipe for disaster... that means three choices...
I think I like option 2 as we can just Drupal::service in aggregator_page_category() and we can get rid of aggregator_load_feed_items
Comment #58
kim.pepperComment #59
kim.pepper@alexpott FYI there is a separate issue for #2040199: Convert aggregator_page_category_form and aggregator_page_source_form to a Controller which does away with aggregator_load_feed_items() and there is also a separate issue #2043581: Move aggregator_load_feed_items to the ItemStorageController, retain procedural wrapper
Comment #59.0
kim.pepperLink to meta issue
Comment #60
kim.pepperBlocked by #2040199: Convert aggregator_page_category_form and aggregator_page_source_form to a Controller as the currently forms just wrap these page callbacks. Once that is in, we can safely combine this and #1974394: Convert aggregator_page_category() to a Controller and move the common code to the controller.
Comment #60.0
kim.pepperrelated issues
Comment #61
kim.pepperunblocked
Comment #62
kim.pepperComment #63
jibranThis should be array.
nice.
Comment #65
disasm CreditAttribution: disasm commentedSwitching to ControllerBase. This also fixes the __construct errors with having the wrong type hint for UrlGeneratorInterface, since we're getting that from ControllerBase now.
Comment #66
disasm CreditAttribution: disasm commentedComment #67
disasm CreditAttribution: disasm commentedforgot to switch to $this->t() this one resolves that.
Comment #69
kim.pepperFixes an issue with $this->redirect()
Comment #70
disasm CreditAttribution: disasm commentedComment #72
disasm CreditAttribution: disasm commentedAttached patch replaces drupal_set_title with #title on the build array. It also renames $database to $connection. As for the test failures, I've done manual testing and can't replicate the issues. I see them though when I run simpletest.
Comment #73
kim.pepperChecked with dawehner in IRC and we still need to provide menu title callbacks for the menu links. Setting the page title doesn't do this for us.
Fixes tests locally.
Comment #74
ParisLiakos CreditAttribution: ParisLiakos commentedCool, its green! thanks guys
i dont know about that. what is the point of implementing ContainerInjectionInterface if you extend from ControllerBase?
you can have access to *anything* with $this->container
No need for the create() hack, really
Yay!
lets store $this->entityManager() result to a variable. No need to always call the method
Comment #75
disasm CreditAttribution: disasm commentedSpoke with @ParisLiakos in IRC. Removing __construct and create function, and adding two getters for the connection and categoryStorage variables. Creating variables for any getter methods called more than once in a function.
Comment #76
disasm CreditAttribution: disasm commentedwrong interdiff, here's the right one.
Comment #77
xjmThanks for your work on this issue! Please see #1971384-43: [META] Convert page callbacks to controllers for an update on the routing system conversion process.
Comment #79
ParisLiakos CreditAttribution: ParisLiakos commentedi think we can easily remove those properties too and just do a one liner like ControlerBase currently does.
eg
protected function getDatabase() {
return $this->container->get('database');
}
Comment #80
ParisLiakos CreditAttribution: ParisLiakos commentedso #2077513: Refactor ControllerBase to be more consistent with FormBase will fundamentaly change the way ControllerBase works, so i guess we should inject everything.
sorry for that
Comment #81
disasm CreditAttribution: disasm commentedreverting #74 aside from #3 per discussion in IRC.
Comment #83
disasm CreditAttribution: disasm commented#81: drupal8.aggregator-module.1974408-80.patch queued for re-testing.
Comment #85
disasm CreditAttribution: disasm commented#81: drupal8.aggregator-module.1974408-80.patch queued for re-testing.
Comment #86
ParisLiakos CreditAttribution: ParisLiakos commentedthis is unnecessary change and also i think we use to call it database anyway, across core
besides that its ready to go
Comment #87
kim.pepperReverts the rename of database => connection.
Comment #88
ParisLiakos CreditAttribution: ParisLiakos commentedthanks
Comment #89
webchickSorry to mark "needs work" for something so simple, but...
Both of these are missing $this-> before t('Title'). I'd just fix it on commit but don't want to inadvertently make a typo that causes HEAD to fail, so testbot validation would be great.
Comment #90
kim.pepperFixes missing t() => $this->t() conversion.
Comment #91
webchickRestoring RTBC. If the bot disagrees it'll move it back to needs work. :)
Comment #92
webchickGreat, thank you!
Committed and pushed to 8.x.
Comment #93.0
(not verified) CreditAttribution: commentedblocked