Updated: Comment #N

Problem/Motivation

Trying to use AccessManager::checkNamedRoute() for menu links, some are not correctly checked, e.g. the /search link because the route attributes are not correctly populated.

This is important since it's needed to make sure route-based access checks work correctly in #2047633: Move definition of menu links to hook_menu_link_defaults(), decouple key name from path, and make 'parent' explicit

Proposed resolution

Figure out how to properly populate all request attributes using just the paramConverterManager. This is in the current patch.

OR - as a fallback approach that may be less performant - this was in the 1st patch, but not what we are doing in the current patch:

Alter AccessManager::checkNamedRoute() so that is builds a complete route like menu_item_route_access() currently does by using the full matcher.

Remaining tasks

figure out the best code

User interface changes

none

API changes

none

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pwolanin’s picture

Status: Active » Needs review
FileSize
1.26 KB

Status: Needs review » Needs work

The last submitted patch, 1: 2181293-1.patch, failed testing.

dawehner’s picture

The obvious reason why we did not a full router request was to safe performance, but yeah I fear things aren't that simple. Do you know exactly why using the param converter did not worked as expected for search links?

dawehner’s picture

Before changing this it would be great to have some benchmarks as well

pwolanin’s picture

Fatal seems to be due to an incomplete mock object in a unit test

PHP Fatal error: Call to undefined method Mock_UrlGeneratorInterface_e59f861c::getPathFromRoute() in /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Access/AccessManager.php on line 202

pwolanin’s picture

Title: AccessManager::checkNamedRoute() is not building a complete route request » AccessManager::checkNamedRoute() is not passing all route defaults building a complete route request
Status: Needs work » Needs review
FileSize
853 bytes

Here's a much simpler patch that at least fixes search.view in menu links.

pwolanin’s picture

FileSize
853 bytes

Silly me - defaults should be 2nd, so we use any non-default params preferably.

dawehner’s picture

Issue tags: +D8MA
FileSize
5.06 KB

It is never as easy as you expect it to be

Crell’s picture

If #8 passes, it looks OK to me visually.

pwolanin’s picture

Title: AccessManager::checkNamedRoute() is not passing all route defaults building a complete route request » AccessManager::checkNamedRoute() is not passing all route defaults (or building a complete route request)
pwolanin’s picture

Priority: Normal » Major
Issue tags: +WSCCI
pwolanin’s picture

Status: Needs review » Reviewed & tested by the community

Green including added tests

dawehner’s picture

pwolanin’s picture

pwolanin’s picture

Issue summary: View changes
alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 83bf082 and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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