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
Comments
Comment #1
kim.pepperComment #2
kim.pepperWe need the AggregatorController for in #1972262: Convert aggregator_feed_add to a new style controller to do this.
Comment #3
ParisLiakos CreditAttribution: ParisLiakos commentedthe issue above is in
Comment #4
kim.pepperThis patch is a straight router conversion. It still calls functions in aggregator.pages.inc which will be moved to the controller eventually.
Comment #5
kim.pepperForgot to remove the old function from aggregator.pages.inc
Comment #6
kim.pepperForgot to remove the old function from aggregator.pages.inc
Comment #8
kim.pepper#4: 1974370-aggregator-page-last-controller-4.patch queued for re-testing.
Comment #9
kim.pepper#5: 1974370-aggregator-page-last-controller-5.patch queued for re-testing.
Comment #10
ParisLiakos CreditAttribution: ParisLiakos commentedLets inject config
should be @todo
Comment #11
ParisLiakos CreditAttribution: ParisLiakos commentedactually lets wait for #1974372: Convert aggregator_page_sources() to a Controller, it will take care inject the configFactory
Comment #12
kim.pepperRe-roll against head. Added fixes from #10.
Comment #13
dawehner@kim.pepper
Please try to use git diff instead of git format-patch, because you end up with multiple commits in the patch file which is really confusing!
Comment #14
kim.pepperHi @dawehner. I usually use git diff, but @tim.plunkett requested I follow the instructions on http://drupal.org/documentation/git/configure which uses format patch! So confusing...
Comment #15
dawehnerWell, http://drupal.org/node/1054616 is also an additional page which is helpful to read. Multiple diffs in one patch is simply not scannable by a human :)
Comment #16
kim.pepperHumans read these?? ;-)
Comment #17
kim.pepperAs an aside, what real benefit do we get from calling
Drupal::moduleHandler()->loadInclude()
instead ofmodule_load_include()
. Neither of them can be mocked out for unit tests.Comment #18
ParisLiakos CreditAttribution: ParisLiakos commentedActually you can mock module handler and then call
nothing you can do with module_load_include
Comment #19
kim.pepperThere ya go!
Comment #20
kim.pepperReformatted patch to use just diff instead of format-patch as per #13.
Comment #21
ParisLiakos CreditAttribution: ParisLiakos commentedWe dont
use ;
global classes inside class context. just prefix with \\Drupal::moduleHandler()
Comment #22
kim.pepperFixes from #21 and others provided by @ParisLiakos in IRC.
Comment #23
dawehnerWhat about injecting config() or at least use \Drupal::config ?
Comment #24
kim.pepperInjected the configFactory and the moduleHandler. A few other code style cleanups as well.
Comment #25
ParisLiakos CreditAttribution: ParisLiakos commentedperfect, thanks!
Comment #27
ParisLiakos CreditAttribution: ParisLiakos commented2 tracker fails? seems irrelevant
#24: 1974370-aggregator-page-last-controller-24.patch queued for re-testing.
Comment #28
kim.pepperBack to RTBC
Comment #29
alexpottSo the big thing is that there is 0 test coverage for this route in simpletest... so a green test is not enough to commit. Ideally this issue should add tests to confirm that the refactor is successful.
Additionally the following changes are unrelated...
This change is not according to coding standards... the indentation is correct. See https://drupal.org/coding-standards/docs#todo
These are unrelated changes and unnecessary.
Comment #30
alexpottAdding tags due to...
Comment #31
kim.pepperReverts the coding standards issues in #29.
Comment #32
kim.pepperAdded a very basic test for the 'aggregator' path, which mirrors existing tests.
Comment #34
kim.pepperDrupal\translation_entity\Tests\EntityTranslationUITest->testTranslationUI() looks like an unrelated failure. Re-testing
Comment #35
kim.pepper#32: 1974370-aggregator-page-last-controller-32.patch queued for re-testing.
Comment #36
kim.pepperAdded tests, so back to RTBC
Comment #37
dawehnerEven I personally think that this listing pages are actually should be views out of the box, this page works pretty well.
Comment #38
dawehnerups.
Comment #39
alexpottCommitted c9b5abc and pushed to 8.x. Thanks!
Comment #40.0
(not verified) CreditAttribution: commentedAdded link to meta issue