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
| Comment | File | Size | Author |
|---|
Issue fork drupal-3315276
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:
- 3315276-fatal-error-when
changes, plain diff MR !12419
Comments
Comment #2
larowlanComment #3
kristen polUpdating tag.
Comment #4
donquixote commentedThe 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.
Comment #5
spadxiii commentedI 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 :)
Comment #6
volegerThe 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
Comment #8
acbramley commentedFinally 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();?Comment #9
smustgrave commentedSorry to be that guy
Can we clean up the issue summary, appears to be missing section with proposed solution
Comment #10
acbramley commentedMy bad, updated.
Comment #11
smustgrave commentedThis is one I'll lean on the test coverage https://git.drupalcode.org/issue/drupal-3315276/-/jobs/6236922
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?
Comment #12
needs-review-queue-bot commentedThe 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.
Comment #13
smustgrave commentedWas a clean rebase
Comment #14
smustgrave commentedJK new phpstan failures https://git.drupalcode.org/issue/drupal-3315276/-/jobs/6365030
Comment #15
quietone commentedSetting to NW because of test failures
Comment #16
acbramley commentedTook me a while to figure out what was going on, the DataProvider was pointing to the wrong method!
Comment #17
astonvictor commentedI was able to reproduce the issue on my local and the MR fixes it. thnaks
+1 RTBC
Comment #18
volegerAre union return types covered in the test?
Comment #21
catchCommitted/pushed to 11.x and cherry-picked to 11.3.x, thanks!