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 |
---|---|---|---|
#43 | 1978924-locale_translate_export_form-43.patch | 18.55 KB | vijaycs85 |
#40 | form-1978924-40.patch | 18.58 KB | YesCT |
#34 | interdiff.txt | 1.02 KB | tim.plunkett |
#34 | form-1978924-34.patch | 18.52 KB | tim.plunkett |
Comments
Comment #1
PanchoHere's the conversion patch, tested to some degree, now let's see if our testbot agrees.
Comment #2
PanchoAwsome - the testbot liked it, too.
So please review now. I'm open for any suggestions.
Comment #3
tim.plunkettThis needs a reroll for FormBase
Comment #4
tim.plunkettUgh.
Comment #5
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 #6
disasm CreditAttribution: disasm commentedFormBase and changes to Language class.
Comment #8
disasm CreditAttribution: disasm commentedremoving request from submitForm. Getting via getRequest() instead.
Comment #10
Gábor HojtsyComment #11
LuxianComment #12
LuxianDone
Locale module test run fine my machine.
Update: I think this is part 2 from the process described in: A faster process for Drupal 8 routing conversions
Comment #13
pfrenssenThese should be ordered alphabetically, with Drupal namespaces at the top, followed by vendors.
This namespace doesn't seem to be used in the class.
Classes do not inherit their documentation, as they do something different than their parent class.
Remove the empty line.
Comment #14
vaibhavjainAdding a new patch, changes defined in #13 incorporated.
Comment #15
Crell CreditAttribution: Crell commentedNo. You should *never* send() a response yourself. Since forms can't return a response, there's a hack only forms are allowed to use. You need the FormBuilder service to do so, but I think you can get that from the container in the base class. Like so:
$this->container()->get('form_builder')->sendResponse($response);
(Where $response is the BinaryFileResponse object you already created.)
Not doing so will result in 2 responses being sent (one here, and another empty one later by Drupal). You don't want that.
Comment #16
tim.plunkettsendResponse is protected, you cannot call it, and it can only be triggered during form building.
I would suggest storing the filename in the $form_state, setting $form_state['rebuild'] = TRUE; and returning the binary response in buildForm.
Comment #17
vijaycs85no more drupal_set_message(). should be replaced by #title
Comment #18
tim.plunkett@vijaycs85 are you confusing that with drupal_set_title? we have no replacement for dsm yet.
Comment #19
vijaycs85indeed I'm. Sorry for the confusion.
Comment #20
ParisLiakos CreditAttribution: ParisLiakos commentedwell, triggering a file download on form submission should be a lot simpler than storing the filename, rebuild the form and then serve the file after a redirection.
how about this? introduce a "response" key, just like the "redirect" one, for every other kind of response a form needs to trigger
Comment #21
pfrenssenI really like this solution. Serving a download as the direct result of a form submission has many use cases, and this is a perfect example. The download is served immediately without redirecting the page. This feels very quick and responsive.
Also, the way this is implemented makes it really easy for developer to modify the response from within a form. A big +1 from me.
Just one small review remark:
Maybe mention this should be a Response object?
Comment #22
ParisLiakos CreditAttribution: ParisLiakos commentedsure! :)
Comment #23
pfrenssenGreat stuff. This looks ready.
Would this need a change record?
Comment #24
webchickSince this patch changes FormBuilder / FormBuilderInterface (which is not at all obvious from the issue title), I'd like Tim to take a look first. Also left him a tell on IRC.
Comment #25
webchick22: drupal.locale_export_form.1978924-22.patch queued for re-testing.
Comment #26
tim.plunkettThis shouldn't be needed, just store the uri and filename somewhere on $this or in $form_state['storage'], and do $form_state['rebuild'] = TRUE. Then you can check it in your buildForm (which can always return a response).
Comment #27
ParisLiakos CreditAttribution: ParisLiakos commentedthat would be then a regression from D7 because you would need one extra server response to accomplish the same thing.
I also think its much more DX friendly doing
than
Comment #28
tim.plunkettThat's fine, but this is FAPI, and that's how rebuild works... I don't think this is appropriate to add in a conversion issue when there is a pre-existing way forward.
And if you do want to add it, either here or otherwise, please add unit tests.
Comment #29
tim.plunkettOkay, @dawehner and I talked about this more and it does make some sense to support this.
I added unit test coverage, and cleaned up the form.
Comment #31
tim.plunkettA custom redirect should not override a custom response.
Comment #32
ParisLiakos CreditAttribution: ParisLiakos commentedagreed! thanks a lot tim :)
Comment #33
alexpottThe comment looks strange since a redirect would actually override a $form_state['response'] value.
Comment #34
tim.plunkettTentatively setting back to RTBC, but assigning to @alexpott to check if this new comment is more adequate.
Comment #36
tim.plunkett34: form-1978924-34.patch queued for re-testing.
Comment #38
Gábor HojtsyPatch passed, fix to tim.plunkett's status following d.o status bingo.
Comment #39
YesCT CreditAttribution: YesCT commentedI'm going to reroll this now. (it doesn't apply)
Comment #40
YesCT CreditAttribution: YesCT commentedthe hunk the patch was removing had a depreciated in it. so conflict due to #2187735: Add removal information to docblock of all @deprecated functions
I checked that was the only thing by diffing the hunk the patch took out and the hunk in head.
easy reroll.
no interdiff since this was a reroll.
removing needs reroll tag.
Comment #41
Gábor HojtsyShould still be RTBC then?
Comment #42
alexpottform-1978924-40.patch no longer applies.
Comment #43
vijaycs85Quick re-roll...
Comment #44
ParisLiakos CreditAttribution: ParisLiakos commentedback to alexpott
Comment #45
martin107 CreditAttribution: martin107 commentedcame here looking for rerolls
Comment #46
YesCT CreditAttribution: YesCT commented43: 1978924-locale_translate_export_form-43.patch queued for re-testing.
Comment #47
alexpottCommitted c53879f and pushed to 8.x. Thanks!
Fixed during commit.
Comment #48
Gábor HojtsyYay, this was a long time coming! :) Thanks all!
Comment #50
victor-shelepen CreditAttribution: victor-shelepen commentedIt is a pitty that it has been closed automaticaly due to no activity. It needs work. It seems RTBC.
Comment #51
victor-shelepen CreditAttribution: victor-shelepen commented43: 1978924-locale_translate_export_form-43.patch queued for re-testing.
Comment #53
victor-shelepen CreditAttribution: victor-shelepen commentedThis issue has been solved well. This comment confused me a little bit,
https://drupal.org/node/1978926#comment-8711483
Comment #54
YesCT CreditAttribution: YesCT commentedsetting back to status from #49