Contrib.social

Stop using getMainRequest() to build $form['#action']

Open on Drupal.org →
Created on 12 June 2015, about 9 years ago
Updated 17 February 2024, 7 months ago

Problem/Motivation

Quoting @dawehner from #2263569-244: Bypass form caching by default for forms using #ajax. → :

To be clear, using the master request is almost always semantically the wrong thing to do

We currently use the master request to populate $form['#action'] in \Drupal\Core\Form\FormBuilder::buildFormAction().

Using getCurrentRequest() caused test failures in \Drupal\system\Tests\Form\RedirectTest, but continuing to use the master request with <current> caused failures in \Drupal\system\Tests\System\AccessDeniedTest

This is due to conflicting use cases covered by RedirectTest and AccessDeniedTest. The latter asserts that people are sent to the original destination after a login while the former tests that it is possible for a form to set a redirect destination (even on exception pages).

The problem is that in HEAD the destination query parameter is used to pass the original location into the subrequest where the exception is rendered. However, the destination parameter always overrides any redirect set during form building. Usage of the master request in the FormBuilder does not really resolve that conflict but actually hides it.

Proposed resolution

Do not use the destination parameter in order to pass the original location to the subrequest but rather a deticated one (_exception_location).

Remaining tasks

Review.

User interface changes

N/A

API changes

TBD

Beta phase evaluation

<!--Uncomment the relevant rows for the issue. -->
📌 Task
Status

Needs work

Version

11.0 🔥

Component
Form  →

Last updated less than a minute ago

Created by

🇺🇸United States tim.plunkett Philadelphia

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • Comment 9 months ago →
    🇫🇷France andypost

    still needs CR and MR for 11.x

  • Open in Jenkins → Open on Drupal.org →
    Environment: php8.3_sqlite3.34
    last update 9 months ago
    30,771 pass
  • Merge request !5985Stop using getMainRequest() to build form action #2505339 → (Open) created by andypost
  • Comment 8 months ago →
    🇫🇷France andypost

    Needs only CR now

  • Pipeline finished with Success
    8 months ago
    Total: 512s
    #70036
  • Status changed to Needs review 8 months ago
  • Comment 8 months ago →
    🇫🇷France andypost

    Added CR draft which needs better wording

  • Status changed to Needs work 8 months ago
  • Comment 8 months ago →
    🇺🇸United States smustgrave

    Small change requests

  • First commit to issue fork.
  • Pipeline finished with Canceled
    8 months ago
    #70489
  • Status changed to Needs review 8 months ago
  • Comment 8 months ago →
    🇫🇷France andypost

    fixed

  • Pipeline finished with Success
    8 months ago
    Total: 498s
    #70492
  • Status changed to RTBC 8 months ago
  • Comment 8 months ago →
    🇧🇪Belgium BramDriesen Belgium 🇧🇪

    Code wise this looks good! All tests are passing 👍

    Also updated the readability of the CR a little bit. Not really sure if the CR needs more info or not.

    I think we can set this to RTBC now.

  • Status changed to Needs work 7 months ago
  • Comment 7 months ago →
    🇬🇧United Kingdom longwave UK

    I still don't think @alexpott or @dawehner's comments from #51 and #52 have been addressed. The CR doesn't really explain why you might be affected by this nor what you should do if you are, and the test coverage is very specific to the fix; it doesn't really prove anything about the original problem.

  • Pipeline finished with Failed
    about 1 month ago
    Total: 149s
    #246957
  • Pipeline finished with Success
    about 1 month ago
    Total: 859s
    #246963
contrib.social Blog FAQ Discussions
Production build 0.71.5 2024