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
Comment | File | Size | Author |
---|---|---|---|
#34 | 1974372-aggregator-sources-controller-34.patch | 7.59 KB | kim.pepper |
#34 | 1974372-interdiff.txt | 1.09 KB | kim.pepper |
#32 | 1974372-aggregator-sources-controller-30.patch | 7.42 KB | kim.pepper |
#32 | interdiff.txt | 779 bytes | kim.pepper |
#28 | 1974372-aggregator-sources-controller-28.patch | 7.43 KB | kim.pepper |
Comments
Comment #1
kim.pepperFirst attempt.
Comment #2
ParisLiakos CreditAttribution: ParisLiakos commentedUse the already injected entityManager here, instead of the procedural helpers
same..we need config.entity service injected
Comment #3
kim.pepperInjected all the dependencies. Seems a lot more code just to have an injected config factory :-(
Comment #4
ParisLiakos CreditAttribution: ParisLiakos commentedshould be @todo and a better message, eg:
@todo remove this once all callbacks are converted.
weird indentation.should be like:
Comment #5
kim.pepperThanks for the review @ParisLiakos Updated with feedback in #4
Comment #6
ParisLiakos CreditAttribution: ParisLiakos commentedlooks good to go now, thanks!
Comment #7
ParisLiakos CreditAttribution: ParisLiakos commentedNo longer applies
Since you are rerolling it
i think the rest of core is using _content here..i just noticed..so maybe we should do so instead of _controller..and i am not sure why there are two ways of doing the same thing
it is an array now should be:
* @return array
* A render array as expected by drupal_render().
Comment #8
kim.pepperRe-roll and fixes issues in #7
Comment #9
ParisLiakos CreditAttribution: ParisLiakos commentedoops, not a form array:) a render array
Comment #10
kim.pepperDamn. I need to actually stop copy paste from reviews. :-)
Fixes comment in #9
Comment #11
dawehnerI tend to like to not store the full config factory but just the config object needed below: 'aggregator.settings'. Do you have an opinion about that?
Comment #12
ParisLiakos CreditAttribution: ParisLiakos commentedat least
aggregator_page_last()
needs the system config, so we should store the configFactory so it can be reused.i think it looks good. thanks a lot @kim.pepper!
Really minor that shouldn't hold this patch:
The comment could be more general maybe:
The config factory to retrieve settings from.
Or just:
The configuration factory.
Comment #13
kim.pepperHere's the comment change in #12.
Comment #14
ParisLiakos CreditAttribution: ParisLiakos commentedThank you!
Comment #15
ParisLiakos CreditAttribution: ParisLiakos commentedreroll for admin overview conversion
Comment #16
alexpottI think we should to the conversion of this function in this patch... so that this conversion is done and the other conversion can use it... then we don't have to redo everyone one but the last one. We just have to remember to remove aggregator_load_feed_items on the last one :)
Comment #17
ParisLiakos CreditAttribution: ParisLiakos commentedactually this function does not belong to the controller
it is also being rewritten to use entity_query in #15266: Replace aggregator category system with taxonomy
then it could be moved to the ItemStorageController or a static method depending on #1742850: Follow-up for entity_load_multiple_by_properties() outcome
Comment #18
ParisLiakos CreditAttribution: ParisLiakos commenteddouble-post:/
Comment #19
ParisLiakos CreditAttribution: ParisLiakos commentedrefactored it a bit..
Comment #20
ParisLiakos CreditAttribution: ParisLiakos commentedwe could also move this function to the .module file as well temporarily
Comment #21
kim.pepperRe-roll.
Comment #22
dawehnerJust in general: this code could be replaced by an EQ, but yeah this is out of scope of this issue.
While manually testing the patch, I recognized the following bug.
This code breaks in objects, so we clearly have no test coverage :(
Comment #23
kim.pepperThanks for the review @dawehner.
This is a re-roll that fixes the Drupal => \Drupal issue and some missing imported. The patch was manually re-tested successfully locally.
Comment #25
kim.pepperFixed missing class imports, and added tests!
Comment #26
kim.pepperComment #28
kim.pepperHmmm. Somehow managed to remove permissions requirement from aggregator_page_last in a merge. Added now.
Comment #29
ParisLiakos CreditAttribution: ParisLiakos commentedwe have the moduleHandler injected so lets use that instead of \Drupal
once more, thanks for the tests!
Comment #31
kim.pepperUsing the injected modulehandler insteadas per #29.
ParisLiakos, I assume this is a temporary step, as most of that code will be inlined or moved to a manager, and we won't need moduleHandler anymore.
Comment #32
kim.pepperbetter attach patch...
Comment #33
dawehnerformat_string should be certainly String::format() but i'm not sure whether ->assert instead of ->assertTrue is the way to go
Comment #34
kim.pepperThanks @dawehner. Fixes #33
Comment #35
dawehnerManually testing works fine.I guess the tag can be removed now as well.
Comment #36
catchI had to double-check the nid, but it really is that old! Committed/pushed to 8x., thanks!
Comment #37
tstoecklerCan anyone tell me what #sorted TRUE does? I couldn't find it anywhere on api.drupal.org
Comment #38.0
(not verified) CreditAttribution: commentedAdded link to meta issue