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 |
---|---|---|---|
#23 | interdiff.txt | 1.23 KB | h3rj4n |
#23 | system-1987808-23.patch | 3.81 KB | h3rj4n |
#21 | interdiff.txt | 968 bytes | h3rj4n |
#21 | system-1987808-20.patch | 3.52 KB | h3rj4n |
#15 | interdiff.txt | 722 bytes | h3rj4n |
Comments
Comment #1
vijaycs85Initial patch...
Comment #3
Dave.Ingram CreditAttribution: Dave.Ingram commentedI'll have a look at these errors.
Comment #4
Dave.Ingram CreditAttribution: Dave.Ingram commentedUpdated this patch to fix the errors. Tested it manually and all seems to be working.
Comment #6
Dave.Ingram CreditAttribution: Dave.Ingram commented#4: admin_compact-1987808-4.patch queued for re-testing.
Comment #8
Dave.Ingram CreditAttribution: Dave.Ingram commented#4: admin_compact-1987808-4.patch queued for re-testing.
Comment #9
dawehnerShould be @param string $node
I can't see why we can't use RedirectResponse already.
Comment #10
vijaycs85updating changes mentioned in #9.
Comment #11
Crell CreditAttribution: Crell commentedThis won't work correctly if Drupal is in a subdirectory. We need instead to inject the generator, and/or rely on code in #1668866: Replace drupal_goto() with RedirectResponse and #1623114: Remove drupal_exit() . That's unfortunately a bit complicated, especially with the generator issue only RTBC, not committed.
We discussed this at the sprint on Friday, and decided that for now we can leave the drupal_goto() in place with a @todo.
Something else just occurred to me. We ARE posting to this route, right? not GETing? If so, we can probably restrict it to just POST.
If we are GETing it, bad on us and we should see if that can be fixed.
Comment #12
dawehnerthis full entry can now also be removed, as it is not longer needed.
Comment #13
tim.plunkettRerolled, injected, refactored.
Comment #14
dawehnerNeeds some docs and public
Comment #15
h3rj4n CreditAttribution: h3rj4n commentedAdded the public part. What do you mean with docs? Some more documentation? Where?
Comment #17
Crell CreditAttribution: Crell commentedI don't know why I put docs in there. I think it just needed the public keyword.
Comment #18
tim.plunkettBecause it needs something on the line after @return.
Comment #19
h3rj4n CreditAttribution: h3rj4n commentedIt couldn't be applied because of the commit of #2056513: Remove PluginUI subsystem, provide block plugins UI as one-off. That happened just before I updated the issue I guess.
Can I just move the code as documented in that issue?
I'll add this to the other class.
Comment #20
tim.plunkettNo, you need to reintroduce SystemController with just your method.
Comment #21
h3rj4n CreditAttribution: h3rj4n commentedAdded the changes.
Comment #23
h3rj4n CreditAttribution: h3rj4n commentedI just missed your comment but your absolutely right. I wasn't reading. Changed it so that it works. The tests don't fail any more on my system.
Comment #24
h3rj4n CreditAttribution: h3rj4n commentedComment #25
Crell CreditAttribution: Crell commentedLet's just get this over with. :-) Thanks, h3rj4n.
Comment #26
dawehnerWell instead of url() we could also directly use generateFromPath on the url generator.
Comment #27
Crell CreditAttribution: Crell commentedI'm assuming we'll change that line again once #2052995: Add a route to the frontpage is in, but no sense waiting for that.
Comment #28
dawehnerI agree.
Comment #29
alexpottCommitted f325f63 and pushed to 8.x. Thanks!
Comment #30
tstoecklerCan someone explain why this part was added? Is this some sort of new standard that controllers shoud implement ControllerInterface?
Comment #31
tim.plunkettIt resulted from blindly following the conversion instructions. It's not harmful, and may eventually be used, but is overkill at this point.
However, the committees have indicated they're okay with this.
Comment #32
tstoecklerOK, sure, I can live with it. Thanks!