Problem/Motivation

Drupal\Core\PathProcessor\PathProcessorFront uses an array_replace to merge query parameters from the inbound path into the frontpage path. However, the $parameters variable is not assigned again, therefore any parameters passed in are lost.

Steps to reproduce

1. Install standard
2. Go to /admin/config/system/site-information and set the frontpage to /admin/people?status=2
3. Visit / - Notice the view is filtered by Status=Blocked (expected)
4. Visit /?status=1 - Notice it's still filtered by Status= Blocked (bug)
5. Visit /?user=foo - Notice no value added in the "Name or email contains" filter (bug)

Proposed resolution

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

Remaining tasks

Write tests

User interface changes

None

Introduced terminology

None

API changes

None

Data model changes

None

Release notes snippet

N/A

CommentFileSizeAuthor
#25 3126761-nr-bot_13wtnjh9.txt91 bytesneeds-review-queue-bot
#11 remove-merge-query-attempt-3126761-11.patch2.25 KBpuddyglum
#9 remove-merge-query-attempt-3126761-9.patch864 bytespuddyglum
#4 set-parameters-3126761-4.patch726 bytespuddyglum
#3 set-parameters-3126761-3.patch607 bytespuddyglum
#2 set-parameters-3126761-2.patch607 bytespuddyglum

Issue fork drupal-3126761

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

jmonkfish created an issue. See original summary.

puddyglum’s picture

puddyglum
Primary language English
Location Elk Grove, CA
commented
StatusFileSize
new set-parameters-3126761-2.patch607 bytes

It makes sense to fix the missing assignment.

puddyglum’s picture

puddyglum
Primary language English
Location Elk Grove, CA
commented
StatusFileSize
new set-parameters-3126761-3.patch607 bytes

Retry patch without trailing whitespace.

puddyglum’s picture

puddyglum
Primary language English
Location Elk Grove, CA
commented
StatusFileSize
new set-parameters-3126761-4.patch726 bytes

Retrying again, unsure what the whitespace truly is.

puddyglum’s picture

puddyglum
Primary language English
Location Elk Grove, CA
commented
Status: Active » Needs review
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Issue tags: +Needs tests

This looks like it could do with a test to exercise this behaviour.

puddyglum’s picture

puddyglum
Primary language English
Location Elk Grove, CA
commented
Issue summary: View changes

In developing a test for this, I found two additional issues introduced in #3000671: Frontpage with query parameter results in "Page not found". I've updated the issue description.

I think both issues can be resolved at the same time as they appear to be related to the handling of query parameters present in Default front page.

puddyglum’s picture

puddyglum
Primary language English
Location Elk Grove, CA
commented
Assigned: Unassigned » puddyglum

It looks like, from #3000671: Frontpage with query parameter results in "Page not found", the only effective change was setting the $path to be something else. The array_merge and subsequent $request->query->replace() did not create changes outside of the function. Working on creating a patch for this.

puddyglum’s picture

puddyglum
Primary language English
Location Elk Grove, CA
commented
Issue summary: View changes
StatusFileSize
new remove-merge-query-attempt-3126761-9.patch864 bytes

I'm not sure why the browser redirects to the path of the frontpage when there is a query string in it. For instance, if you set front page to /admin/people, it will stay at /. If you set front page to /admin/people?anything, it will redirect to /admin/people. I don't see anything in PathProcessor that is causing that.

Attached patch basically cleans up the code and removes the unnecessary processing of potential query parameters.

Status: Needs review » Needs work

The last submitted patch, 9: remove-merge-query-attempt-3126761-9.patch, failed testing. View results

puddyglum’s picture

puddyglum
Primary language English
Location Elk Grove, CA
commented
StatusFileSize
new remove-merge-query-attempt-3126761-11.patch2.25 KB

Removing the tests against $request->query->all(), which also only used a copy of the $request object. My apologies for not testing this myself before providing the patch.

To summarize this fix, it keeps the $path = $components['path']; fix from #3000671: Frontpage with query parameter results in "Page not found" which prevents Page not found when a user adds parameters. It removes the portion that was thought to add query parameters to the current request. It only changed a copy of the current request.

I would rather that it did change the current request, but after reviewing the code I feel that is outside the scope of the PathProcessor, and changing the request should be made in the Routing or Url systems.

puddyglum’s picture

puddyglum
Primary language English
Location Elk Grove, CA
commented
Status: Needs work » Needs review
puddyglum’s picture

puddyglum
Primary language English
Location Elk Grove, CA
commented

Should we add a validation on the Default front page field, so that adding query parameters is not allowed? For instance, with or without the fix, if you add a ? to the path, it will pass validationn, but frontpage will display Page not found. If you add ?anything to the path then it will redirect to the path.

Visiting / in the borwser with these Default front page values:
a. Value of /valid-path => allowed, 200, browser URL is /
b. Value of /valid-path? => allowed, 404, browser URL is /
c. Value of /valid-path?some-query => allowed, 200, browser URL is /valid-path

If we disallow query strings, users will not experience cases (b) or (c). I don't believe it creates any breaking changes either.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.2.x-dev

Drupal 8 is end-of-life as of November 17, 2021. There will not be further changes made to Drupal 8. Bugfixes are now made to the 9.3.x and higher branches only. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.15 was released on June 1st, 2022 and is the final full bugfix release for the Drupal 9.3.x series. Drupal 9.3.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.4.x-dev branch from now on, and new development or disruptive changes should be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.9 was released on December 7, 2022 and is the final full bugfix release for the Drupal 9.4.x series. Drupal 9.4.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.5.x-dev branch from now on, and new development or disruptive changes should be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
Issue tags: +Needs Review Queue Initiative, +Needs subsystem maintainer review

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!

Version: 9.5.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

acbramley’s picture

acbramley commented
Title: Unused array_replace in Frontpage PathProcessor » $parameters not reassigned with array_replace in Frontpage PathProcessor
Issue summary: View changes
Issue tags: +Bug Smash Initiative

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.

acbramley opened merge request !12667

acbramley’s picture

acbramley commented
Issue tags: -Needs tests
acbramley’s picture

acbramley commented
Status: Needs work » Needs review
needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3126761-nr-bot_13wtnjh9.txt91 bytes

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.

acbramley’s picture

acbramley commented
Status: Needs work » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs subsystem maintainer review

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!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed 4d349db and pushed to 11.x. Thanks!

There were conflicts with 11.2.x so did not backport.

Now that this issue is closed, please review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, please credit people who helped resolve this issue.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Fixed » Reviewed & tested by the community

Doesn't look like this got pushed.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Now that this issue is closed, please review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, please credit people who helped resolve this issue.

  • alexpott committed 4d349db9 on 11.x 
    Issue #3126761 by puddyglum, longwave, smustgrave, acbramley: $...

Status: Fixed » Closed (fixed)

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