Contrib.social

Confusing behavior with FormState::setFormState and FormState::setMethod

Open on Drupal.org →
Created on 20 January 2023, almost 2 years ago
Updated 14 January 2024, 11 months ago

Problem/Motivation

There is odd behavior/inconsistencies with the way the methods setFormState and setMethod work.

In the code

      $form_state->setFormState([
        'view' => $view,
        'display' => $view->display_handler->display,
        'exposed_form_plugin' => $view->display_handler->getPlugin('exposed_form'),
        'rerender' => TRUE,
        'method' => 'get',
        'no_redirect' => TRUE,
        'always_process' => TRUE,
      ]);

The check

$form_state->isMethodType('get') returns false because isMethodType($type) wraps $method_type in strtoupper() which will compare "get" === "GET".

However, using $form_state->setMethod('get') will cause $form_state->isMethodType('get') to evaluate TRUE.

I find this confusing because setting the method using setFormState(['method' => 'get']) caused a form to render with the post method, however using setMethod('get') worked. In my opinion, the behavior should be the same regardless of if someone is setting a full array of form state settings or if they are just setting the method.

Steps to reproduce

Create a form in code and set the form state as displayed above. The form will render as POST if the value in setFormState() is not uppercase.

Proposed resolution

I suggest either making the value checked against case insensitive, for example 'get' should equal 'GET'.

Perhaps adjusting

  /**
   * {@inheritdoc}
   */
  public function isMethodType($method_type) {
    return $this->method === strtoupper($method_type);
  }

to 

  /**
   * {@inheritdoc}
   */
  public function isMethodType($method_type) {
    return strtoupper($this->method) === strtoupper($method_type);
  }

I wouldn't recommend trying to change setFormState because of the way that it loops through each value in the array to set properties in the form state object.

🐛 Bug report
Status

Needs work

Version

11.0 🔥

Component
Form  →

Last updated 3 days ago

Created by

🇺🇸United States dorficus

Live updates comments and jobs are added and updated live.
  • Novice

    It would make a good project for someone who is new to the Drupal contribution process. It's preferred over Newbie.

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.

  • First commit to issue fork.
  • Comment almost 2 years ago →
    🇮🇳India reenaraghavan
  • Comment almost 2 years ago →
    🇺🇸United States volkswagenchick San Francisco Bay Area

    Hi @reenaraghavan I see that you have over 30 issue credits.
    It would benefit the community if Novice issues were reserved for novice contributors. Thanks.

  • Comment almost 2 years ago →
    🇮🇳India reenaraghavan

    Hi @volkswagenchick

    I have not contributed in Drupal core much, so just tried with a Novice.
    Will make sure not to pick Novice from next time onwards.

    Thanks

  • Comment almost 2 years ago →
    🇺🇸United States volkswagenchick San Francisco Bay Area

    Thanks for your understanding, when we let beginners learn, we all benefit.

  • First commit to issue fork.
  • Merge request !3502Issue #3335308: Confusing behavior with FormState::setFormState and FormState::setMethod → (Open) created by royalpinto007
  • Status changed to Needs review almost 2 years ago
  • Comment almost 2 years ago →
    🇮🇳India royalpinto007

    Previously, the isMethodType function in the HTTP Request class compared the HTTP method type case-sensitively. This made it difficult to match method types that were entered in a different case from the expected value. This commit updates the function to use strtoupper on both the $method_type parameter and the $method property, allowing for case-insensitive comparison.

    As a beginner, it was good to learn from the statements provided above in the problem/motivation section.

    Thanks!

  • Status changed to Needs work almost 2 years ago
  • Comment almost 2 years ago →
    🇺🇸United States smustgrave

    This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request → as a guide.

    As a bug this will need a test case

  • Comment about 1 year ago →
    🇮🇳India Abhijith S

    Abhijith S → made their first commit to this issue’s fork.

  • Status changed to Needs review about 1 year ago
  • Comment about 1 year ago →
    🇮🇳India Abhijith S
  • Status changed to Needs work about 1 year ago
  • Comment about 1 year ago →
    🇺🇸United States smustgrave

    MR needs updating to 11.x

  • Merge request !5912Update isMethodType to compare case-insensitively → (Open) created by Abhijith S
  • Status changed to Needs review about 1 year ago
  • Comment about 1 year ago →
    🇮🇳India Abhijith S

    MR updated to 11.x

  • Status changed to RTBC about 1 year ago
  • Comment about 1 year ago →
    🇺🇸United States smustgrave

    @Abhijith S this issue was tagged for novice, which are intended for new users. See you have over 19 pages of posts so believe you should be good to work on non novice issues.

    MR was actually very very behind, didn't have the code for gitlab so the pipeline wasn't running.

    There were 2 failures:
1) Drupal\Tests\Core\Form\FormStateTest::testIsMethodType with data set #0 ('get', 'get', true)
Failed asserting that false is identical to true.
/builds/issue/drupal-3335308/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:121
/builds/issue/drupal-3335308/vendor/phpunit/phpunit/src/Framework/Constraint/IsIdentical.php:79
/builds/issue/drupal-3335308/core/tests/Drupal/Tests/Core/Form/FormStateTest.php:334
/builds/issue/drupal-3335308/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
/builds/issue/drupal-3335308/vendor/phpunit/phpunit/src/Framework/TestSuite.php:684
/builds/issue/drupal-3335308/vendor/phpunit/phpunit/src/Framework/TestSuite.php:684
/builds/issue/drupal-3335308/vendor/phpunit/phpunit/src/TextUI/TestRunner.php:651
/builds/issue/drupal-3335308/vendor/phpunit/phpunit/src/TextUI/Command.php:144
/builds/issue/drupal-3335308/vendor/phpunit/phpunit/src/TextUI/Command.php:97
2) Drupal\Tests\Core\Form\FormStateTest::testIsMethodType with data set #1 ('get', 'GET', true)
Failed asserting that false is identical to true.
/builds/issue/drupal-3335308/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:121
/builds/issue/drupal-3335308/vendor/phpunit/phpunit/src/Framework/Constraint/IsIdentical.php:79
/builds/issue/drupal-3335308/core/tests/Drupal/Tests/Core/Form/FormStateTest.php:334
/builds/issue/drupal-3335308/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
/builds/issue/drupal-3335308/vendor/phpunit/phpunit/src/Framework/TestSuite.php:684
/builds/issue/drupal-3335308/vendor/phpunit/phpunit/src/Framework/TestSuite.php:684
/builds/issue/drupal-3335308/vendor/phpunit/phpunit/src/TextUI/TestRunner.php:651
/builds/issue/drupal-3335308/vendor/phpunit/phpunit/src/TextUI/Command.php:144
/builds/issue/drupal-3335308/vendor/phpunit/phpunit/src/TextUI/Command.php:97
FAILURES!
Tests: 46, Assertions: 69, Failures: 2.

    Test coverage is there and change makes sense. Even if just an extra precaution.

  • Open on Drupal.org →
    Environment: PHP 8.1 & MySQL 5.7
    last update about 1 year ago
    Not currently mergeable.
  • Open on Drupal.org →
    Environment: PHP 8.1 & MySQL 5.7
    last update 12 months ago
    Not currently mergeable.
  • Open on Drupal.org →
    Environment: PHP 8.1 & MySQL 5.7
    last update 12 months ago
    Not currently mergeable.
  • Open on Drupal.org →
    Environment: PHP 8.1 & MySQL 5.7
    last update 12 months ago
    Not currently mergeable.
  • Open on Drupal.org →
    Environment: PHP 8.1 & MySQL 5.7
    last update 12 months ago
    Not currently mergeable.
  • Open on Drupal.org →
    Environment: PHP 8.1 & MySQL 5.7
    last update 12 months ago
    Not currently mergeable.
  • Open on Drupal.org →
    Environment: PHP 8.1 & MySQL 5.7
    last update 12 months ago
    Not currently mergeable.
  • Open on Drupal.org →
    Environment: PHP 8.1 & MySQL 5.7
    last update 12 months ago
    Not currently mergeable.
  • Open on Drupal.org →
    Environment: PHP 8.1 & MySQL 5.7
    last update 12 months ago
    Not currently mergeable.
  • Open on Drupal.org →
    Environment: PHP 8.1 & MySQL 5.7
    last update 12 months ago
    Not currently mergeable.
  • Status changed to Needs work 11 months ago
  • Comment 11 months ago →
    needs-review-queue-bot

    The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

    This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

    Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

contrib.social Blog FAQ Discussions
Production build 0.71.5 2024