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 |
---|---|---|---|
#62 | overlay-1978938-62.patch | 9.35 KB | pfrenssen |
#62 | interdiff.txt | 2.14 KB | pfrenssen |
#58 | overlay-1978938-58-test_only.patch | 4.6 KB | pfrenssen |
#58 | overlay-1978938-58.patch | 9.17 KB | pfrenssen |
#58 | interdiff.txt | 2.49 KB | pfrenssen |
Comments
Comment #1
sidharthapHere is the initial patch created. we may required to correct this thing if not works.
Comment #2
dawehnerC&P error :)
Should be probably more something like OverlayDismissMessageAccessCheck or something shorter?
_access at the front seems to be redundant.
no need for the constructor if nothing is injected.
some hidden spaces :) These should be removed according to http://drupal.org/node/1354
The request object should be injected.
the user data should be injected as well.
Comment #3
ParisLiakos CreditAttribution: ParisLiakos commentedhi:)
an addition to above
should be _controller instead of _content and new RedirectResponse instead of drupal_goto
Comment #4
sidharthapComment #5
sidharthapCorrected class name and removed other changes.
Comment #6
ParisLiakos CreditAttribution: ParisLiakos commentedfiles should be 0644
the url needs to be absolute for this to work
array('absolute' => TRUE)
you can totally remove the menu item now
newline missing
Comment #7
dawehnerAs long we don't inject anything you don't have to implement the interface.
Comment #8
sidharthapComment #10
sidharthapComment #11
dawehnerNo need for two dots here :)
It actually returns a redirect response ...
I guess we should update the comment to match the new code. Maybe check whether the new access system can deal with it already.
Comment #12
laurentchardin CreditAttribution: laurentchardin commentedAdding a link to http://drupal.org/node/1978932 since we are both introducing the same controller.
Comment #13
laurentchardin CreditAttribution: laurentchardin commentedAdded dawehner's return + updated issue url for CSRF support in routing (not there yet) + added a todo about ?destination support here.
Comment #14
ParisLiakos CreditAttribution: ParisLiakos commentedThanks for your patience. There are a few more nitpicks left:
Lets remove the one newline
Add a newline here inbetween
we need a newline inbetween here
i believe this is not needed anymore. ?destination now works
Comment #15
disasm CreditAttribution: disasm commentedreroll!
Comment #16
disasm CreditAttribution: disasm commentedAttached patch addresses Paris's comments above, and also cleans up some other coding standards.
Comment #17
ParisLiakos CreditAttribution: ParisLiakos commentedthanks! but, found more:/
nit! still need one less newline here:)
D'oh i just saw the comment:) probably C&P err from book module
ah, two backslashes here, should be one
Comment #18
pguillard CreditAttribution: pguillard commentedNow?
Comment #19
ParisLiakos CreditAttribution: ParisLiakos commentedhmm i think #18is based on #15 but #16 is a lot better
Comment #20
disasm CreditAttribution: disasm commentedattached patch and interdiff fixes requested changes above.
Comment #21
ParisLiakos CreditAttribution: ParisLiakos commentedthanks:)
Comment #22
YesCT CreditAttribution: YesCT commentedThis issue was RTBC and passing tests on July 1, the beginning of API freeze.
Comment #23
alexpottDeprecated :) use the request instead...
Comment #24
dawehnerFix that and the access checker return values.
Comment #25
somepal CreditAttribution: somepal commentedconsidering #23, re-roll of #20
Comment #26
somepal CreditAttribution: somepal commentedComment #27
alexpott@somepal what's the difference between #24 and #25?
Comment #28
somepal CreditAttribution: somepal commented@alexpott while #25 contains
$GLOBALS['user']->uid
to get the current user, @dawehner in #24 prefers it should be as,
Both the patches accidentally arrived at the same time. So #25 is not modification of #24 in any way.
Comment #29
alexpottNo worries @somepal xposts happen... the usage of globals is now deprecated so the patch in #24 is correct whilst the patch in #25 is not actually changing anything :) see https://drupal.org/node/2032447
I consider the patch in #24 rtbc... but if I do that then a commit might take longer :)
Comment #30
somepal CreditAttribution: somepal commented@alexpott ok then we are gtg with #24 :)
Comment #31
alexpottCommitted #24... 1021ebb and pushed to 8.x. Thanks!
Comment #33
dawehnerThis line seriously can't work, sorry. I guess we also need testcoverage then.
Comment #34
dawehnerstill needs tests
Comment #35
Crell CreditAttribution: Crell commentedCode looks fine; not sure what sort of tests we want to add here... (No more webtests, please...)
Comment #36
dawehnerTo be honest I think that a unit test ensures less here ... We could have unit tested that code without actually knowing whether it works.
Comment #37
dawehnerWrote some proper tests and injected quite a bunch of service.
Comment #39
jibranwhat?
80 char limit.
Comment #40
dawehneroh, no wonder that this fails.
Comment #41
jibranIt is green. It has unit tests. It is major. Let's get this in.
Comment #42
webchickNot sure why this is major, but I'd like to hold it for a couple of days in the hopes that #2057589: [Docs clean-up] Rename ControllerInterface to ContainerInjectionInterface gets committed.
Comment #43
Letharion CreditAttribution: Letharion commentedHoping that #2057589: [Docs clean-up] Rename ControllerInterface to ContainerInjectionInterface does get in, I'm jumping the gun and updating the patch. Not setting back to needs review as the patch doesn't apply, yet.
So just to be really clear, deliberately not changing status, but this patch is not the one that has been RTBCd.
Comment #45
Letharion CreditAttribution: Letharion commentedEdit: Ops, didn't realize testbot was gonna ruin the status setting. Oh well, leaving as is now.
Comment #47
Letharion CreditAttribution: Letharion commented#2057589: [Docs clean-up] Rename ControllerInterface to ContainerInjectionInterface is now in. Patch from #43 now applies.
Comment #48
dawehnerLet's get back to RTBC
Comment #49
alexpottThis can use the new $this->redirect method that is on ControllerBase now that we're extending from it.
Comment #50
mrded CreditAttribution: mrded commentedComment #52
dawehnerSorry to just fix that, but explaining would need the same amount of time.
Comment #54
h3rj4n CreditAttribution: h3rj4n commented#52: overlay-1978938-52.patch queued for re-testing.
Comment #56
dawehnerReroll.
Comment #57
pfrenssenAdd a blank line before the @return.
Indentation.
Remove one of the blank lines.
Indicating bad indentation. It's the namespace wrapper.
What's this? Does not seem to be needed.
Comment #58
pfrenssenI addressed the remarks from above. I also noticed that when I run the tests without the patch applied that I do not get any tests results back. Is that how these unit tests are supposed to behave?
Comment #59
dawehnerPer default the phpunit test runner just tell you about failed assertions. I would recommend to not run them via the simpletest UI as their are tons of bogs in the integration. (For example try to let them run twice)
Sorry but this change lets the phpunit test fail in a phpunit test only enviroment. Please add that bit back.
Comment #60
pfrenssenOk I will address this when I get back home this evening. Is there some documentation available on running PHPUnit tests so I can try this locally?
Comment #61
pfrenssenHere is a bit of documentation on running PHP Unit tests, in the change record: Test framework PHPUnit has been added. So the call to drupal_set_message() needs to be overridden in the test because PHPUnit does not include the procedural Drupal code by default, it relies on methods in classes by the autoloader.
Comment #62
pfrenssenOk I get it. We can't post a 'test_only' patch to prove that the test works, because the old code uses the drupal_valid_token() function from the global namespace, which the unit test cannot use.
The weird indentation of the test is because we expect to remove the override of drupal_set_message() in the future, and this would make the future patch much more readable as we wouldn't need to reformat the entire class. So it was actually fine like it was.
Sorry for the confusion, I was not familiar with this approach. This is a new patch, with interdiff against dawehner's last patch from #56. Ignore the one from #58.
Comment #63
pfrenssenComment #64
mrded CreditAttribution: mrded commentedComment #65
dawehnerYeah this is sad and confusing that we need to do that.
Comment #66
catchCommitted/pushed to 8.x, thanks!