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 |
---|---|---|---|
#42 | drupal-1987572-42.patch | 12.6 KB | mrded |
#42 | interdiff-1987572-33-42.txt | 1.5 KB | mrded |
#33 | drupal-1987572-33.patch | 12.54 KB | mrded |
#33 | interdiff-1987572-28-33.txt | 754 bytes | mrded |
#28 | drupal-1987572-28.patch | 12.67 KB | mrded |
Comments
Comment #1
vijaycs85Initial patch...
Comment #2
dawehnerAs there is no need for any injection, there is no need for the Interface as well.
What is the point in using the database here? I guess this was just a c&p
Afaik we don't need the title, so there should be no reason to keep the hook_menu entry.
Comment #3
kim.pepper#1: 1987572-session-test-get-route-1.patch queued for re-testing.
Comment #4
vijaycs85Closing as it is test module issue.
Comment #5
Sean Buscay CreditAttribution: Sean Buscay commentedComment #6
Sean Buscay CreditAttribution: Sean Buscay commentedThis patch converts all menu call backs in session_test.module to routes, and adds the controller for the tests.
Comment #8
Sean Buscay CreditAttribution: Sean Buscay commentedThe tests failed at a part of the code which the patch did not touch. The tests which the patch relies on have intermittently failed on local test systems but this is the first known time they have failed with the test bot. The tests have been re-run to see if the fail occurs each test run or just intermittently.
The next step will be to determine if the patch introduced something that caused the fail with the test bot test, or whether the fail was due to the previously non reproducible fails from some local systems.
Comment #9
Sean Buscay CreditAttribution: Sean Buscay commented#6: 1987572-convert-session-tests-to-routes.patch queued for re-testing.
Comment #11
disasm CreditAttribution: disasm commentedUnassigning since this hasn't been worked on in a while.
Comment #12
mrded CreditAttribution: mrded commentedWorking on this as part of London sprint.
Comment #13
mrded CreditAttribution: mrded commentedPlease, take a look my patch.
Comment #14
vijaycs85Nice one @mrded, few comments at first look.
Minor: remove prefix 'Page callback/ Menu callback' ?
I guess, we can inject t()
Comment #16
mrded CreditAttribution: mrded commentedNew version.
Comment #17
vijaycs85good work @mrded. Looks good to me.
Comment #18
dawehnerIf you use ControllerBase you could directly use $this->t()
Comment #19
vijaycs85Quick one for #18
Comment #20
vijaycs85wrong diff file...
Comment #21
dawehnerHa, someone was lazy here :)
Let's just add a @return statement and @param on some of them.
The full hook_menu can be killed as we just have MENU_CALLBACKS on there.
Comment #22
disasm CreditAttribution: disasm commentedComment #23
mrded CreditAttribution: mrded commentedThere is a new patch.
Comment #24
dawehnerThe point is to also describe what the values are instead of just adding the @param/@return statements.
If you extend from ControllerBase you can use just $this->t() and you should.
Comment #25
disasm CreditAttribution: disasm commentedComment #26
mrded CreditAttribution: mrded commentedNo, I cannot. I got the follow error:
( ! ) Fatal error: Call to undefined method Drupal\session_test\Controller\SessionTestController::t() in /Users/mrded/Sites/drupal8/core/modules/system/tests/modules/session_test/lib/Drupal/session_test/Controller/SessionTestController.php on line 74
Comment #27
disasm CreditAttribution: disasm commentedmrded, here is how you extend ControllerBase so you can use $this->t() in the class:
This becomes:
use Drupal\Core\Controller\ControllerBase;
replace this with:
class SessionTestController extends ControllerBase {
Then the constructor and create are unneeded, as well as $translationManager.
Comment #28
mrded CreditAttribution: mrded commented@disasm, thank you for your help!
There is a new patch, hopefully it will be the final version :)
Comment #30
disasm CreditAttribution: disasm commented@mrded please attach an interdiff when you make changes. This makes it easier on reviewers to look at what's different between the patch in #23 and #28. I'm retesting this because having troubles seeing how a test that doesn't use the session_test module is failing.
Comment #31
disasm CreditAttribution: disasm commented#28: drupal-1987572-28.patch queued for re-testing.
Comment #32
disasm CreditAttribution: disasm commentedLooking good! One minor changge, and then this is ready.
Since nothing is being injected, this block can be completely removed.
Comment #33
mrded CreditAttribution: mrded commentedComment #34
dawehnerAre you sure this should not rather use a Response object?
Comment #35
mrded CreditAttribution: mrded commentedNo :) Testing results is same. Can you advise me what is the best way?
Comment #36
disasm CreditAttribution: disasm commentedIf it still passes tests, lets switch it to a response object. If that causes a fail to happen, lets keep it the way it is.
Comment #37
mrded CreditAttribution: mrded commentedJust to be sure, when you say "response object" do you mean changing $this->t() to $this->translationManager->translate() ?
Comment #38
dawehnerNo, I rather suggest
return new Response($this->t(...));
and that's it.Comment #39
mrded CreditAttribution: mrded commentedShould I wrap all returns to new Response() ? Where can I read about it? Thank you.
Comment #40
disasm CreditAttribution: disasm commenteddawehner is only referring to this one. It's printing, instead of returning a Response object. In general, you should never print data from a controller. return a string, or an array is fine, but never print data.
this would become return new Response($this->t('A message was set.'));
Comment #41
disasm CreditAttribution: disasm commentedComment #42
mrded CreditAttribution: mrded commentedGot it.
Comment #43
dawehnerThank you!
Comment #44
mrded CreditAttribution: mrded commentedFinally! Thank you guys for your help with it! :)
Comment #45
webchick#42: drupal-1987572-42.patch queued for re-testing.
Comment #46
webchickWe're trying to remove calls to global variables throughout D8 core. However, I grepped and couldn't find much in the way of lower-case "session" and I found much in the way of "SESSION." So I think this is fine for now.
(minor) two newlines rather than one.
Fixed on commit.
Committed and pushed to 8.x. Thanks!
Comment #47
tstoecklerFollow-up: #2090969: Minor code-style issues in SessionTestController