Contrib.social

Unused array_replace in Frontpage PathProcessor

Open on Drupal.org →
Created on 10 April 2020, over 5 years ago
Updated 26 January 2023, over 2 years ago

Issue discovery

During code quality scanning this line came into question:
core/lib/Drupal/Core/PathProcessor/PathProcessorFront.php Line 52
array_replace($parameters, $request->query->all());

      if (!empty($components['query'])) {
        parse_str($components['query'], $parameters);
        array_replace($parameters, $request->query->all());
        $request->query->replace($parameters);
      }

The entire line can be removed, or else $parameters can be assigned the return value of the call to array_replace. I believe that was the purpose, and would make sense to replace.

$parameters = array_replace($parameters, $request->query->all());

Unexpected frontpage behaviors

In developing a test for this, I found two additional issues introduced in #3000671: Frontpage with query parameter results in "Page not found" → :

  1. Browser redirects to URL path.

    When query parameters (GET vars) are present in Default front page URL, visiting / redirects the browser to the URL with the query removed.
    For example (from the related issue):

    • Default front page: /admin/people?status=2
    • Browser requests /, redirects to: /admin/people (path is present while query parameters are not).

    Expected same behavior as when no query parameters present in Default front page: browser should load /admin/people?status=2 while remaining at /.

  2. Query parameters missing.
    The query parameters from Default front page URL are not passed to the target path.
    For example (from the related issue):
    • Default front page: /admin/people?status=2
    • Browsers loading the / URL will display the People page from /admin/people, but the specified Status is Blocked filter (from ?status=2) is not applied.
    • Browsers loading the /?status=2 URL will be redirected to /admin/people?status=2 and display the People page with the Status is Blocked filter applied.
    • Browsers loading the /?status=1 URL will be redirected to /admin/people?status=1 and display the People page with the Status is Active filter applied.
🐛 Bug report
Status

Needs work

Version

9.5

Component
Routing  →

Last updated 12 days ago

Created by

🇺🇸United States puddyglum Elk Grove, CA

Live updates comments and jobs are added and updated live.
  • Needs tests

    The change is currently missing an automated test that fails when run with the original code, and succeeds when the bug has been fixed.

  • Needs subsystem maintainer review

    It is used to alert the maintainer(s) of a particular core subsystem that an issue significantly impacts their subsystem, and their signoff is needed (see the governance policy draft for more information). Also, if you use this tag, make sure the issue component is set to the correct subsystem. If an issue significantly impacts more than one subsystem, use needs framework manager review instead.

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

    Not sure I can answer #13 so tagging for subsystem review

    See this was also previously tagged for tests so that still needs to happen

    Thanks!

  • Comment 2 months ago →
    🇦🇺Australia acbramley

    Triaged as part of BSI and reproduced this bug manually, still an issue on 11.x. Some of the reported issues in the IS aren't reproducible though.

    Rewriting the IS with the standard template.

  • Merge request !12667Issue #3126761: Fix array_replace in PathProcessorFront → (Open) created by acbramley
  • Comment 2 months ago →
    🇦🇺Australia acbramley
  • Comment 2 months ago →
    🇦🇺Australia acbramley
  • Pipeline finished with Success
    2 months ago
    Total: 1078s
    #542672
  • Comment 19 days 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 necessarily 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.

  • Comment 16 days ago →
    🇦🇺Australia acbramley
  • Pipeline finished with Success
    16 days ago
    Total: 533s
    #586703
  • Comment 12 days ago →
    🇺🇸United States smustgrave

    Not sure why I tagged for sub-maintainer but that was when we just got NRQI off the ground.

    Ran the test-only feature here https://git.drupalcode.org/issue/drupal-3126761/-/jobs/6378866

    1) Drupal\Tests\Core\PathProcessor\PathProcessorFrontTest::testProcessInbound with data set "frontpage with query parameters and request query parameters" ('/node?example=muh', '/', '/node', ['muh', 'buh'], ['buh'])
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
     'example' => 'muh'
-    'example2' => 'buh'
 )
/builds/issue/drupal-3126761/core/tests/Drupal/Tests/Core/PathProcessor/PathProcessorFrontTest.php:41
2) Drupal\Tests\Core\PathProcessor\PathProcessorFrontTest::testProcessInbound with data set "frontpage with query parameters and replacement request query parameters" ('/node?example=muh', '/', '/node', ['cuh', 'buh'], ['cuh', 'buh'])
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
-    'example' => 'cuh'
-    'example2' => 'buh'
+    'example' => 'muh'
 )

    The actual fix core/lib/Drupal/Core/PathProcessor/PathProcessorFront.php makes complete sense.

    LGTM!

contrib.social Blog FAQ Discussions
Production build 0.71.5 2024