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
Comment | File | Size | Author |
---|---|---|---|
#8 | access_manager-2181293.patch | 5.06 KB | dawehner |
#7 | 2181293-7.patch | 853 bytes | pwolanin |
Comments
Comment #1
pwolanin CreditAttribution: pwolanin commentedThis change seems to solve some access problems in #2047633: Move definition of menu links to hook_menu_link_defaults(), decouple key name from path, and make 'parent' explicit
Comment #3
dawehnerThe 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?
Comment #4
dawehnerBefore changing this it would be great to have some benchmarks as well
Comment #5
pwolanin CreditAttribution: pwolanin commentedFatal 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
Comment #6
pwolanin CreditAttribution: pwolanin commentedHere's a much simpler patch that at least fixes search.view in menu links.
Comment #7
pwolanin CreditAttribution: pwolanin commentedSilly me - defaults should be 2nd, so we use any non-default params preferably.
Comment #8
dawehnerIt is never as easy as you expect it to be
Comment #9
Crell CreditAttribution: Crell commentedIf #8 passes, it looks OK to me visually.
Comment #10
pwolanin CreditAttribution: pwolanin commentedComment #11
pwolanin CreditAttribution: pwolanin commentedComment #12
pwolanin CreditAttribution: pwolanin commentedGreen including added tests
Comment #13
dawehnerGiven that this blocks #2047633: Move definition of menu links to hook_menu_link_defaults(), decouple key name from path, and make 'parent' explicit
Comment #14
pwolanin CreditAttribution: pwolanin commentedComment #15
pwolanin CreditAttribution: pwolanin commentedComment #16
alexpottCommitted 83bf082 and pushed to 8.x. Thanks!