Skip to content
Snippets Groups Projects

Convert patch to MR.

Closes #2918231

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
54 54 [$entity_type, $bundle] = explode(':', $route->getRequirement($this->requirementsKey) . ':');
55 55
56 56 // For an entity type that has bundles, the $bundle must be specified.
57 if (empty($bundle) && $this->entityTypeManager->getDefinition($entity_type)->hasKey('bundle')) {
57 $entity_type_definition = $this->entityTypeManager->getDefinition($entity_type);
  • @thomas.fleming Why you checked for $this->entityTypeManager->getDefinition() not returning NULL? Since the optional parameter $exception_on_invalid is TRUE by default, an exception will be thrown in that case. A null pointer will never happen.

  • Hey @vidorado probably irrelevant at this point, and apologize for not including better info on my reasoning for this. IIRC I added this check to ensure that the more helpful exception in EntityCreateAccessCheck was thrown when the replication steps were followed. Otherwise, you get the plugin not found exception, which seems out of context given the replication steps.

  • Hi @thomas.fleming!

    This is the modified code snippet:

    $entity_type_definition = $this->entityTypeManager->getDefinition($entity_type);
    if (empty($bundle) && $entity_type_definition && $entity_type_definition->hasKey('bundle')) {
      throw new \Exception("The _entity_create_access route access requires a bundle parameter because the $entity_type entity type uses bundles.");
    }

    All you added is a local var $entity_type_definition in order to be able to check it just in the if() below. But, as I see, $entity_type_definition will never be falsy at this point, since $this->entityTypeManager->getDefinition($entity_type) will throw a PluginNotFoundException exception if the $entity_type is invalid.

    As I see, you guard won't prevent PluginNotFoundException to be thrown, so I believe this change is not neccessary.

    Do you agree with me, and don't mind if I revert it, in that case? :smile:

    Thanks in advance!

    Edited by Víctor Dorado
  • I think this should be enough to prevent throwing PluginNotFoundException?

    Suggested change
    57 $entity_type_definition = $this->entityTypeManager->getDefinition($entity_type);
    57 $entity_type_definition = $this->entityTypeManager->getDefinition($entity_type, FALSE);
  • Please register or sign in to reply
  • Víctor Dorado added 1110 commits

    added 1110 commits

    Compare with previous version

  • added 1 commit

    • 9137bfce - Ensure an exception is thrown when no bundle is specified

    Compare with previous version

  • Víctor Dorado added 2 commits

    added 2 commits

    • 0c3f6b05 - Fix an _entity_create_access requirement not specifying the bundle in menu_link_content module
    • 794efdbd - Deprecate instead of throwing an Exception

    Compare with previous version

  • 53 53 public function access(Route $route, RouteMatchInterface $route_match, AccountInterface $account) {
    54 54 [$entity_type, $bundle] = explode(':', $route->getRequirement($this->requirementsKey) . ':');
    55 55
    56 // For an entity type that has bundles, the $bundle must be specified.
    57 $entity_type_definition = $this->entityTypeManager->getDefinition($entity_type);
    58 if (empty($bundle) && $entity_type_definition && $entity_type_definition->hasKey('bundle')) {
    59 @trigger_error(
    60 sprintf(
    61 'Defining a \'%s\' route requirement without a bundle for an entity type which has bundles (%s) is deprecated in drupal:11.2.0 and will be disallowed in drupal:12.0.0. Specify a bundle, either as a string or as a route parameter placeholder, in the route requirement. See https://www.drupal.org/node/3505093',
    • Suggested change
      61 'Defining a \'%s\' route requirement without a bundle for an entity type which has bundles (%s) is deprecated in drupal:11.2.0 and will be disallowed in drupal:12.0.0. Specify a bundle, either as a string or as a route parameter placeholder, in the route requirement. See https://www.drupal.org/node/3505093',
      61 'Defining a \'%s\' route requirement without a bundle for an entity type (%s) with bundles is deprecated in drupal:11.3.0 and will be disallowed in drupal:12.0.0. Specify a bundle, either as a string or as a route parameter placeholder, in the route requirement. See https://www.drupal.org/node/3505093',
    • Please register or sign in to reply
  • 53 53 public function access(Route $route, RouteMatchInterface $route_match, AccountInterface $account) {
    54 54 [$entity_type, $bundle] = explode(':', $route->getRequirement($this->requirementsKey) . ':');
    55 55
    56 // For an entity type that has bundles, the $bundle must be specified.
    57 $entity_type_definition = $this->entityTypeManager->getDefinition($entity_type);
    58 if (empty($bundle) && $entity_type_definition && $entity_type_definition->hasKey('bundle')) {
    • Suggested change
      58 if (empty($bundle) && $entity_type_definition && $entity_type_definition->hasKey('bundle')) {
      58 if (empty($bundle) && $entity_type_definition?->hasKey('bundle')) {
    • Please register or sign in to reply
    Please register or sign in to reply
    Loading