Instead of injecting request service, rather use the Symfony 2.4 request_stack in FormBuilder.

CommentFileSizeAuthor
#14 2225605-use-request-stack-in-form-builder-14.diff10.89 KBznerol
#13 interdiff.txt963 bytesznerol
#13 2225605-use-request-stack-in-form-builder-13.diff10.94 KBznerol
#11 2225605-use-request-stack-in-form-builder-11.diff11 KBznerol
#8 interdiff.txt856 bytesznerol
#8 2225605-use-request-stack-in-form-builder-8.diff11.84 KBznerol
#5 interdiff.txt478 bytesznerol
#5 2225605-use-request-stack-in-form-builder-5.diff11 KBznerol
#1 2225605-use-request-stack-in-form-builder.diff12.03 KBznerol

Comments

znerol’s picture

znerol commented
Status: Active » Needs review
StatusFileSize
new 2225605-use-request-stack-in-form-builder.diff12.03 KB
znerol’s picture

znerol commented
Parent issue: » #2223189: Use regular upstream HttpKernel instead of Drupal's custom
Related issues: -#2223189: Use regular upstream HttpKernel instead of Drupal's custom

Status: Needs review » Needs work

The last submitted patch, 1: 2225605-use-request-stack-in-form-builder.diff, failed testing.

znerol’s picture

znerol commented
Status: Needs work » Postponed

postponing this until we got rid of the request service injected into the url generator.

znerol’s picture

znerol commented
StatusFileSize
new 2225605-use-request-stack-in-form-builder-5.diff11 KB
new interdiff.txt478 bytes

Reroll after #2229223: RequestStack is not persisted across kernel rebuilds. Also push the request stack in core/authorize.php.

I cannot reproduce the test failures from #1 locally, so perhaps we're lucky and the test-bot also agrees.

znerol’s picture

znerol commented
Status: Postponed » Needs review

Status: Needs review » Needs work

The last submitted patch, 5: 2225605-use-request-stack-in-form-builder-5.diff, failed testing.

znerol’s picture

znerol commented
Status: Needs work » Needs review
StatusFileSize
new 2225605-use-request-stack-in-form-builder-8.diff11.84 KB
new interdiff.txt856 bytes

Ok, seems like unit-tests are failing, let's try the trick from #2229223: RequestStack is not persisted across kernel rebuilds

Update, this is the trick from #2228341-78: Objectify session management functions + remove session.inc to make DUTB pass.

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Reviewed & tested by the community

perfect!

znerol’s picture

znerol commented
Status: Reviewed & tested by the community » Postponed

Because the fix introduced in #8 is actually taken from #2228341: Objectify session management functions + remove session.inc, I'd like to postpone this on the session.inc issue. The latter is way more important and I'd rather avoid getting in the way of it.

znerol’s picture

znerol commented
Status: Postponed » Needs review
StatusFileSize
new 2225605-use-request-stack-in-form-builder-11.diff11 KB

This is not blocked anymore, reroll.

dawehner’s picture

dawehner
Primary language German
commented 
+++ b/core/lib/Drupal/Core/Form/FormBuilder.php
@@ -1240,7 +1248,10 @@ public function setErrorByName($name, array &$form_state, $message = '') {
+    $request = $this->requestStack->getCurrentRequest();
+    if ($request) {

@@ -1254,7 +1265,10 @@ public function getErrors(array $form_state) {
+    $request = $this->requestStack->getCurrentRequest();
+    if ($request) {

We could merge the assignment and the if into one line. btw. what is the reason we do need this here? Maybe we should document that and otherwise return FALSE in the later function.

znerol’s picture

znerol commented
StatusFileSize
new 2225605-use-request-stack-in-form-builder-13.diff10.94 KB
new interdiff.txt963 bytes

Re #12: I think that the condition was necessary before when neither installer nor update.php cared to populate the request-stack. Meanwhile we have it around anywhere, therefore they can be removed I guess.

znerol’s picture

znerol commented
StatusFileSize
new 2225605-use-request-stack-in-form-builder-14.diff10.89 KB

Reroll after #2079797: Provide a trait for $this->t() and $this->formatPlural()

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Reviewed & tested by the community

Perfect!

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Component: base system » forms system

Didn't see this (wrong component!) but it looks good.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented

This also unblocks my work in #2209977: Move form validation logic out of FormBuilder into a new class

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed 
+++ b/core/core.services.yml
@@ -122,9 +122,7 @@ services:
-    calls:

+++ b/core/scripts/run-tests.sh
@@ -389,6 +389,7 @@ function simpletest_script_bootstrap() {
   $container->set('request', $request, 'request');
+  $container->get('request_stack')->push($request);

It seems like it's super easy to forget that new line since one might reasonably expect that when you change something in the request, it'd handle pushing it onto the stack for you. Asked Tim about this and he noted that drupal_handle_request() does that for you in most instances, but run-tests.sh and authorize.php are different entry points that boot a slimmed down Drupal so they need extra handling.

Apart from that, this looks pretty straight-forward, soooo...

Committed and pushed to 8.x. Thanks!

  • Commit 340a0fa on 8.x by webchick: 
    Issue #2225605 by znerol: Use request stack in form builder.

Status: Fixed » Closed (fixed)

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