AccessManager should (once again) bail out early to save resources and avoid adding cacheable metadata for no reason

Deliberately not adding a test yet to see what the test suite has to say with this change.

Only remaining failure is MediaEntityMetadataApiTest, which uses the route ckeditor5.media_entity_metadata, which has:

name: ckeditor5.media_entity_metadata
path: '/ckeditor5/{editor}/media-entity-metadata'
defaults:
  _controller: '\Drupal\ckeditor5\Controller\CKEditor5MediaController::mediaEntityMetadata'
requirements:
  _entity_access: editor.use
  _custom_access: '\Drupal\ckeditor5\Controller\CKEditor5MediaController::access'
  _csrf_token: 'TRUE'
options:
  parameters:
    editor:
      type: 'entity:editor'
      converter: paramconverter.entity
  _access_checks:
    - access_check.entity
    - access_check.custom
    - access_check.csrf

Now, CKEditor5MediaController::access() used to throw a BadRequestException as follows:

    $uuid = $request->query->get('uuid');
    if (!$uuid || !Uuid::isValid($uuid)) {
      throw new BadRequestHttpException();
    }

So this piece of test code expected that, but the comment above was misleading.

    // Ensure that users that don't have access to the filter format receive
    // either 404 or 403.
    $this->drupalLogout();
    $token = $this->container->get('csrf_token')->get(ltrim($path, '/'));
    $this->drupalGet($path, ['token' => $token]);
    $this->assertSession()->statusCodeEquals(400);

But that no longer happens because entity_access: editor.use fails before the access check, leading to the other one never failing on the absence of the UUID. Whether that's desired is up for debate, but I'd say it's better to 403 and not run extra access checks than to run ones that don't matter and show their 400 status codes.

The fix is to always assert 403, as that's what's being tested here according to the comment.

See FilterFormatAccessControlHandler.php:

    // All users are allowed to use the fallback filter.
    if ($operation == 'use') {
      if ($filter_format->isFallbackFormat()) {
        return AccessResult::allowed();
      }
      else {
        return AccessResult::allowedIfHasPermission($account, $filter_format->getPermissionName());
      }
    }

In the test the anonymous user does not have the permission and the filter is not the fallback, so 403 is returned and the other checks no longer run with the MR here. The test already checks for a missing or malformed UUID returning 400 when the entity access check does go through, so there is nothing else to be done here.

Deleted because it was a red herring

The performance gain comes from this route:

name: field_ui.display_mode
path: /admin/structure/display-modes
defaults:
  _controller: '\Drupal\system\Controller\SystemController::systemAdminMenuBlockPage'
  _title: 'Display modes'
requirements:
  _permission: 'administer display modes'
  _access_admin_menu_block_page: 'TRUE'
options:
  _admin_route: true
  _access_checks:
    - access_check.permission
    - access_check.admin_menu_block_page

That second check no longer runs in OpenTelemetryPerformanceTest::doTestNodePageAdministrator() because it does not assign the "administer display modes" permission, implicitly proving that this MR works and improves performance.

11.x MR created

I didn't touch that code at all o.O Even on a rerun it goes red.

Also goes red in the daily 11.x tests, so unrelated to this issue.

https://git.drupalcode.org/project/drupal/-/pipelines/989488

Yeah it's fine. I tested it and it works out well.

The idea is that you do not need cacheable metadata of checks that will never happen, and that's exactly what we're doing here. If a check happens and fails, then the cacheable metadata needs to carry onto the next check, which also happens in the MR here.