Path has unnecessary query appended.

Got the same issue on 9.5.9 as well

I found that this issue happens due to this change at the core: https://www.drupal.org/project/drupal/issues/3277784 🐛 copyRawVariables should support default route parameters Fixed

Commit: https://git.drupalcode.org/project/drupal/-/commit/a894a04bac8f2cff2339b...

Thankyou for suggestion @vacho but unfortunately it doesn't work to me.

Please, review again if it was fixed for you, for my runs, also I am unsure if the test fails is related to this issue.

It works for me. Thank you so much.

@_renify_ nice to know that works for you. I think should be set as "Fixed" when is merged. So for now is "Needs review" if you can review the code, and the testing issues that pipelines returns after this you can set as "Review and Tested by comunity"

Is this anything to do with 🐛 Ajax state leaking to Views destination paths Fixed ?

I also ran into this issue updating from 9.3.5 -> 9.5.9. Also updated from PHP 7.4 -> 8.1

Re: comment #2, the core issue to resolve the regression 🐛 [regression] route defaults are now automatically route parameters Fixed , I think

About comment #11
@cilefen No, that's about "Route defaults that do not start with a leading _" turn as query parameters, Also I tested on Drupal 10, 11 and the fix in this issue with page_manager still happens, and the MR fixes.

About comment #13
@cilefen , is related yes! but it is not a regression, I mean a core code doesn't turn back. #3358402 is to fix a title issue related as you can see at MR https://git.drupalcode.org/project/drupal/-/merge_requests/3953/diffs.

Hi @_renify_ and @vacho

I've reviewed and tested both

• - Patch from comment #10 https://www.drupal.org/project/page_manager/issues/3362561#comment-15082650 🐛 Path has unnecessary query appended. Needs work
• - Merge request https://git.drupalcode.org/project/page_manager/-/merge_requests/13#note...

and they are basically the same and they both seem to work on the first look, but they cause problems with Page Manager pages to not work as expected and after debugging and looking into it I strongly believe (though I might be wrong as I do not understand the internals of Page Manager) that It is just breaking the PageManagerRoutes::alterRoutes() method (defined in https://git.drupalcode.org/project/page_manager/-/blob/8.x-4.0-rc2/src/R... ) and thus:

• - removes extra queries from the path
• - but also breaks at least the function of Page Manager pages with variant that has no selection criteria (thus should be used all the time)

I would say it is quite obvious why as it changes the Route defaults name of the overridden route name from overridden_route_name to _overridden_route_name on line $defaults['_overridden_route_name'] = $overridden_route_name; in https://git.drupalcode.org/project/page_manager/-/blob/8.x-4.0-rc2/src/R... but then it forgets to change the name when using it in VariantRouteFilter in 3 places:
- https://git.drupalcode.org/project/page_manager/-/blob/8.x-4.0-rc2/src/R...
- https://git.drupalcode.org/project/page_manager/-/blob/8.x-4.0-rc2/src/R...
- https://git.drupalcode.org/project/page_manager/-/blob/8.x-4.0-rc2/src/R...
when calling the $route->getDefault('overridden_route_name')

If I change it there to use _overridden_route_name the bad side effect I can observe is removed, but the intended fix is gone.

Here is a work in progress patch which tries to replace default by adding an underscore at the start but it is not complete since if we try to replace page_manager_page then the page manager access starts failing.

The reason for that failure is inside PageAccessCheck the routes defaults / parameters starting with underscore are not available.

Not sure if this is the correct approach to resolve the issue, but posting it here in case if it helps to proceed with this issue further.

The last submitted patch, 16: 3362561-15.patch, failed testing. View results →
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

This issue 🐛 copyRawVariables should support default route parameters Fixed changes a lot for the logic of this module.

So using default params that start with _ makes a lot of sense, although I had to rewrite logic for _page_access (route match does not fetch params that start with _ and are not a part of url).

The patch contains all the fixes including the tests update. Not sure how tests run if there are changes in the patch...

The last submitted patch, 18: 3362561-18.patch, failed testing. View results →
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

I think need to upload patch without tests, because old tests are performed

It will break all other contrib integration modules as it expect overrides without underscore

@andypost good call, maybe we should try to fix it from drupal core side, let's say follow up issue of 🐛 copyRawVariables should support default route parameters Fixed

yep, very probably at least for backward compatibility reason

The thing is that the core change is already in the core! #3277784

I think we need to refactor this into page_manager core as is the proposal, also WIP solutions are welcome.

There is a problem with #18 which is conflicting with pathauto here is how to reproduce it.

1. Create a user role and give them permission to edit a node without applying the patch
2. Check if they can edit that node
3. Now apply the patch and try to edit same node using that new role

An error will prevent them to save that node saying

Either the path '/node/' is invalid or you do not have access to it.

Need to check section src/Entity/PageAccessCheck.php

TODO:

Fix edge cases for PageAccessCheck.php

I added some lines to the existing patch (the part of the access check) that load the page manager page entity if it is not loaded already. This seems to fix the access issues in our usecase (language switch links).

This does feel a lot like a bandaid for a bigger issue though, more investigation would be nice. Especially by someone more familiar with the routing system and the way parameters are being upcast to objects.

I've tested the patch from #28 and reviewed it closely.

It is breaking page access for us on various pages and links (like breadcrumbs).

I agree with @L_VanDamme:

This does feel a lot like a bandaid for a bigger issue though, more investigation would be nice. Especially by someone more familiar with the routing system and the way parameters are being upcast to objects.

The /page_manager/src/Routing/PageManagerRoutes.php needs more work and testing.

I also doubt that the page_manager_page.view string should be changed to version with underscore _page_manager_page.view as it’s an access request and the change refers to something not existing.

This is the line change I'm talking about /page_manager/src/Routing/PageManagerRoutes.php

$requirements['_page_access'] = '_page_manager_page.view';

I've updated our bandaid with a fix that should hopefully fix the breadcrumb issues in general.

Patch #30 looks stable and I tested and works for my project. Please validate in deep

Also, please works over MR https://git.drupalcode.org/project/page_manager/-/merge_requests/13 to be able to take advantage of gitlab MR diffs and git log

Als needs works to fix testing https://www.drupal.org/pift-ci-job/2716580 →

Patch is breaking access check for node path alias when trying to update existing node

@sorlov

Patch is breaking access check for node path alias when trying to update existing node

Possible fixed this error - need to check.

The last submitted patch, 30: 3362561-30.patch, failed testing. View results →
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

The last submitted patch, 34: 3362561-34.patch, failed testing. View results →
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Fixed code_sniffers

The last submitted patch, 38: 3362561-38.patch, failed testing. View results →

I think we could move to Needs review

Also added a child issue https://www.drupal.org/project/page_manager/issues/3373026 🐛 Block Menus that set to render level 2 and below, do not render in page variants Content Needs review
As it seems the MR solution solve it.

I updated the MR to stop using RouteMatch parameters or request attributes as they are not reliable. Instead, we should use the Route defaults as we can be 100% sure they will be present and correct (as we set them ourselves).

This is needed because we have noticed very rare edge cases where the page entity could not be found in the RouteMatch parameters or request attributes. In our case this was most easily reproduced right after logging in with users that have a certain role. I did not debug what was actually causing this in code.

- Code looks good.
- Testing locally, works!!

So RTBC

I'm seeing this problem as well. Any word on when this might be merged into a release? Thanks!

Great work guys, seems to be working well, any thoughts when it will be merged?

Current merge request as a patch.

@#49 🐛 Path has unnecessary query appended. Needs work please elaborate on your additional patch.

@#48 🐛 Path has unnecessary query appended. Needs work thanks for providing a patch to the merge for others to better local testing on installed latest dev and better review. But just to let you know: you can simply add ".patch" on the end of the url of the latest merge while downloading to get a patch from latest merge.

Frist run patch review:

$ wget https://git.drupalcode.org/project/page_manager/-/merge_requests/13.patch
--2024-01-29 22:27:24--  https://git.drupalcode.org/project/page_manager/-/merge_requests/13.patch
Resolving git.drupalcode.org (git.drupalcode.org)... 146.75.122.217
Connecting to git.drupalcode.org (git.drupalcode.org)|146.75.122.217|:443... connected.
HTTP request sent, awaiting response... 200 OK
Length: unspecified [text/plain]
Saving to: '13.patch'

13.patch                                                 [ <=>                                                                                                                ]  42.46K  --.-KB/s    in 0.01s

2024-01-29 22:27:24 (3.01 MB/s) - '13.patch' saved [43479]

$ git apply -v 13.patch
Checking patch src/Routing/PageManagerRoutes.php...
Checking patch tests/src/Unit/PageManagerRoutesTest.php...
Checking patch tests/src/Functional/PageNodeSelectionTest.php...
Checking patch page_manager.services.yml...
Checking patch src/Entity/PageAccessCheck.php...
Checking patch src/EventSubscriber/RouteNameResponseSubscriber.php...
Checking patch src/EventSubscriber/RouteParamContext.php...
Checking patch src/Routing/PageManagerRoutes.php...
Checking patch src/Routing/VariantRouteFilter.php...
Checking patch tests/src/Functional/PageNodeSelectionTest.php...
Checking patch tests/src/Unit/PageManagerRoutesTest.php...
Checking patch tests/src/Unit/RouteNameResponseSubscriberTest.php...
Checking patch tests/src/Unit/VariantRouteFilterTest.php...
Checking patch src/Entity/PageAccessCheck.php...
Checking patch src/Entity/PageAccessCheck.php...
Checking patch tests/src/Unit/PageManagerRoutesTest.php...
Checking patch page_manager.post_update.php...
Checking patch page_manager.services.yml...
Checking patch src/Entity/PageAccessCheck.php...
Applied patch src/Routing/PageManagerRoutes.php cleanly.
Applied patch tests/src/Unit/PageManagerRoutesTest.php cleanly.
Applied patch tests/src/Functional/PageNodeSelectionTest.php cleanly.
Applied patch page_manager.services.yml cleanly.
Applied patch src/Entity/PageAccessCheck.php cleanly.
Applied patch src/EventSubscriber/RouteNameResponseSubscriber.php cleanly.
Applied patch src/EventSubscriber/RouteParamContext.php cleanly.
Applied patch src/Routing/PageManagerRoutes.php cleanly.
Applied patch src/Routing/VariantRouteFilter.php cleanly.
Applied patch tests/src/Functional/PageNodeSelectionTest.php cleanly.
Applied patch tests/src/Unit/PageManagerRoutesTest.php cleanly.
Applied patch tests/src/Unit/RouteNameResponseSubscriberTest.php cleanly.
Applied patch tests/src/Unit/VariantRouteFilterTest.php cleanly.
Applied patch src/Entity/PageAccessCheck.php cleanly.
Applied patch src/Entity/PageAccessCheck.php cleanly.
Applied patch tests/src/Unit/PageManagerRoutesTest.php cleanly.
Applied patch page_manager.post_update.php cleanly.
Applied patch page_manager.services.yml cleanly.
Applied patch src/Entity/PageAccessCheck.php cleanly.

After applying the merge 13 from #46 🐛 Path has unnecessary query appended. Needs work cleanly wihtout flaws the following frontpage error occured on latest Drupal 10.2 stable release causing a WSOD:

The website encountered an unexpected error. Try again later.

ArgumentCountError: Too few arguments to function Drupal\page_manager\Entity\PageAccessCheck::__construct(), 0 passed in web/core/lib/Drupal/Component/DependencyInjection/Container.php on line 259 and exactly 1 expected in Drupal\page_manager\Entity\PageAccessCheck->__construct() (line 31 of modules/contrib/page_manager/src/Entity/PageAccessCheck.php).

Drupal\Component\DependencyInjection\Container->createService() (Line: 177)
Drupal\Component\DependencyInjection\Container->get() (Line: 100)
Drupal\Core\Access\CheckProvider->loadCheck() (Line: 157)
Drupal\Core\Access\AccessManager->performCheck() (Line: 136)
Drupal\Core\Access\AccessManager->check() (Line: 113)
Drupal\Core\Access\AccessManager->checkRequest() (Line: 107)
Drupal\Core\Routing\AccessAwareRouter->checkAccess() (Line: 92)
Drupal\Core\Routing\AccessAwareRouter->matchRequest() (Line: 105)
Symfony\Component\HttpKernel\EventListener\RouterListener->onKernelRequest()
call_user_func() (Line: 111)
Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher->dispatch() (Line: 157)
Symfony\Component\HttpKernel\HttpKernel->handleRaw() (Line: 76)
Symfony\Component\HttpKernel\HttpKernel->handle() (Line: 58)
Drupal\Core\StackMiddleware\Session->handle() (Line: 48)
Drupal\Core\StackMiddleware\KernelPreHandle->handle() (Line: 28)
Drupal\Core\StackMiddleware\ContentLength->handle() (Line: 32)
Drupal\big_pipe\StackMiddleware\ContentLength->handle() (Line: 53)
Asm89\Stack\Cors->handle() (Line: 48)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle() (Line: 51)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle() (Line: 36)
Drupal\Core\StackMiddleware\AjaxPageState->handle() (Line: 51)
Drupal\Core\StackMiddleware\StackedHttpKernel->handle() (Line: 704)
Drupal\Core\DrupalKernel->handle() (Line: 19)

After removing the applied patch and reverting to latest page_manager dev site operates again.

But just to let you know: you can simply add ".patch" on the end of the url of the latest merge while downloading to get a patch from latest merge.

If someone makes a commit to the issue fork, such a patch will change. That is no good for a Composer-based patching workflow.

Thanks for you rcoming back on this! +1

If someone makes a commit to the issue fork, such a patch will change.

Yes, the number will change. So the merge 13 would change to merge 14 on the next one. But you can keep your path to the merge number 13 which will not change. Apart from that each merge would require then a new patch in the comments, which clutters the issue queue and would confuse users to keep uptodate on latest patch and reviews

@dqd I'm not sure I understand. Can you give an example of that URL that would be the equivalent of the patch in #48?

I have an issue with access conditions related to https://www.drupal.org/project/page_manager/issues/2837833 🐛 'Required contexts without a value: node' when editing menu Needs work . The issue appeared after update #43 update. Fixed on RouteMatch with a separate patch.

#55 works for Drupal 10.2.6 / Page manager4.0@RC

mmm #48 say is a patch from MR, but it isn't https://www.drupal.org/project/page_manager/issues/3362561#comment-15339088 🐛 Path has unnecessary query appended. Needs work

and now we have some enhacens from #48 that is out of current MR, needs to get compatible enhances into MR.

japerry → changed the visibility of the branch 8.x-4.x to hidden.

Please don't make patches as it makes things very confusing to review. I -think- the MR is the most updated, running tests now that head is passing on D9, 10, and 11. It'd be good if someone in this issue can also bring in the MR to verify it is working for them, or better yet, make a test that verifies what this is supposed to fix.

Tests are passing but would want some further validation on the MR before its merged.