Simplified custom access checker does not have access to Request

Yeah for some reason (no idea why) \Drupal\Core\Access\CustomAccessCheck::access just passes along the $route_match as well as $request.

Here is a bugfix for it.

Rebase of #5

Hi, I can confirm this patch works for Drupal 8.5.5.

Funny to be learning something new, run into errors and discover it's a bug to be solved in Drupal 8.7 ;D
Why not 8.6 ?
Note, I didn't run the test, don't know why it's failing in #9 ?

Why not 8.6 ?

All the bugs are fixed first in the development version and later if necessary/possible are backported to the stable version.

So, 8.6 is almost out (The first alpha has been already released) so any new fix first will be commited to 8.7 and later can be backported to 8.6.

I will run again the tests on #9 to see if it was a random error.

Anytime I navigate to /user/# I get the error "The website encountered an unexpected error. Please try again later.".
Watchdog log shows the following error :
RuntimeException: Callable "Drupal\Core\Access\CustomAccessCheck::access" requires a value for the "$request" argument. in Drupal\Component\Utility\ArgumentsResolver->handleUnresolvedArgument() (line 142 of /srv/bindings/37df6945aeee4c2cb52bb8fb431fc585/code/core/lib/Drupal/Component/Utility/ArgumentsResolver.php).

After applying the patch... still no luck :-( This should be related, if not the same, no?

Re-roll the patch from #09 for 8.7.x.

Here are my changes the following declaration (impossible to generate an interdiff see below):

1. Make the $request optional to prevent breaking the whole custom Access:

+   * @param \Symfony\Component\HttpFoundation\Request $request
+   *   Optional, a request. Only supply this parameter when checking the
+   *   incoming request.

-  public function access(Route $route, RouteMatchInterface $route_match, AccountInterface $account) {
+  public function access(Route $route, RouteMatchInterface $route_match, AccountInterface $account, Request $request = NULL) {

PS: I wasn't able to generate the interdiff file sorry guys ...

1 out of 2 hunks FAILED -- saving rejects to file /var/folders/5c/cwjk37zd5xz5qrknkqzfkhgr0000gn/T//interdiff-1.jIEori.rej
interdiff: Error applying patch1 to reconstructed file

Solves the problem for me. Luckily I found this post, I was trying to learn D8 routing, menu and access systems and this bug was driving me crazy for days! My problems are not completely solved. But getting a lot less WSODs now.

Does not apply to D8.8 though. I will try and re-roll an D8.9 version tomorrow

REstoring status

Hi,

I am attaching patch rolled again for 8.9.x and 9.1.x

Thanks,
Sadashiv.

Forgot to fix getMock deprecation, now fixed

Thanks,
Sadashiv.

Tested with my D8 project in which I encountered this bug and #25 works for me, thanks @sadashiv.
Will try and test with D9 some time soon.

Using Drupal 8.9.8 the patch in #25 applies cleanly.

If I use a simple route and access check similar to #1 such as

my_test.test_route:
  path: '/my_test'
  defaults:
    _controller: '\Drupal\my_test\Controller\TestController::test'
  requirements:
    _custom_access:  '\Drupal\my_test\Controller\TestController::access'

namespace Drupal\my_test\Controller;

use Drupal\Core\Access\AccessResult;
use Drupal\Core\Controller\ControllerBase;
use Drupal\Core\Routing\Access\AccessInterface;
use Symfony\Component\HttpFoundation\Request;

class TestController extends ControllerBase implements AccessInterface {

  public function access(Request $request) {
    return AccessResult::allowed();
  }

  public function test(Request $request) {
    return [
      '#type' => 'markup',
      '#title' => t('Testing'),
      '#markup' => $this->t('This is a test page'),
    ];
  }

}

then when I access the route directly $request is available in the access check so the patch looks to work correctly.

Now this is probably outside of the remit of this problem but it's sort of linked.

What should happen if the route is used as a tab on a node view page?

my_test.test_route: # The first plugin ID
  route_name: my_test.test_route
  title: 'My Test'
  base_route: entity.node.canonical
  appears_on:
    - entity.node.canonical
  options:
    parameters:
      node: 'entity:node'

If I do this then when viewing the node view/edit page the access check is called but $request is missing again.

Should $request be available on these pages, is this caused by something else?

Is good for D8.

#28 looks relevant are we missing something passing request when checking access to a route for a tab?

From my understanding this is already the case for any other access checker on local tasks, see https://git.drupalcode.org/project/drupal/-/blob/9.2.x/core/lib/Drupal/C...

This code is calling \Drupal\Core\Access\AccessManager::checkNamedRoute, which sets $request as null.

Maybe the documentation could be better:

+   *   Optional, a request. The parameter is not always provided. For example checking access for local tasks will not have the request object available.

Fixed the custom command fail issue in patch #25

Needs a re-roll, and could also use the comment improvement from #31.

please review the patch.

Rerolling the patch with all changes.

Thanks,
Sadashiv.

Retrying with minor changes

Pretty sure this is a soft merge failure caused by #3217717: Replace usages of the at() matcher, which is deprecated. There are now two calls to controllerResolved->expect($this->exactly(3)) which I assume means there are suppose to be... 6? I need to dig into this test more but mocks are kinda inscrutable so it might take a few.

yeah, was definitely that issue but I didn't notice the patches here where still using the at() method which is why they weren't working.

This is a reroll of the RTBC'd #32 without the later changes + the at() changes in the interdiff.

Did a review and everything in this is pretty straightforward and looks good to go. Didn't really touch anything other then fixing the tests(feel free to adjust credit accordingly) but probably not appropriate to RTBC.

Touched up the summary into template and document why this is useful and other issue cleanup. Technically this is working by design just missing some very expected functionality so updating this to a feature as well.

The changes above have passed tests and I've reconfirmed that this patch applies cleanly and does its job. Look to be RTBC to me.

Womp womp. I'll try to take a look at this.

Tests seem to be failing due to #3255836: Test fails due to Composer 2.2. I guess we'll just wait until that's resolved. Not flipping back to RTBC until we can get those to pass.

New tests passed, marking this RTBC again.

This seems to be a random failure.

Drupal\Tests\layout_builder\FunctionalJavascript\* again

RTBC++

+1 RTBC

+1 RTBC. Hope the random test failures are resolved now.

Ouch, just ran into this DX pita. +1 for commit.

Back to RTBC.

Back to RTBC after more random test failures (#3269085: [random test failure] Random test fail in EntityAutocompleteTest).

Reroll 9.5.x-dev

Is there anything stopping this from getting committed? It has been RTBCed and rerolled for over 3 years.

I'm not really sure. While I remember _why_ I got here, but its been so long I honestly don't remember the code at all...

That said, the current patch looks the same as all the other RTBC patches and applies cleanly so putting it back to RTBC.

Doesn't apply to 11.x

Issue #68 is functional; it requires only minor adjustments. The patch has been updated for versions 10.x. However, it wasn't originally applied to 11.x, but after making the necessary changes in the 11.x branch, the patch is now successfully applied. As discussed earlier, the patch modifications resolve the issue effectively, so it functionality remains as is.

Restoring status after reroll

Committed/pushed to 11.x, thanks!

Fixing my credit.