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 |
---|---|---|---|
#29 | wscci-1987624-29.patch | 9.58 KB | dawehner |
#29 | interdiff.txt | 1.02 KB | dawehner |
#24 | drupal8.batch_test.1987624-24.patch | 9.48 KB | disasm |
#22 | drupal8.batch_test.1987624-22.patch | 152.35 KB | disasm |
#22 | interdiff.txt | 3.1 KB | disasm |
Comments
Comment #1
vijaycs85Need to rewrite the whole module to make test sync with current test implementation. For more details, please refer: #1988802: [META] Rewrite test modules in system to provide better unit testing.
Comment #2
ayelet_Cr CreditAttribution: ayelet_Cr commentedComment #3
robeano CreditAttribution: robeano commentedWorking on this one.
Comment #4
robeano CreditAttribution: robeano commentedAdding a patch which converts the page callbacks in batch_test module to use the new BatchTestController.
Changes made:
This patch covers page callbacks. Forms have not been converted.
This issue now covers the needs of other page callback issues which will be marked as duplicates:
#1987622: Convert batch_test_large_percentage() to a new style controller
#1987626: Convert batch_test_no_form() to a new style controller
#1987628: Convert batch_test_programmatic() to a new style controller
#1987632: Convert batch_test_theme_batch() to a new style controller
#1987630: Convert batch_test_redirect_page() to a new style controller
Comment #6
robeano CreditAttribution: robeano commentedA couple of the controllers needed to support path arguments. The routes have been updated. Also, the controller methods were renamed to simplify things a bit. Ready for a review.
Comment #7
disasm CreditAttribution: disasm commentedThis is almost RTBC! A couple comments below:
Because these aren't MENU_CALLBACK, the menu items should remain, remove page callback and access callback, and add 'route_name' => 'name_of_route_in_yaml_file'.
need a blank line between last two closing braces. Also missing a newline at end of file.
Comment #8
piyuesh23 CreditAttribution: piyuesh23 at QED42 commentedUpdated the Patch with the information mentioned in the previous comment. Uploading the new patch and interdiff file.
Comment #9
piyuesh23 CreditAttribution: piyuesh23 commentedComment #11
disasm CreditAttribution: disasm commentedThanks for picking this up piyuesh23. Note that we aren't doing any major cleanups for test modules (creating services, moving helper methods into controllers, etc... We just need to get tests working again. That being said, there are some minor cleanups that shouldn't take much more than 5 minutes to do. I've listed them below.
patterns should begin with a leading '/'
Since we're not injecting anything, lets just make this a plain class and not extend/implement anything.
not needed. No services are injected in this controller.
We don't call controller methods menu callbacks. start with the description, no need for the prefix.
Comment #12
piyuesh23 CreditAttribution: piyuesh23 commented@disasm Thanks for reviewing and helping out with this issue. Uploading the fixed patch and interdiff files.
Comment #13
piyuesh23 CreditAttribution: piyuesh23 commentedComment #15
disasm CreditAttribution: disasm commentedLets use _title property in the routing.yml file.
Comment #16
disasm CreditAttribution: disasm commentedSpent some time debugging these test failures, and these are passing locally now. Here's the patch. We needed a default set for value. Also, fixed the titles above, so this should be green and ready to be reviewed. Thanks piyuesh for all your work on this!
Comment #18
disasm CreditAttribution: disasm commented#16: drupal8.system-module.1987624-16.patch queued for re-testing.
Comment #19
dawehnerThat one is really just a menu callback, so can be dropped.
I guess we should put some @return and some @param on there, just short ones.
Comment #20
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 #21
dawehner.
Comment #22
disasm CreditAttribution: disasm commentedaddresses comments in #19. $value param has no description, because digging into the functions that it's passed to explains about as much as these methods do.
Comment #23
dawehnerThis diff seems too big.
Comment #24
disasm CreditAttribution: disasm commentedtry again. no interdiff, same as above.
Comment #26
disasm CreditAttribution: disasm commented#24: drupal8.batch_test.1987624-24.patch queued for re-testing.
Comment #27
disasm CreditAttribution: disasm commentedI ran a few simple tests as well as enabled content translation for pages. Are there any specific other manual tests that need done? I experienced no errors in my manual testing.
Comment #28
disasm CreditAttribution: disasm commented#27 was meant for a different issue.
Comment #29
dawehnerJust fixing some tiny docs
Comment #30
jibranPatch is green and we already have tests for this code so i think it is good to go.
Comment #31
webchickCommitted and pushed to 8.x. Thanks!