Contrib.social

EntityCreateAccessCheck needs to enforce a bundle is set by the route for entity types that support bundles

Open on Drupal.org →
Created on 23 October 2017, over 7 years ago
Updated 8 May 2024, 12 months ago

EntityCreateAccessCheck provides a route access checker which can be used in two ways:

For an entity type that does not have bundles, you just give the entity type ID:

  requirements:
    _entity_create_access: 'menu'

For an entity type that does have bundles, you need to give the entity type ID and a bundle, which can be loaded from a route placeholder:

  requirements:
    _entity_create_access: 'taxonomy_term:{taxonomy_vocabulary}'

If you use it without the bundle, but with an entity type that does have bundles, nothing breaks, until a contrib modules implements hook_entity_create_access() and expects to find a $bundle parameter, and makes API calls that pass $bundle to APIs that expect it. Then everything breaks and it's really hard to debug. (For example, this problem caused a crash that was apparently in OG, but was caused by the incorrect use of this route access in Commerce - #2911542: "Add payment" and "Add order" routes need to use _entity_create_any_access, not _entity_create_access → .)

The use without the bundle value with an entity type that does have bundles is incorrect because:

1. The _entity_create_access router check is handled by EntityCreateAccessCheck, which calls:

    return $this->entityManager->getAccessControlHandler($entity_type)->createAccess($bundle, $account, [], TRUE);

2. EntityAccessControlHandlerInterface::createAccess() says:

   * @param string $entity_bundle
   *   (optional) The bundle of the entity. Required if the entity supports
   *   bundles, defaults to NULL otherwise.

I think EntityCreateAccessCheck should simply throw an exception if the bundle value isn't available for an entity type that has bundles.

🐛 Bug report
Status

Needs work

Version

11.0 🔥

Component
Entity  →

Last updated about 9 hours ago

Created by

🇬🇧United Kingdom joachim

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • Comment 12 months ago →
    🇺🇸United States thomas.fleming

    thomas.fleming → made their first commit to this issue’s fork.

  • Merge request !7981Convert patch to MR. → (Open) created by thomas.fleming
  • Pipeline finished with Failed
    12 months ago
    Total: 575s
    #167943
  • Status changed to Needs review 12 months ago
  • Comment 12 months ago →
    🇺🇸United States thomas.fleming

    I converted the patch to a merge request and also updated the call to entityManager to use entityTypeManager. Also confirmed that this is still an issue.

  • Pipeline finished with Failed
    12 months ago
    Total: 663s
    #168018
  • Status changed to Needs work 12 months ago
  • Comment 12 months ago →
    🇺🇸United States smustgrave

    Thanks for reviving this one and verifying still an issue

    Tagging for issue summary as it should follow the standard issue template.

    Also tagging for tests since the change appeared to break a number of tests.

    Thanks!

  • First commit to issue fork.
  • Pipeline finished with Canceled
    3 months ago
    Total: 141s
    #417695
  • Pipeline finished with Failed
    3 months ago
    Total: 432s
    #417698
  • Comment 3 months ago →
    🇪🇸Spain vidorado Logroño (La Rioja)

    I believe this is a significant change that could cause WSODs after an update. Instead of throwing an exception, we should trigger a deprecation error first.

    I've also fixed some failing tests caused by a core route that defined a route requirement for a menu_link_content entity—which has bundles—without specifying a bundle.

    Additionally, a change record has been issued. I will create a follow-up issue to replace the deprecation error with an \InvalidArgumentException.

  • Pipeline finished with Success
    3 months ago
    Total: 2092s
    #417777
  • Comment 2 months ago →
    🇺🇸United States smustgrave

    Can the CR be tweaked some. The code snippets are those suppose to be before/after?

  • Comment 2 months ago →
    🇪🇸Spain vidorado Logroño (La Rioja)

    I tried to clarify them a bit :)

    Thanks!

  • Comment 2 months ago →
    🇬🇧United Kingdom joachim

    > I believe this is a significant change that could cause WSODs after an update. Instead of throwing an exception, we should trigger a deprecation error first.

    Agreed, and if that's now the case, then this in the CR is wrong:

    > The former will no longer be permitted. An exception will now be thrown if attempted.

  • Comment 2 months ago →
    🇪🇸Spain vidorado Logroño (La Rioja)

    Ow, yes, you're right. I was thinking about Drupal 12, but the CR is for the Drupal 11.x minor version where this issue will be merged.

    Sorry, I'm yet a CR rookie! xD

  • Comment about 1 month ago →
    🇺🇸United States smustgrave

    Lets switch to a deprecation then :)

  • Comment about 1 month ago →
    🇪🇸Spain vidorado Logroño (La Rioja)

    The current solution is actually triggering a dreprecation! Check this

    It's ready to review :)

contrib.social Blog FAQ Discussions
Production build 0.71.5 2024