\ReflectionParameter::getClass() is deprecated in PHP 8.0

So this is not the only place that we use ReflectionParameter::getClass() but I agree that we should handle each one in an issue like this so we can review and think about what to do in each case.

https://stackoverflow.com/q/62705458/308851

@Charlie ChX Negyesi is #8 pointing out we have a problem with union types?

I'm thinking actually that #6 is wrong and we should address all the usages of deprecated ReflectionParameter methods together as I think the are a couple of complexities that mean we should add a BC layer that we can call.

Here's all the changes of this ilk that we need from #3156595: Make Drupal 9 installable on PHP8. Now I'm going to add a testable BC layer that also has special handling for 'self' - see https://github.com/phpDocumentor/ReflectionDocBlock/pull/240

Here's the helper method and test. Unfortunately ProxyBuilder doesn't depend on utility - but also it's usecase is a little more specialised so I think we shouldn't try to use the help anyway. Also the proxy builder changes will have the reasonable side effect of making it handle scalar typehints correctly. So we might want to carve that out into a separate issue to add explicit test coverage anyway.

@alexpott yes for union types.

Edit: TranslationManager and combination of various entity interfaces are examples.

@Charlie ChX Negyesi sure union types are a related issue but it's not necessary to handle them here. They're not possible in PHP 7 so we can't use them in core - so support for them should be a follow-up or maybe just a related issue.

This is a bug fix for PHP 8. So it's not a task.

Splitting out the ProxyBuilder changes - #3156944: Support nullable typehints and return types in ProxyBuilder and as a result support PHP 8

If we are writing a helper class anyways, what about:

$wrapper = Reflection::create($param);
if ($wrapper->hasParameterClassName()) {
  $interface = $wrapper->getParameterClassName();
}
public static function hasParameterClassName() : bool {
  return $this->parameter->hasType() && !$this->parameter->getType()->isBuiltin();
}
public static function getParameterClassName() : string {
  $class = $this->parameter->getType()->getName();
  return $class === 'self' ? $this->parameter->getDeclaringClass()->getName() : $class;
}

Meantime core already require doctrine/reflection so maybe there's no need to introduce new component?

Setting NW because every component require own composer.json file and few text files

@andypost: hm, I looked at https://www.doctrine-project.org/projects/doctrine-reflection/en/1.2/ref... and don't know what would be useful from there to provide this functionality?

@andypost: is there docs on what a new component requires in core? Tried to find it via google, no luck.

I agree with @Charlie ChX Negyesi that introducing a has* method would help provide the condition part of the method as a separately useful functionality. Making the consumer use the has* method first to avoid getting an exception may not be the best idea. (I am not a Drupal core framework manager though ;)

@Gábor Hojtsy: sorry, I was wrong - this component already defined (drupal/core-utility)

I like making the class final but interesting how we can get rid of it in future or how we can extend it as no policy accepted yet

1. +++ b/core/lib/Drupal/Component/Utility/Reflection.php
   @@ -0,0 +1,30 @@
   + * Provides helper methods for reflection.
   + */
   +final class Reflection {
   ...
   +  public static function getParameterClassName(\ReflectionParameter $parameter) : ?string {
   

   There's only one method, and as class defined final - how it expected it can add new methods?

   Related #3019332: Use final to define classes that are NOT extension points

2. +++ b/core/lib/Drupal/Core/Entity/EntityResolverManager.php
   @@ -74,6 +75,10 @@ protected function getControllerClass(array $defaults) {
   +    if ($controller === NULL) {
   +      return NULL;
   +    }
   

   it could use a comment about early return

@andypost we have plenty of final classes in core. Anything static and in component is by design not meant to be extended - as it actually makes no sense. Adding final just enforces this sensibility from a code POV.

As far as I can see there is nothing more to do here. Created a CR (https://www.drupal.org/node/3173795) and updated the issue summary.

Re #17 - this is part of the utility component - it is not standalone.

Re #20 Using final and static for classes in component has precedent. The API contract is \Drupal\Component\Utility\Reflection::getParameterClassName() which once we land this patch we won't change. Since the class is final we are free to add more reflection utility methods to this class as we require as nothing can extend it and add other methods. If we decide to get rid of the class then we can deprecate it like any other class / method.

Re #16 I think adding a hasParameterClassName() is not worth it. In the code samples in #16 the hasParameterClassName() will always have to be called before the getParameterClassName(). Having getParameterClassName() declare that is can return NULL is more robust.

We don't use yoda conditions in Drupal...

1. +++ b/core/lib/Drupal/Component/Utility/Reflection.php
   @@ -0,0 +1,30 @@
   +      $class = $parameter->getType()->getName();
   

   In order to make this backportable to Drupal 8 and PHP 7.0 compat we'd need to do

   $type = $parameter->getType();
   $class = $type instanceof \ReflectionNamedType ? $type->getName() : (string) $type;
   

   I don't think we should do this here. But at least the option of backporting this to D8 is there. I've looked at similar changes in Symfony - for example \Symfony\Component\HttpKernel\ControllerMetadata\ArgumentMetadataFactory::getType() (it is private) and \Symfony\Component\DependencyInjection\LazyProxy\ProxyHelper::getTypeHint() (public static) and no of them fit perfectly unfortunately.

2. +++ b/core/lib/Drupal/Component/Utility/Reflection.php
   @@ -0,0 +1,30 @@
   +      if ($class === 'self') {
   

   We need to cope with parent typehints too :)

Addressing #2 and borrowing code from \Symfony\Component\HttpKernel\ControllerMetadata\ArgumentMetadataFactory::getType() which is very very similar.

I looks ready except docs

1. +++ b/core/lib/Drupal/Component/Utility/Reflection.php
   @@ -0,0 +1,35 @@
   +      $lc_name = strtolower($name);
   +      switch ($lc_name) {
   +        case 'self':
   ...
   +        case 'parent':
   

   Would be great to ref to https://wiki.php.net/rfc/case-sensitivity

2. +++ b/core/lib/Drupal/Core/Entity/EntityResolverManager.php
   @@ -74,6 +75,10 @@ protected function getControllerClass(array $defaults) {
   +    if ($controller === NULL) {
   +      return NULL;
   

   still needs comment, I see this change is not covered in \Drupal\Tests\Core\Entity\EntityResolverManagerTest

Re #26.1 I don't understand why linking to a draft RFC helps here. I think it's unnecessary.

Re #26.2 - actually is not part of this and deserves it's own issue. Imo this doesn't need a test. It is all about the internals ... see https://3v4l.org/4oYFT

@alexpott thank you, it explains, needs separate issue, #26.1 is optional cos explains class names are case insensitive

#26.2 issue #3173958: PHP 8 error: method_exists(): Argument #1 ($object_or_class) must be of type object|string, null given

Rtbc +1

It could use to add remains of https://github.com/doctrine/reflection/pull/43#issuecomment-710255073

LGTM. I agree that neither point in #26 needs fixing here. Looks like we might need more similar issues to replace the Doctrine component in the near future though.

This looks good to me. Worst case is if we have to home-grow a replacement for Doctrine Reflection, we might eventually want to move the code around, but it's one static method so easy to do that if we ever wanted to.

For union types there is now this new issue:
#3315276: Fatal error when using a union type on a controller's callback

Publish the CR