[Meta] Implement strict typing in existing code

Opened 📌 Fix strict type errors detected by phpstan Active which found 271 errors that need fixing.

Added new child 🐛 TypeError: PermissionChecker::hasPermission(): $permission must be of type string Needs review

What about adding type declarations in places where it is not a breaking change? For example, in Unicode::validateUtf8() and Xss::filter(), if you pass a non-string as the text parameter, that value will be used in a call to strlen(). This will result in the error message "Argument #1 ($string) must be of type string".

If we added type declarations to the two functions I mention, then there would be an error message when those functions are called instead of when strlen() is called. That would make it easier to track down the code that is ultimately causing the problem.

@Liam Morland currently drupal and PHP allow \Stringable objects, strings, integers, floats and Booleans to be passed to these functions. NULL is also allowed, but triggers a PHP deprecated notice. So to avoid a breaking change, you'd have to allow that full set of accepted types.

Get a load of this: https://git.drupalcode.org/project/drupal/-/merge_requests/7196/diffs

And check out its pipeline, which actually compiles and executes! (Although it doesn't pass tests, but it looks like the failures are due to pre-existing type-related bugs and weaknesses.)

Thanks to contravariance, it seems we can safely add type hints to untyped method parameters in core interfaces and classes. Any subclasses' untyped parameters will be implicitly treated as mixed, which lines up with the contravariance rules. It's not possible for subclasses to have added parameter type hints, since that would violate contravariance.

How would we want to go about it, if we wanted to do it? Maybe one big MR to add parameter type hints to all core service interfaces?

Note that there's also https://www.drupal.org/about/core/policies/core-change-policies/drupal-d... → for changing type hints, adding arguments, re-ordering methods. If we can just add type hints to interfaces and methods that don't have them, we should add a section for this too.

How would we want to go about it, if we wanted to do it? Maybe one big MR to add parameter type hints to all core service interfaces?

If we can script things so that we can apply type hints based on the phpdoc, then one big issue sounds great.

However given that tests failed in the canary MR, it's likely we'll find incorrect docs, or wrong types being passed and etc., and have to split those out.

There's also the question of whether we'd want to only do this in 11.x, I guess probably yes since it'll be an API change if callers are relying on loose typing.

would we need to tell contrib not to add the new type hints until they drop support for 10.x?

That seems reasonably fair to me. "Drupal 11 uses strict parameter typing. If your subclasses aren't using strict typing, no worries -- your code won't break. Strict typing is optional, and will continue to be optional. BUT, if you want to use the strict typing, you must drop Drupal 10 support."

If we can script things so that we can apply type hints based on the phpdoc, then one big issue sounds great.

FWIW rector has a set of existing rules for adding type declarations, for example AddParamTypeFromPropertyTypeRector for getters and setters. Unfortunately there doesn't seem to be a rule for AddParamTypeFromPhpDoc or similar, but there are a bunch of helpers in there for inspecting php docs. I think if we were to script this we should start by contributing the rule to rector.

I did a simple test modifying the existing \Rector\Php80\Rector\FunctionLike\MixedTypeRector and replacing checks for mixed and MixedType with string and StringType. This was successfully able to detect untyped params that had string types in their docs and add the string type declaration. We could certain rinse and repeat for primitive types like int and bool, and I'm sure with a bit more time we could make this generic across all types.

Opened 📌 Add string type to untyped string params Active to demonstrate #50.

Yeah the phpdoc rector was removed a while back. We might be able to get the code and revive it, but rector has had quite te changes to internals since.

Just read thru this issue and thought I should correct my comment in #45 - I was thrown off by the "Implement strict typing" title which made me think of strict_types, but this issue seems to be more about just adding type declarations, not strict_types. So in that context, adding a string type declaration with \Stringable objects being passed in should be fine.

Yep there is nothing stopping us - except actual bugs where our types are wrong - from adding argument types across core now, and contrib can follow at a later time.

I think to avoid contrib doing anything twice though we should try to consider return types too, although they have to be done in the other order - bottom of the class hierarchy first. Core can add argument types now, we tell contrib to add both argument and return types for Drupal 11, then core can add return types everywhere in Drupal 12.

one big issue sounds great

The concern I have with one big issue is that there will be a bunch of work done to get the patch ready and then further commits will make it out-of-date. The review will say it needs a re-roll. This process will just repeat forever. If we commit patches that add declarations to an individual class, it will get done.

We could encourage people to add type declarations anytime they are working on a class that doesn't have them.

Re #55:

there will be a bunch of work done to get the patch ready and then further commits will make it out-of-date

Not necessarily. 📌 Add string type to untyped string params Active only changes the interfaces, which change much less frequently than concrete classes. Besides, it's scripted - that means it's easily rerolled if needed. And thirdly, it's not exactly an issue that needs to take forever because it's adding or changing APIs/features. Once tests pass, and everyone agrees on which branches to commit it to, etc., and a change record is written, it's not going to be hard to land.

While I'm happy that a lot of debate has sparked about how we can already add types to existing interfaces, the prevailing thoughts still seems to be "We should fix types first and then we can raise PHPStan".

I personally wouldn't feel comfortable even adding the types to existing interfaces, especially not in a large MR until we've verified that the PHPDoc types for the interfaces are correct, that is something that PHPStan can help us with.

For example in the canary MR the following change was made (unchanged PHPDoc added for needed context):

   /**
    * Creates new handler instance.
    *
    * Usually \Drupal\Core\Entity\EntityTypeManagerInterface::getHandler() is
    * preferred since that method has additional checking that the class exists
    * and has static caches.
    *
    * @param mixed $class
    *   The handler class to instantiate.
    * @param \Drupal\Core\Entity\EntityTypeInterface $definition
    *   The entity type definition.
    *
    * @return object
    *   A handler instance.
    */
-  public function createHandlerInstance($class, EntityTypeInterface $definition = NULL);
+  public function createHandlerInstance(string $class, EntityTypeInterface $definition = NULL);

PHPStan (at a sufficient level) would complain about the mismatch between string in the code and mixed in the PHPDoc.

The implementation of EntityTypeManager looks as follows:

    if (is_subclass_of($class, 'Drupal\Core\Entity\EntityHandlerInterface')) {
      $handler = $class::createInstance($this->container, $definition);
    }
    else {
      $handler = new $class($definition);
    }
    if (method_exists($handler, 'setModuleHandler')) {
      $handler->setModuleHandler($this->moduleHandler);
    }
    if (method_exists($handler, 'setStringTranslation')) {
      $handler->setStringTranslation($this->stringTranslation);
    }

    return $handler;

is_subclass_of can take a class instance but also a class string. The preferable input is likely to be a class-string (since a class instance wouldn't actually be used). So a decision has to be made here on an interface level what the intended inputs are. Most likely this is a class-string for an EntityHandlerInterface. That provides EntityTypeManager with a deprecation path forward for its else statement that other classes could follow.

Using the expressiveness of PHPStan's extended types in the PHPDoc and PHPs more limited type system in the PHPCode, unless we want to make a conscious change of how the entity type manager works, the interface probably should be changed to:

  /**
   * Creates new handler instance.
   *
   * Usually \Drupal\Core\Entity\EntityTypeManagerInterface::getHandler() is
   * preferred since that method has additional checking that the class exists
   * and has static caches.
   *
   * @param class-string<\Drupal\Core\Entity\EntityHandlerInterface> $class
   *   The handler class to instantiate.
   * @param \Drupal\Core\Entity\EntityTypeInterface $definition
   *   The entity type definition.
   *
   * @return object
   *   A handler instance.
   */
  public function createHandlerInstance(string $class, EntityTypeInterface $definition = NULL);

(Even while writing this I had originally added \Drupal\Core\Entity\EntityHandlerInterface instances as valid type because the current code would work when it's provided a handler instance; however the code likely doesn't make sense in that way. That points me to the requirement for human judgement with an automated system likely encoding unwanted types that require more breaking changes later).

I'm all in favor of starting to add better type annotations to existing interface arguments, regardless of our decision on PHPStan level :D However, I do think the above demonstrates unfortunately we probably need to do so in a non-automated fashion in reviewable chunks.

With that said, since this is an issue close to my heart, I'm happy to volunteer as a first reviewer for anyone that sets up a PR like the above (easiest to ping me in the Drupal Slack).

I do agree with #57 that this needs significant manual review of each case; if we add argument types but later they turn out to be wrong (especially if they are too narrow) once they have propagated down that will be harder to fix than ensuring we get them right first time.

With TranslatableMarkup - we nearly always want the object... we only want to do the translation if the string is printed to something a user is going to see.

In fact this is true of anything using MarkupInterface - we nearly always want Twig to be the thing that converts it from an object to a string.

Yeah, and this is related to my point: if we start adding string types (because that is what the docblock says) when we should be using TranslatableMarkup, and a downstream class follows suit, then we can't widen the type on the interface later without causing errors.

We should probably be type-hinting string|Stringable in those cases too rather than MarkupInterface.

then we can't widen the type on the interface later without causing errors.

It's possible to do this by removing the parameter from the interface altogether and adding it back later, but it's quite a painful process and we shouldn't set ourselves up to do it where we can avoid it.

xjm → credited larowlan → .

In 🐛 Using partially synchronized image fields causes validation errors and php warnings Needs work , contributors were adding return typehints to a new subclass that were not present on the parent. Since the subclass is new, it doesn't have any subclasses of its own to break; however, given how much Drupal allows altering everything, it is still possible that this could cause a disruption. I did an extensive evaluation of the use of the methods in contrib and determined that contrib, at least, would be unaffected by the changes in this particular issue. In general, only rare/edgecase usecases would be broken.

I discussed this with @catch and @larowlan and we agreed that we will adopt a practice of requesting return typehints in new subclasses, and reverting the change in any rare cases that contrib breaks as a result.

Fixing issue credit.

PHP 8.4 becoming stricter so after related it will be more cleaner