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 |
---|---|---|---|
#40 | drupal-convert_locale_change-1978928-40.patch | 5.02 KB | piyuesh23 |
#40 | interdiff.txt | 671 bytes | piyuesh23 |
#36 | interdiff.txt | 1.51 KB | piyuesh23 |
#35 | drupal-convert_locale_change-1978928-35.patch | 4.98 KB | piyuesh23 |
#35 | interdiff.txt | 1.51 KB | piyuesh23 |
Comments
Comment #1
PanchoComment #2
InternetDevels CreditAttribution: InternetDevels commentedAs this issue hasn`t been fixed yet we are going to work today with this issue during Code Sprint UA.
Comment #3
PanchoThat's fine!
Comment #4
InternetDevels CreditAttribution: InternetDevels commentedPatch attached.
Comment #5
dawehnerThe construct method should be documented
This should have a @return documentation. Let's also use a camelCase method here.
If you have just a callback you don't need an entry in hook_menu anymore.
Comment #6
InternetDevels CreditAttribution: InternetDevels commentedHere is the new patch.
Comment #7
dawehnerIf you just do a redirect response I would say that _controller is the better choice as _content
That's the last bit, I promise :)
Comment #8
InternetDevels CreditAttribution: InternetDevels commentedAnd finally (I hope so :))...
Comment #10
dutchyodaComment #11
dutchyodaComment #12
piyuesh23 CreditAttribution: piyuesh23 at QED42 commentedTested on my local and the patch works fine. Ran the forum test case through simpletest module on my local. Attaching a screenshot for more info.
Queuing this issue for re-testing.
Comment #13
piyuesh23 CreditAttribution: piyuesh23 commented#8: locale-change_callback_to_controller-1978928-8.patch queued for re-testing.
Comment #15
disasm CreditAttribution: disasm commentedreroll!
Comment #17
piyuesh23 CreditAttribution: piyuesh23 at QED42 commentedThe failed patch was missing the route_name property. Added it back. Attaching the fixed patch.
Comment #18
piyuesh23 CreditAttribution: piyuesh23 commentedComment #20
piyuesh23 CreditAttribution: piyuesh23 commentedFixing the patch. Updated the include path for couple of components and added route_name property. Attaching the patch and re-queuing it for testing.
Comment #21
piyuesh23 CreditAttribution: piyuesh23 commentedComment #23
piyuesh23 CreditAttribution: piyuesh23 commentedRenaming interdiff.patch to interdiff.txt.
Comment #24
piyuesh23 CreditAttribution: piyuesh23 commentedComment #26
piyuesh23 CreditAttribution: piyuesh23 commentedController was missing a return statement for the batch process. Tests might be failing coz of this. Attaching another patch and requeing for testing.
Comment #27
piyuesh23 CreditAttribution: piyuesh23 commentedComment #28
rahul.shindePatch provided in #26 is working as expected. Please find attachment (Available translation updates | drupal8.png) for better understanding.
Comment #29
YesCT CreditAttribution: YesCT commentedThis issue was RTBC and passing tests on July 1, the beginning of API freeze.
Comment #30
alexpottNeed to inject the url generator.
Let's remove the entire hook menu... a user should never actually land on the page as the controller does a redirect to admin/reports/translations
Comment #31
piyuesh23 CreditAttribution: piyuesh23 commented@alexpott
Thanks for the review, makes sense to remove the menu hook completely..:)
Updating the patch to inject the url generator properly and removing the menu item.
Comment #32
piyuesh23 CreditAttribution: piyuesh23 commentedComment #34
tim.plunkettYou're missing $this->urlGenerator
While rerolling, can you just make this one line?
return new RedirectResponse($this->urlGenerator->
...Comment #35
piyuesh23 CreditAttribution: piyuesh23 commented@tim Thanks. Uploading a patch with the above fixes.
Comment #36
piyuesh23 CreditAttribution: piyuesh23 commented@tim Thanks. Uploading a patch with the above fixes.
Comment #37
piyuesh23 CreditAttribution: piyuesh23 commentedComment #39
tim.plunkettWell, you actually need to pass in the generator service.
Comment #40
piyuesh23 CreditAttribution: piyuesh23 commented@tim Thanks for helping out on this. Updated the patch.
Comment #41
piyuesh23 CreditAttribution: piyuesh23 commentedComment #42
dawehnerThis looks fine.
Comment #43
alexpottCommitted 2b08f2e and pushed to 8.x. Thanks!
Comment #45
Gábor HojtsyComment #46
cilefen CreditAttribution: cilefen commented