Problem/Motivation

Using a union type in a controller's route callback throws a fatal error.

Fatal error: Uncaught Error: Call to undefined method ReflectionUnionType::isBuiltin() in /data/app/core/lib/Drupal/Component/Utility/Reflection.php:21
Stack trace:
#0 /data/app/core/lib/Drupal/Core/Entity/EntityResolverManager.php(143): Drupal\Component\Utility\Reflection::getParameterClassName()
#1 /data/app/core/lib/Drupal/Core/Entity/EntityResolverManager.php(214): Drupal\Core\Entity\EntityResolverManager->setParametersFromReflection()
#2 /data/app/core/lib/Drupal/Core/EventSubscriber/EntityRouteAlterSubscriber.php(48): Drupal\Core\Entity\EntityResolverManager->setRouteOptions()
#3 [internal function]: Drupal\Core\EventSubscriber\EntityRouteAlterSubscriber->onRoutingRouteAlterSetType()
#4 /data/app/core/lib/Drupal/Component/EventDispatcher/ContainerAwareEventDispatcher.php(142): call_user_func()
#5 /data/app/core/lib/Drupal/Core/Routing/RouteBuilder.php(189): Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher->dispatch()
#6 /data/app/core/lib/Drupal/Core/ProxyClass/Routing/RouteBuilder.php(83): Drupal\Core\Routing\RouteBuilder->rebuild()
#7 /data/app/core/includes/common.inc(587): Drupal\Core\ProxyClass\Routing\RouteBuilder->rebuild()

In my case I had a route that had a node parameter originally only accepting a single bundle but when I enhanced it to 2 it started breaking.

Steps to reproduce

This is just following my setup...

Create 2 Content types Article and Blog
Create bundle classes for the above bundles and use hook_entity_bundle_info_alter to wire them up.
Create a route that looks like the following:

my_module.my_route
  path: '/some/path/{node}'
  defaults:
    _controller: '\Drupal\my_module\Controller\MyController::build'
  requirements:
    _entity_access: 'node.view'
    _format: 'json'
  options:
    parameters:
      node:
        type: entity:node
        bundle:
          - article
          - blog

With the controller callback:

public function build(Article|Blog $node): array {

This last bit specifically is what triggers the error. The ReflectionUnionType is returned because of this parameter type.

Proposed resolution

Safeguard against parameter types that aren't ReflectionNamedType in Reflection::getParameterClassName

Remaining tasks

Review https://git.drupalcode.org/project/drupal/-/merge_requests/12419
Open follow-ups if needed to handle these better

User interface changes

N/A

Introduced terminology

N/A

API changes

N/A

Data model changes

N/A

Release notes snippet

N/A

Issue fork drupal-3315276

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

acbramley created an issue. See original summary.

larowlan’s picture

kristen pol’s picture

Issue tags: -#drupalsouth +DrupalSouth

Updating tag.

donquixote’s picture

The Reflection::getParameterClassName() needs to somehow deal with union types.
But what would be a suitable return value in case of union types?

A possible backwards compatible solution could be:
- Let Reflection::getParameterClassName() return NULL, if the type is a union or intersection type.
- Optionally, add another parameter to control throwing an exception instead of returning NULL. OR introduce another method like "::demandParameterClassName()",
- Add a new method Reflection::getParameterClassNames() that also supports union types. This one would return NULL for intersection types.
- Alternatively, a new method could receive the type itself as a parameter, so it could also be used with return types.
- All the existing calling code should treat the NULL case as if there is no parameter type at all. Gradually, each piece of calling code could be updated to also support union types.

---------

This said: For the example controller, I think it would be better to simply use "Node" or "NodeInterface" as the parameter type, and then within the method call instanceof to work with the more specific bundle classes.
As a workaround, you could also rename the parameter from `$node` to something like `$article_or_post` where you are sure that no entity type exists with this name.

spadxiii’s picture

Version: 9.5.x-dev » 11.x-dev
StatusFileSize
new733 bytes

I ran into this with a rest controller myself where it had "string|int" as its parameter.

Added a small change to the if-statement in \Drupal\Component\Utility\Reflection::getParameterClassName:
method_exists($parameter->getType(), 'isBuiltin')

I'm not sure how to handle union types in this case, but this patch allowed me to prevent the error from happening :)

voleger’s picture

The same error appears when generating a proxy class for a class with union types used in definitions.
php core/scripts/generate-proxy-class.php '\Drupal\Core\Batch\BatchProcessor' "core/lib/Drupal/Core".
Affected me working on #2959723: Create an initial class for the batch processor service

acbramley’s picture

Status: Active » Needs review

Finally getting around to this one, I think for now we just return NULL and don't automate the entity upcasting because I can't think of a good way that you could safely upcast an entity parameter if it's a union type?

Perhaps only if all of them are a subclass of $entity_type->getClass();?

smustgrave’s picture

Status: Needs review » Needs work
Issue tags: +Needs issue summary update

Sorry to be that guy

Can we clean up the issue summary, appears to be missing section with proposed solution

acbramley’s picture

Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs issue summary update

My bad, updated.

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

This is one I'll lean on the test coverage https://git.drupalcode.org/issue/drupal-3315276/-/jobs/6236922

1) Drupal\Tests\Core\Entity\EntityResolverManagerTest::testSetRouteOptionsWithUnionEntityType with data set #0 ('Drupal\Tests\Core\Entity\Basi...onType')
Error: Call to undefined method ReflectionUnionType::isBuiltin()
/builds/issue/drupal-3315276/core/lib/Drupal/Component/Utility/Reflection.php:21
/builds/issue/drupal-3315276/core/lib/Drupal/Core/Entity/EntityResolverManager.php:150
/builds/issue/drupal-3315276/core/lib/Drupal/Core/Entity/EntityResolverManager.php:221
/builds/issue/drupal-3315276/core/tests/Drupal/Tests/Core/Entity/EntityResolverManagerTest.php:243
2) Drupal\Tests\Core\Entity\EntityResolverManagerTest::testSetRouteOptionsWithUnionEntityType with data set #1 ('Drupal\Tests\Core\Entity\test...n_type')
Error: Call to undefined method ReflectionUnionType::isBuiltin()
/builds/issue/drupal-3315276/core/lib/Drupal/Component/Utility/Reflection.php:21
/builds/issue/drupal-3315276/core/lib/Drupal/Core/Entity/EntityResolverManager.php:150
/builds/issue/drupal-3315276/core/lib/Drupal/Core/Entity/EntityResolverManager.php:221
/builds/issue/drupal-3315276/core/tests/Drupal/Tests/Core/Entity/EntityResolverManagerTest.php:243
ERRORS!
Tests: 21, Assertions: 38, Errors: 2, PHPUnit Deprecations: 17.
Exiting with EXIT_CODE=2

Which seems direct assertions for what was added.

Summary is pretty clear about checking for instances of ReflectionNamedType. Which matches the MR

Going to mark but only question I have is do we need an else condition to log or catch those instances that aren't ReflectionNamedType? Or overkill?

needs-review-queue-bot’s picture

Status: Reviewed & tested by the community » Needs work
StatusFileSize
new91 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

smustgrave’s picture

Status: Needs work » Reviewed & tested by the community

Was a clean rebase

smustgrave’s picture

quietone’s picture

Status: Reviewed & tested by the community » Needs work

Setting to NW because of test failures

acbramley’s picture

Status: Needs work » Reviewed & tested by the community

Took me a while to figure out what was going on, the DataProvider was pointing to the wrong method!

astonvictor’s picture

I was able to reproduce the issue on my local and the MR fixes it. thnaks
+1 RTBC

voleger’s picture

Are union return types covered in the test?

  • catch committed 964bfbb2 on 11.3.x
    fix: #3315276 Fatal error when using a union type on a controller's...

  • catch committed 1ba26c89 on 11.x
    fix: #3315276 Fatal error when using a union type on a controller's...
catch’s picture

Version: 11.x-dev » 11.3.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 11.x and cherry-picked to 11.3.x, thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.