Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Pancho’s picture

Assigned: Unassigned » Pancho
Status: Active » Needs review
FileSize
10.7 KB

Here's the conversion patch, tested to some degree, now let's see if our testbot agrees.

Pancho’s picture

Issue tags: +D8MI, +WSCCI-conversion

Awsome - the testbot liked it, too.
So please review now. I'm open for any suggestions.

tim.plunkett’s picture

Status: Needs review » Needs work
Issue tags: -D8MI, -WSCCI-conversion

This needs a reroll for FormBase

tim.plunkett’s picture

Issue tags: +D8MI, +WSCCI-conversion

Ugh.

xjm’s picture

Thanks 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.

disasm’s picture

Status: Needs work » Needs review
FileSize
5.86 KB
10.66 KB

FormBase and changes to Language class.

Status: Needs review » Needs work

The last submitted patch, drupal8.locale-module.1978924-6.patch, failed testing.

disasm’s picture

Status: Needs work » Needs review
FileSize
796 bytes
10.68 KB

removing request from submitForm. Getting via getRequest() instead.

Status: Needs review » Needs work

The last submitted patch, drupal8.locale-module.1978924-8.patch, failed testing.

Gábor Hojtsy’s picture

Issue tags: +language-ui
Luxian’s picture

Assigned: Pancho » Luxian
Luxian’s picture

Status: Needs work » Needs review
FileSize
2.16 KB
10.92 KB

Done

  • Re-rolled patch against lates 8.x
  • Updated Language::$langcode to Language::$id
  • Update locale.routing.yml to use _form instead of context

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

pfrenssen’s picture

Status: Needs review » Needs work
+++ b/core/modules/locale/lib/Drupal/locale/Form/ExportForm.phpundefined
@@ -0,0 +1,155 @@
+use Drupal\Core\Form\FormBase;
+use Drupal\Core\Language\Language;
+use Drupal\locale\PoDatabaseReader;
+use Drupal\Component\Gettext\PoStreamWriter;

These should be ordered alphabetically, with Drupal namespaces at the top, followed by vendors.

+++ b/core/modules/locale/lib/Drupal/locale/Form/ExportForm.phpundefined
@@ -0,0 +1,155 @@
+use Symfony\Component\HttpFoundation\Request;

This namespace doesn't seem to be used in the class.

+++ b/core/modules/locale/lib/Drupal/locale/Form/ExportForm.phpundefined
@@ -0,0 +1,155 @@
+/**
+ * {@inheritdoc}
+ */
+class ExportForm extends FormBase {

Classes do not inherit their documentation, as they do something different than their parent class.

+++ b/core/modules/locale/lib/Drupal/locale/Form/ExportForm.phpundefined
@@ -0,0 +1,155 @@
+      $response->prepare($request)
+        ->send();
+
+    }

Remove the empty line.

vaibhavjain’s picture

Assigned: Luxian » Unassigned
Issue summary: View changes
Status: Needs work » Needs review
FileSize
10.9 KB

Adding a new patch, changes defined in #13 incorporated.

Crell’s picture

+++ b/core/modules/locale/lib/Drupal/locale/Form/ExportForm.php
@@ -0,0 +1,153 @@
+      $response = new BinaryFileResponse($uri);
+      $response->setContentDisposition('attachment', $filename);
+      $response->prepare($request)
+        ->send();

No. 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.

tim.plunkett’s picture

sendResponse 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.

vijaycs85’s picture

+++ b/core/modules/locale/lib/Drupal/locale/Form/ExportForm.php
@@ -0,0 +1,153 @@
+      drupal_set_message('Nothing to export.');

no more drupal_set_message(). should be replaced by #title

tim.plunkett’s picture

no more drupal_set_message(). should be replaced by #title

@vijaycs85 are you confusing that with drupal_set_title? we have no replacement for dsm yet.

vijaycs85’s picture

indeed I'm. Sorry for the confusion.

ParisLiakos’s picture

well, 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

pfrenssen’s picture

I 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:

+++ b/core/lib/Drupal/Core/Form/FormBuilderInterface.php
@@ -114,6 +114,11 @@ public function getForm($form_arg);
+   *   - response: Used when a form needs to return some kind of response, eg
+   *     a \Symfony\Component\HttpFoundation\BinaryFileResponse when triggering
+   *     a file download. If you need to use a
+   *     \Symfony\Component\HttpFoundation\RedirectResponse please use the
+   *     redirect key instead.

Maybe mention this should be a Response object?

ParisLiakos’s picture

pfrenssen’s picture

Status: Needs review » Reviewed & tested by the community

Great stuff. This looks ready.

Would this need a change record?

webchick’s picture

Assigned: Unassigned » tim.plunkett
Status: Reviewed & tested by the community » Needs review

Since 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.

webchick’s picture

tim.plunkett’s picture

+++ b/core/lib/Drupal/Core/Form/FormBuilder.php
@@ -650,6 +650,12 @@ public function processForm($form_id, &$form, &$form_state) {
+        // If there is a response in form_state, respect that instead of doing
+        // a redirect.
+        if (isset($form_state['response']) && $form_state['response'] instanceof Response) {
+          return $form_state['response'];
+        }

+++ b/core/modules/locale/lib/Drupal/locale/Form/ExportForm.php
@@ -0,0 +1,150 @@
+      $response = new BinaryFileResponse($uri);
+      $response->setContentDisposition('attachment', $filename);
+      $form_state['response'] = $response;

This 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).

ParisLiakos’s picture

that 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

+      $response = new BinaryFileResponse($uri);
+      $response->setContentDisposition('attachment', $filename);
+      $form_state['response'] = $response;

than

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).

tim.plunkett’s picture

That'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.

tim.plunkett’s picture

Okay, @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.

Status: Needs review » Needs work

The last submitted patch, 29: language-1978924-29.patch, failed testing.

tim.plunkett’s picture

Assigned: tim.plunkett » Unassigned
Status: Needs work » Needs review
FileSize
2.7 KB
18.44 KB

A custom redirect should not override a custom response.

ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community

agreed! thanks a lot tim :)

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/lib/Drupal/Core/Form/FormBuilder.php
@@ -651,9 +651,14 @@ public function processForm($form_id, &$form, &$form_state) {
+        if ($redirect = $this->redirectForm($form_state)) {
+          $form_state['response'] = $redirect;
+        }
+
...
+        // a redirect.
+        if (isset($form_state['response']) && $form_state['response'] instanceof Response) {
+          return $form_state['response'];
         }

The comment looks strange since a redirect would actually override a $form_state['response'] value.

tim.plunkett’s picture

Assigned: Unassigned » alexpott
Status: Needs work » Reviewed & tested by the community
FileSize
18.52 KB
1.02 KB

Tentatively setting back to RTBC, but assigning to @alexpott to check if this new comment is more adequate.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 34: form-1978924-34.patch, failed testing.

tim.plunkett’s picture

34: form-1978924-34.patch queued for re-testing.

The last submitted patch, 34: form-1978924-34.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Reviewed & tested by the community

Patch passed, fix to tim.plunkett's status following d.o status bingo.

YesCT’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

I'm going to reroll this now. (it doesn't apply)

YesCT’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
18.58 KB

the 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.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Should still be RTBC then?

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

form-1978924-40.patch no longer applies.

error: patch failed: core/modules/locale/locale.bulk.inc:149
error: core/modules/locale/locale.bulk.inc: patch does not apply

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
18.55 KB

Quick re-roll...

ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community

back to alexpott

martin107’s picture

Issue tags: -Needs reroll

came here looking for rerolls

YesCT’s picture

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed c53879f and pushed to 8.x. Thanks!

diff --git a/core/modules/locale/locale.bulk.inc b/core/modules/locale/locale.bulk.inc
index 2bfe7fe..5f72ad8 100644
--- a/core/modules/locale/locale.bulk.inc
+++ b/core/modules/locale/locale.bulk.inc
@@ -7,7 +7,6 @@
 
 use Drupal\locale\Gettext;
 use Drupal\Core\Language\Language;
-use Symfony\Component\HttpFoundation\BinaryFileResponse;
 use Drupal\file\FileInterface;
 
 /**

Fixed during commit.

Gábor Hojtsy’s picture

Yay, this was a long time coming! :) Thanks all!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

victor-shelepen’s picture

Status: Closed (fixed) » Needs review

It is a pitty that it has been closed automaticaly due to no activity. It needs work. It seems RTBC.

victor-shelepen’s picture

Status: Needs review » Needs work

The last submitted patch, 43: 1978924-locale_translate_export_form-43.patch, failed testing.

victor-shelepen’s picture

Status: Needs work » Closed (works as designed)

This issue has been solved well. This comment confused me a little bit,
https://drupal.org/node/1978926#comment-8711483

+++ b/core/modules/locale/lib/Drupal/locale/Form/LocaleForm.php
....
@@ -22,16 +22,6 @@ public function import() {
-   * Wraps locale_translate_export_form().
-   *
-   * @todo Remove locale_translate_export_form().
-   */
-  public function export() {
-    module_load_include('bulk.inc', 'locale');
-    return drupal_get_form('locale_translate_export_form');
-  }
YesCT’s picture

Status: Closed (works as designed) » Closed (fixed)

setting back to status from #49