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 |
---|---|---|---|
#66 | batch-1987816-66.patch | 7.89 KB | dawehner |
#63 | drupal8.system-module.1987816-63.patch | 7.89 KB | disasm |
#63 | interdiff.txt | 413 bytes | disasm |
#62 | drupal8.system_batch_page.1987816-62.patch | 7.84 KB | disasm |
#62 | interdiff.txt | 395 bytes | disasm |
Comments
Comment #1
pdrake CreditAttribution: pdrake commentedComment #3
pdrake CreditAttribution: pdrake commentedComment #4
dawehnerYeah there is a @todo but anyway: I guess this should be Drupal::setContainer
This could use Drupal::request()
Instead of injecting the request object you can directly have this on the controller method, so it looks like that: public function batchPage(Request $request) { ... }
Callbacks can be skipped totally from hook_menu.
Comment #5
pdrake CreditAttribution: pdrake commentedI think this patch addresses everything except your comment in authorize.php. That was copied from elsewhere, might make sense to just address the todo in all places at once (which could be in this patch, or not).
Comment #6
ParisLiakos CreditAttribution: ParisLiakos commentedi really like this patch! very good job, thanks.
some notes:
i doubt this todo will be removed in D8:(
but drupal_container will.
so better use
Drupal::getContainer()->set(..);
should be Drupal::request()
this also needs updating, since we are returning a Response
this should return a Response object i think
Comment #7
pdrake CreditAttribution: pdrake commentedThis version of the patch restores the menu callback because of the theme callback.
Comment #8
pdrake CreditAttribution: pdrake commentedOk, here's another try, addressing most of the comments in #6.
Comment #9
pdrake CreditAttribution: pdrake commentedThat didn't work. Trying again.
Comment #11
pdrake CreditAttribution: pdrake commented#9: drupal-batch_page_controller-1987816-9.patch queued for re-testing.
Comment #12
ParisLiakos CreditAttribution: ParisLiakos commentedlooks great! just a nitpick
we use the full namespace on docblocks
i am not sure whether we should remove this, but i agree with removing it, since fixing this would require quite some work, which is D9 material imo
Also, the @todo that this refers to, no longer exists...so maybe just rephrase it to:
@todo Refactor this.
if not removing completely
Comment #13
pdrake CreditAttribution: pdrake commentedComment #14
ParisLiakos CreditAttribution: ParisLiakos commentedsorry for that, but full namespace needs the leading \
Comment #15
pdrake CreditAttribution: pdrake commentedComment #16
dawehnerIt would be cool to ensure that the theme callback is still working, but i'm not really sure how to ensure that.
Comment #17
ParisLiakos CreditAttribution: ParisLiakos commentedi think its ready to go
Comment #18
alexpott#15: drupal-batch_page_controller-1987816-15.patch queued for re-testing.
Comment #20
ParisLiakos CreditAttribution: ParisLiakos commentedreroll
Comment #22
ParisLiakos CreditAttribution: ParisLiakos commentedyeah, no has there
Comment #23
ParisLiakos CreditAttribution: ParisLiakos commentedsame patch..sigh
Comment #24
ParisLiakos CreditAttribution: ParisLiakos commentedinterdiff
Comment #25
dawehnerI'm wondering whether it might be possible to know whether this is $_GET or $_POST all the time? In that case we could directly access the values via $request->query->get for example.
Comment #26
ParisLiakos CreditAttribution: ParisLiakos commentedi agree, i think its always GET, but i am not sure, thats why i left it that way
Comment #27
pguillard CreditAttribution: pguillard commented- I re-rolled that patch and replaced $request->get('op', ''); with $request->query->get('op', '');
- Tested quickly by checking manually for updates here /admin/reports/updates/check
Hope it's fine, but please check, as I'm not experienced enough !!
Comment #28
pguillard CreditAttribution: pguillard commentedParisLiakos, corrected the missing $request->query->get
Comment #29
ParisLiakos CreditAttribution: ParisLiakos commentedthanks!
this middle line is not needed
Seems we forgot to add @param documentation for the $request parameter
Comment #30
ParisLiakos CreditAttribution: ParisLiakos commentedeer..i meant to NW
Comment #31
pguillard CreditAttribution: pguillard commentedDone
Comment #32
ParisLiakos CreditAttribution: ParisLiakos commentedperfect, thanks!
Comment #33
YesCT CreditAttribution: YesCT commentedThis issue was RTBC and passing tests on July 1, the beginning of API freeze.
Comment #34
fubhy CreditAttribution: fubhy commentedWait, what's with RouteBuilderStatic? We added a @todo for this issue there but we don't touch it with the patch from #31.
Comment #35
ParisLiakos CreditAttribution: ParisLiakos commentedi dont have a clue..why would we need that in this conversion? or maybe its not needed now? dunno really, but we can open a followup to investigate it.
Comment #36
alexpottThis comment is unnecessary.
We now should be using the request here... instead of getting from global state.
Comment #37
vijaycs85Updating changes for #36
Comment #38
dawehner$_REQUEST also potentially contain $_POST information, so let's go with $request->get('id'), as it is safer, see
the comments #25, #26# and #27 on this issue.
Comment #39
vijaycs85Sure, updating as per #38...
Comment #40
dawehnerThank you very much!
Comment #41
xjm#39: 1987816-system_batch_page-controller-39.patch queued for re-testing.
Comment #42
alexpottThis @todo is a bit vague. It seems this was suggested in #12 and introduced in #13. Actually, we should remove the @todo as the request service is synthetic and this is exactly what #2004086: The Request service must be synthetic is adding.
Comment #43
dawehnerIgnore that please.
Comment #44
disasm CreditAttribution: disasm commentedreroll!
Comment #45
jibranNW because of #42.
Comment #46
jibranNW because of #42.
Comment #47
dawehnerI am wondering why you did this change. On the longrun this will be done using synthetic on the request service but is this really needed here?
If you do return HTML in the sense of a drupal page you should use rather _content than _controller
Comment #48
disasm CreditAttribution: disasm commentedLets see if this passes without all the additional changes to core files. I like having the $request object instead of $_REQUEST in _batch_page() but this is touching way too many things in my opinion and I think that's out of scope for this conversion.
Comment #49
dawehnerThese changes are perfectly fine
Comment #50
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 #51
dawehnerComment #52
disasm CreditAttribution: disasm commentedUndoing #48. Removing setting the request int the container. We'll see if this passes that way or if it's required. As for changing to _content, the method has detailed comments about how we don't want to create a full page so the message will be displayed on the following page load, hence the usage of _controller.
Comment #54
disasm CreditAttribution: disasm commented#52: drupal8.system_batch_page.1987816-52.patch queued for re-testing.
Comment #56
disasm CreditAttribution: disasm commented#52: drupal8.system_batch_page.1987816-52.patch queued for re-testing.
Comment #57
dawehnerThe size of the patch is too damn high.
Comment #58
disasm CreditAttribution: disasm commentedtry this one.
Comment #59
dawehnerI kind of think that we need manual testing on this one as batch is damn complicated: For example the manual testing has to ensure that the admin theme is still used.
Comment #60
ParisLiakos CreditAttribution: ParisLiakos commentedlets remove this @todo like #42 points out
Also, lets please restore the set('request', $request) call in authorize.php which was removed in #52
Since we are creating the request, lets put in the container, so we avoid potential breakage in future of services that depend on it
Comment #61
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 #62
disasm CreditAttribution: disasm commentedadds back the thing that was removed that I think was added at some point. You get the picture.
Comment #63
disasm CreditAttribution: disasm commentedadds back setting of request in container in authorize.php.
Comment #64
ParisLiakos CreditAttribution: ParisLiakos commentedlets get this finally in..if bot agrees ofc
Comment #65
webchickThis part looked weird to me, but I notice we do the same thing in update.php so apparently this is just stuff you have to do in standalone scripts these days.
However, no longer applies. :(
Comment #66
dawehnerThis bit is also done in #2004086: The Request service must be synthetic, totally fine.
Comment #67
jibranTested manually worked fine. Back to RTBC.
Comment #68
jibranRemoving the tag.
Comment #69
webchickCommitted and pushed to 8.x. Thanks!