[Meta] Implement strict typing in existing code

I’m very much in favor of getting type hints as much as possible into existing code.
However, we’ve been announcing for years now (I think) that “Drupal 9.0.0 will just be the last version of Drupal 8 minus all deprecated stuff”. Making this change in Drupal 9 would break that promise in a major way – instead, almost every single module will have to be adapted in some way (even if this can be automated) to be compatible with Drupal 9, the same module code running on 8 and 9 would be almost impossible (as long as any Core interfaces/classes are implemented/extended).

So, much to my disappointment, it doesn’t seem like we’ll get comprehensive type hints in Drupal Core any time soon. (Even in Drupal 10 we’d have the same problem again, making the update a lot harder than it would be for sites, something I think we want to very much avoid. We could at least announce it years in advance this time, and not make that same promise about just removing deprecated stuff – but I doubt it would still be worth it.)

#2 is very spot on. We made a big promise before understanding the real implications. We have already added to that promise … and we will update 3rd party dependencies (which is very much needed).

While I understand the problem of having this breaking change in contrib and custom code I'm also saddened that we won't be able to add type hints and return types to our code base ever if we follow that promise to the letter.

I don't think we did not understand the consequences. The Drupal 7 to 8 move had a consequence of 71% of Drupal sites still being on Drupal 7 even 4.5 years after Drupal 9 is released. We cannot deal with that lack of update rate. So we need to make changes that let people come along rather than breaking everything all at once.

Newly added APIs for example can start use strict APIs once the PHP requirements align, so long as those APIs work with the rest of Drupal. In fact that would be a good testing ground for resolving big gnarly issues like #3029540: [Symfony 4] Sub class \Symfony\Component\Validator\ConstraintViolation and use that in \Drupal\Core\TypedData\Validation\ExecutionContext::addViolation() where we hit a problem with markup objects vs. strict typing. Once some new APIs start using it several issues will come up and will need to be worked on.

This is really the same problem as any other change to an interface, it's not unique to type hints.

It's worth reading https://www.drupal.org/core/d8-bc-policy again on exactly what changes are allowed.

Let's take Entity::load() for an example.

There are two ways I could could see doing this:

A: The very long way, compatible 100% with our current bc policy:

1. Add EntityInterface::loadEntity($id) method with the strict type hint in 9.x and deprecate Entity::load() for removal in 10.x - we can add the method because Entity is a base class.

2. In 10.1.x or later, add back Entity::load() with the return type hint (at this point non-updated modules from the 9.x era that override the method would break, but they've had plenty of time), and deprecate Entity::loadEntity() for removal in Drupal 11.

This provides plenty of notice for contrib modules, but the obvious disadvantage is that all calls to the method have to be updated twice, which would be a lot.

B. The short way, requires that we allow ourselves to break interface methods for implementors only for major releases.

1. Add the strict type hint to EntityInterface::load()
2. Any module overriding EntityInterface::load() will need to branch for 10.x (no other changes necessary).

If we look at real-world disruption, how many modules actually override EntityInterface::load()?

In core there is only one: Views UI https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Entity%21...

I did not grep contrib, but if there are not contrib modules overriding Entity::load() then it turns out to be in practice a non-issue from the point of view of a continuous upgrade path or at worst an extremely tiny issue requiring a couple of contrib modules to branch. This compared to millions of lines of code that calls EntityInterface::load() having to be updated twice, and all the modules that do so having to branch by Drupal 10.1

However this does not apply everywhere. For plugin interfaces, we expect field formatters and widgets to actually implement methods, and conversely we expect hardly any contrib or custom code to call those methods. So for plugin interfaces option A seems better.

There's also the MS model, all over the place in Win32:

C. Introduce alternate methods with extended signatures, like: EntityInterface::LoadEx() with a string type hint (params and return).

A little expansion on the examples in the issue summary to think about.

Example 1:

The fatal error here is on the return value typehint. The base class (or interface) declares it will return an array, so the child (or implementing class) has no right to break the contract by returning anything broader. A child class can sometimes declare a return type that is more specific though i.e.

<?php
interface C {
    public function foo() : Iterable;
}

class Baz implements C {
    public function foo() : array {
        return [];
    }
}

(new Baz())->foo();

is valid in 7.1+

The argument typehint here only throws a warning on PHP 7.0 and 7.1, and that warning was removed in 7.2, because an expansion of the types of arguments in a child or implementing class accepts does not break the contract (you can still call public function foo($x) if $x is an int). As long as the child class only broadens the argument type, there are no problems in php 7.2+

Example 2

Opposite issue here. The warning here is only on the more restrictive argument typehint. The base class (or interface) declares it can accept anything, so calling code typehinting on the interface or base class expects to be able to pass anything, and the child class can't break the contract by requiring something more restrictive. There is no problem, even in PHP 7.0 with the child or inheriting class providing a more restrictive return typehint than the base class or interface. i.e.

<?php
class Base {
    public function foo($x) {
        return [];
    }
}

class Child extends Base {
    public function foo($x) : string {
        return "great " . $x;
    }
}

$c = new Child();

echo $c->foo("bar");

Is perfectly acceptable in php 7.0, and contrib and custom code is free to start using that as early as Drupal 8.8.

I'll assume that we take the stance that the typehints in the docblock define the interface, and the act of making them PHP typehints is not in and of itself a breaking change (That is to say if the docblock says an argument or return type is one thing, and your calling code or implementation does another and it happens to work right now, that's on you. The typehints aren't breaking your code, your code is already broken)

In that case, the path forward is pretty clear. For arguments, Whenever our minimum supported php version is 7.2, (Probably Drupal 9, but whenever), we can safely add scalar typehints to interfaces and base classes which have them declared in the docblock.

For return types, we need a major cycle to deprecate implementations of interfaces that do not typehint the return type. I would suggest that after Drupal 9 is released we declare that it is deprecated to inherit or implement a method with a return type declared in its docblock without typehinting the return value. It would be amazing if we could come up with a drupal-check like tool, or something built into drupal.org to detect and report this, but the idea is to give contrib and custom code the entire Drupal 9 cycle to get on board. It gets slightly messy if you think about custom code extending contrib code which inherits core interfaces, because adding a return typehint to a contrib class method would break any custom code that overrode that contrib class method. Contrib would have to evaluate whether their classes are likely to be extended and perhaps release a new branch if they felt it would be a breaking change for their users, but they would have the entire Drupal 9 cycle to do it.

Then for Drupal 10.0, we can use a phpdoc to typehint tool to convert core all at once. I think a path like this is doable as long as it's declared and documented early in the D9 cycle.

we can use a phpdoc to typehint tool to convert core all at once.

Drupal8-Rector could be the perfect tool for automated this task and it could also provide an automated upgrade path for custom and contrib modules. Which means that Drupal core might be able to ship this kind of BC breaking change even in Drupal 9 because Drupal 8 codes could be automatically upgraded to the new "Drupal 9 syntax".

Adding missing return types automatically:
https://travis-ci.org/mxr576/drupal8-rector/builds/530614205
Mixed return types could be a problem here. For example a hook_update_N() implementation can return nothing or a translated string. Based on this its return type should be ?string or ?\Drupal\Core\StringTranslation\TranslatableMarkup, although if this return type-hint is being used the update hook implementation must ends with return null if it does not return a translated string. This means existing update hooks have to be validated and missing return nulls have to be added. (This should be also possible with D8-Rector.)
Adding void return type would not be valid solution even though it would cover the most common "nothing is being returned" use case for update hooks, but it would not cover the optional translated string return value.

Adding missing parameter types automatically:
https://travis-ci.org/mxr576/drupal8-rector/builds/530614789
IIRC the problem with this rector that it is not able to identify parameter types from "Implements hook_XY" annotation. We may figure out a solution for that.

@ catch/#5: Interesting idea. However, I’m not sure whether evaluating the feasibility of variant B for every single method is worth it. In the end, there won’t be that many interface methods that are overridden rarely enough (I’d guess?), and once we do it for all methods which are overridden at most, say, three times in all of contrib, we’ll still inconvenience a lot of module maintainers in total. And we’d still be stuck with a lot of methods without proper type hints.

@ mikelutz/#7: Ah, very good point, thanks! Forgot that PHP 7.2+ actually takes into account that parameter types should be contravariant.
So, yes, once we depend on that we could at least add parameter type hints wherever we want. (At least, in my opinion we should say that we can. But probably that needs to be officially decided and communicated beforehand.)

Not sure about the feasibility of requiring module maintainers to add return type hints for classes implementing interfaces themselves. This would a) require all of those maintainers to come up with the proper type hint on their own, and b) lead to problems (as you say) every time such a class is then again sub-classed.

Created a coding standards proposal to update our coding standards to always use type hints as much as possible for all new code: #3060914: All new Drupal 8/9 code must use strict type hints where possible. Would love your +1 support and opinion there!

So, the symfony team has come up with a policy and solution on how to add return typehints, and I believe we can implement something similar during the D9 cycle to allow us to include return typehints in D10. Beginning with Symfony 4.4, they've begun triggering errors in their classloader related to return typehints.

When a class is loaded that extends a Symfony base class or implements a symfony interface,
If the interface has an overridden/implemented method with a @return tag in the docblock
And the loaded class does not declare that return type as a php return type

Then it triggers an error of the format " Method "Symfony\Component\DependencyInjection\ContainerInterface::get()" will return "?object" as of its next major version. Doing the same in child class "Drupal\Component\DependencyInjection\Container" will be required when upgrading.
1x in FieldConfigAccessControlHandlerTest::setUp from Drupal\Tests\field\Unit"

If, for whatever reason the implementing/extending class cannot add a php type-hint (can't typehint object and be php 7.0 compatible, or the class itself is extended in contrib or whatever), then the error can be suppressed by adding a matching @return tag to the subclass' method's docblock ({@inheritdoc} doesn't count. You need to actually have the @return tag).

Specifically, With Drupal 9 on Symfony 4.4, we will likely need to change our docblocks from @inheritdoc to a copy of the docblock. This will suppress the deprecations for core, but display them if contrib happens to be extending any of these classes. (as opposed to suppressing them in our error handler, which would suppress them for contrib too). Then we can safely add these php return type-hints for the classes that extend/implement SF classes/interfaces in Drupal 10.

I believe we can do something similar for our own interfaces in Drupal 9, either working with the symfony team to find a way to enable the deprecation notices for our interfaces, or in a custom classloader based on the SF4.4 classloader where we add support for triggering these errors against Drupal core interfaces and base classes.

That, hopefully combined with ongoing improvements to the static analysis tools I think should provide enough warning to contrib to add the return type hints, or choose to add the @return tag to the docblock to suppress the error, with the understanding that the php return type hint will need to be added for D10 compatibility, be that in a separate branch or later release.

@mikelutz: how would tools detect this for contribs preparing for Drupal 9? Looks like this adds another layer of Symfony imposed changes that we would need to check somehow.

This won't be an issue in preparing for Drupal 9. These deprecation errors will be introduced in Drupal 9, and modules would have to update for Drupal 10. The detection would come via a regular E_USER_DEPRECATED error.

Technically, I suppose this means that If you have deprecation testing on in your drupalci.yml, then your working D8 module might have test failures against D9 because you won't have been able to address the new deprecations yet, although this won't be the only place where that might be an issue.

If this proves to be a problem for PMs and RMs, we could (reasonably easily) decide to suppress all new symfony deprecation messages (whether core would trigger them or not) in Drupal 9.0, and then re-enable them for Drupal 9.1. It might make for an easier transition.

@mikelutz: yeah I was thinking that people would want to use non-suppressed deprecation testing to prepare for Drupal 9 so this will be "in the way" (AKA making it harder to adapt to Drupal 9).

I'm in favor of enforcing strict typing; it helps to prevent bugs.

I understand it would be a major break but not doing so could also lead to breaking code.

Developers are now already using strict typing in custom & contrib, sometimes even for classes that extend core classes which don't apply this.
EntityInterface is a good example where things could go really wrong:

  /**
   * Gets the identifier.
   *
   * @return string|int|null
   *   The entity identifier, or NULL if the object does not yet have an
   *   identifier.
   */
  public function id();

Imagine someone extending ContentEntityInterface in MyContentEntityInterface and using "int" for the id property. Now since ConfigEntityInterface typically uses strings, core could decide to use "string" instead of "int", resulting in a broken MyContentEntityInterface...

Well, exactly that is an example that's not possible to do, not without union types from PHP8.

If we had adopted usage of final classes this issue would apply only to abstract classes and interfaces.

I think it's too late in 9.0.x development to do this anymore for that branch.

In general, changing interface contracts requires a major-only change since it's a BC break. We'll need to figure out how to dovetail that with the continuous upgrade path somehow... moving to 9.1.x to continue the discussion. #11 sounds promising

There's sort-of an example of this happening in #3055194: [Symfony 5] The signature of the "Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher::dispatch()" method should be updated to "dispatch($event, string $eventName = null)", not doing so is deprecated since Symfony 4.3. due to upstream changes in Symfony, using their policy as described in #11.

That case is extra-complex because they also reversed the argument order of EventDispatcher::dispatch(), but see the differences between 4.2 and 4.3

4.2
https://github.com/symfony/event-dispatcher/blob/4.2/EventDispatcherInte...

  public function dispatch($eventName, Event $event = null);

https://github.com/symfony/event-dispatcher/blob/4.2/EventDispatcher.php

public function dispatch($eventName, Event $event = null)

4.3:

https://github.com/symfony/event-dispatcher/blob/4.3/EventDispatcherInte...

https://github.com/symfony/event-dispatcher-contracts/blob/master/EventD...

 interface EventDispatcherInterface
    {
        /**
         * Dispatches an event to all registered listeners.
         *
         * For BC with Symfony 4, the $eventName argument is not declared explicitly on the
         * signature of the method. Implementations that are not bound by this BC constraint
         * MUST declare it explicitly, as allowed by PHP.
         *
         * @param object      $event     The event to pass to the event handlers/listeners
         * @param string|null $eventName The name of the event to dispatch. If not supplied,
         *                               the class of $event should be used instead.
         *
         * @return object The passed $event MUST be returned
         */
        public function dispatch($event/*, string $eventName = null*/);
    }

https://github.com/symfony/event-dispatcher/blob/4.3/EventDispatcher.php

    public function dispatch($event/*, string $eventName = null*/)

This is the deprecation message removed by the patch:

-      'The signature of the "Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher::dispatch()" method should be updated to "dispatch($event, string $eventName = null)", not doing so is deprecated since Symfony 4.3.',

So if we boil down what they've done to a simplified version, it looks more something like this:

Old version:

Interface foo {
  public function bar($argument)

Interim version:

Interface foo {
  public function bar(/* string $argument /*);
}

Major version change:

Interface foo {
  public function bar(string $argument);
}

i.e. removing parameters from a method on an interface, isn't a hard bc break, because implementing classes (and extending interfaces) are allowed to add additional parameters.

Classes then work whether they use the old signature or the new one, because the interface is temporarily not specifying anything at all.

This then allows the parameter to be added back to the interface in the major version update, with the new type hint.

Re #20 - why do we need the interim version in the simplified case. I think the only reason Symfony commented out the argument is so they can switch the order.

To answer my own question to allow the following code to work... https://3v4l.org/cMfng - so implementors of the interface can upgrade early.

I'm not sure how practical this will turn out for core though...

How would we change something like FormatterBase::construct()... ie.

public function __construct($plugin_id, $plugin_definition, FieldDefinitionInterface $field_definition, array $settings, $label, $view_mode, array $third_party_settings) {

@alexpott I think we'd need to remove literally all of the named parameters and use func_get_args() in Drupal 9, then add all the parameters back with the desired type hints in Drupal 10.

@catch so why wouldn't implement Symfony's debug classloader and change the typehints in D10? And have a breaking change between 9 and 10. The disruption to developer experience / type safety / extra code does not feel worth it.

@alexpott so I'm not sure a constructor is the best example, but let's say it's a different method on FormatterBase.

If we do the 'interim' method, then all core subclasses (which won't be covered by the bc policy) can be updated in Drupal 9 to add the correct type hints to their methods, this would also mean we don't need to suppress our own deprecation warnings. Those classes won't need any changes in Drupal 10, which minimises divergence for backports.

Also, contrib modules can start to update their modules for Drupal 10 compatibility now, instead of having to do everything at once, and in a new branch, for Drupal 10.

See #11 for way to suppress the deprecation method for return type hints in Symfony, but that appears to be designed for the exception rather than the usual case - and it means they have to do different work in two branches to keep up. Also while that's possible for type hints, I don't see how it could ever be possible for parameters where the PHP behaviour is different. So if we don't do the interim method, then contrib modules have an unresolvable deprecation message which they can never fix in Drupal 9.

A couple of problems I can see with this approach:

• With things like Blocks / Controllers / Formatters we say they are not under BC but people doo extend them all the time so if we make the changes in D9 we will break contrib and custoom
• We lose typehinting in IDEs and autocompletion when you generate methods to implement an interface

I'm not saying that we shouldn't do something like this but there are definitely downsides.

Here's example of break (from alexpott's comment in slack)
Fatal error: Declaration of PDOStatement::fetch(int $fetch_style = PDO::FETCH_BOTH, int $cursor_orientation = PDO::FETCH_ORI_NEXT, int $cursor_offset = 0) must be compatible with Drupal\Core\Database\StatementInterface::fetch($mode = null, $cursor_orientation = null, $cursor_offset = null) in ..../drupal8alt.dev/core/lib/Drupal/Core/Database/Statement.php on line 0

Updated the issue summary with a couple more examples.

Short version is: we can enable things so that contrib can add the requisite type hints in Drupal 9, if we accept a DX regression in Drupal 9 core (via the func_get_args() trick), then add the type hints to core in Drupal 10.

Return type hints are easier, but we need a way to inform contrib/custom developers that they need to add them, before they're actually added in core.

Union types are/will be a thing in PHP8, so that should make this a bit more doable for when PHP8 is a Drupal requirement.

https://wiki.php.net/rfc/union_types_v2

PoC for #30 in #3217961: Prepare for Connection::select() signature to be fully typehinted in D10.

There's some useful bits in #3164389: Enforce block plugins returning an array as well; I suggested there that we actually add a core API for this type of deprecation. The API should:

1. Raise deprecation warnings if the incoming data does not match the expected data type, for parameter typehints.
2. Where possible, raise deprecations in the caller or discovery API (e.g. BlockManager for implementations of BlockInterface) when return data types do not match our expectations.
3. Raise deprecations for implementations that are not existing core implementations where the expected typehint is not present, both to ensure new core implementations get the typehint, and to notify contrib that the typehint will be needed in D10.
4. Allow a way that existing core implementations' non-core subclasses trigger the deprecation for the missing typehint.
5. Allow existing core implementations themselves to not raise the deprecation for the missing typehint.

Rector rules to add the typehints where appropriate will also be super helpful.

Symfony recently added support for triggering deprecation messages for upcoming return type hints in the debug class loader. This appears to 'just work' when you update to Symfony 5 - as long as the deprecations are documented in a special format - updating 9.3.x to Symfony 5 produces thousands of test fails due to return type hint deprecations.

#3161889-141: [META] Symfony 6 compatibility / https://github.com/symfony/symfony/pull/42623

We'd need to backport this to have it available in Drupal 9, although we could adopt the documentation format before that.

#35 was over-optimistic, it 'works' for every method with a @return annotation and no return type hint, so not something we can directly adopt. Opened #3232131: [Symfony 5] Symfony's DebugClassLoader triggers deprecation messages for missing return type hints, where there is no deprecation.

I think we should probably split return type hints off to their own issue - for that we can adapt/adopt what Symfony's done and it'll work. Then leave this issue for methods.

Edit: opened #3333824: Enable existing interfaces to add return type hints with a deprecation message for implementors

Opened #3404246: [META] Fix strict type errors detected by phpstan which found 271 errors that need fixing.

Added new child #3410098: Deprecate passing non-strings to UserSession::hasPermission() and User::hasPermission()

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.

Also, I like saying "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 arguments. 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.

The bigger issue though is if we only add the type hints in 11.x, then contrib can't update and also stay compatible with 10.x, which breaks the continuous upgrade path, would we need to tell contrib not to add the new type hints until they drop support for 10.x?

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."

In other words: the mere presence of strict parameter types in core is not a BC break (unless somehow your code was counting on the parameters being untyped, which seems somewhat unlikely to me).

But the adoption of strict types by subclasses is a breaking change, and therefore can only be done across a major version boundary.

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 #3436327: Add type declaration to untyped scalar params on interfaces 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. #3436327: Add type declaration to untyped scalar params on interfaces 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 endless back-and-forth; 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.

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.

One example is perhaps TranslatableMarkup where we need to consider if we want the object or just the string output to be carried around.

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.

In #3218426: Using partially synchronized image fields causes validation errors and PHP warnings, 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

Opened #3461318: Enforce return types in all new methods and functions