Comments

Assigned:Unassigned» Pancho
Status:Active» Needs review
StatusFileSize
new10.7 KB
PASSED: [[SimpleTest]]: [MySQL] 56,563 pass(es).
[ View ]

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

Issue tags:+D8MI, +WSCCI-conversion

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

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

This needs a reroll for FormBase

Issue tags:+D8MI, +WSCCI-conversion

Ugh.

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.

Status:Needs work» Needs review
StatusFileSize
new5.86 KB
new10.66 KB
FAILED: [[SimpleTest]]: [MySQL] 58,661 pass(es), 25 fail(s), and 8 exception(s).
[ View ]

FormBase and changes to Language class.

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new796 bytes
new10.68 KB
FAILED: [[SimpleTest]]: [MySQL] 59,174 pass(es), 0 fail(s), and 14 exception(s).
[ View ]

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.

Issue tags:+language-ui

Assigned:Pancho» Luxian

Status:Needs work» Needs review
StatusFileSize
new2.16 KB
new10.92 KB
PASSED: [[SimpleTest]]: [MySQL] 59,264 pass(es).
[ View ]

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

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.

Assigned:Luxian» Unassigned
Issue summary:View changes
Status:Needs work» Needs review
StatusFileSize
new10.9 KB
PASSED: [[SimpleTest]]: [MySQL] 60,363 pass(es).
[ View ]

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

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

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.

+++ 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

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.

indeed I'm. Sorry for the confusion.

StatusFileSize
new12.65 KB
PASSED: [[SimpleTest]]: [MySQL] 59,559 pass(es).
[ View ]
new3.97 KB

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

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?

StatusFileSize
new12.7 KB
PASSED: [[SimpleTest]]: [MySQL] 63,049 pass(es).
[ View ]
new1.14 KB

sure! :)

Status:Needs review» Reviewed & tested by the community

Great stuff. This looks ready.

Would this need a change record?

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.

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

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

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.

StatusFileSize
new9.4 KB
new16.76 KB
FAILED: [[SimpleTest]]: [MySQL] 63,333 pass(es), 23 fail(s), and 0 exception(s).
[ View ]

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.

Assigned:tim.plunkett» Unassigned
Status:Needs work» Needs review
StatusFileSize
new2.7 KB
new18.44 KB
PASSED: [[SimpleTest]]: [MySQL] 63,679 pass(es).
[ View ]

A custom redirect should not override a custom response.

Status:Needs review» Reviewed & tested by the community

agreed! thanks a lot tim :)

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.

Assigned:Unassigned» alexpott
Status:Needs work» Reviewed & tested by the community
StatusFileSize
new18.52 KB
PASSED: [[SimpleTest]]: [MySQL] 64,359 pass(es).
[ View ]
new1.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.

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

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

Status:Needs work» Reviewed & tested by the community

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

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

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

Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new18.58 KB
PASSED: [[SimpleTest]]: [MySQL] 64,598 pass(es).
[ View ]

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.

Status:Needs review» Reviewed & tested by the community

Should still be RTBC then?

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

Status:Needs work» Needs review
StatusFileSize
new18.55 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,095 pass(es).
[ View ]

Quick re-roll...

Status:Needs review» Reviewed & tested by the community

back to alexpott

Issue tags:-Needs reroll

came here looking for rerolls

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.

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

Status:Fixed» Closed (fixed)

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