Refined ServiceManager factory, delegator and configuration types, to allow for easier introspection of mis-configuration at type level #98

boesing · 2021-07-31T20:21:32Z

Q	A
Documentation	kinda
QA	yes

Description

This reduces the overall baseline by 140 errors. Some errors still have to be suppressed inline.

src/AbstractPluginManager.php

src/Config.php

src/ServiceManager.php

src/AbstractPluginManager.php

src/PluginManagerInterface.php

src/ServiceManager.php

src/ConfigInterface.php

weierophinney

I think this is a GREAT addition! I've flagged a few nitpicks, which are primarily to close a few more Psalm errors as well as simplify some of the methods by promoting early returns.

Otherwise, 🚢

src/AbstractPluginManager.php

src/Config.php

src/ConfigInterface.php

src/ServiceManager.php

test/AbstractPluginManagerTest.php

test/CommonServiceLocatorBehaviorsTrait.php

boesing · 2021-09-15T14:40:14Z

After some talks to @Ocramius, we are thinking about extracting the @template-Annotation into a dedicated patch to avoid upstream analysis errors.

The dedicated patch would target v4.0 which does not have any ETA and which would require a huge amount of work in a majority of our components. Maybe this is something for the hacktoberfest?

src/AbstractPluginManager.php

src/Config.php

src/ConfigInterface.php

Ocramius · 2021-09-15T14:52:09Z

src/Factory/DelegatorFactoryInterface.php

@@ -23,6 +23,7 @@ interface DelegatorFactoryInterface
     * A factory that creates delegates of a given service
     *
     * @param  string             $name
+     * @psalm-param callable():mixed $callback
     * @param  null|array         $options
     * @return object


If this is supposed to return object, then $callback cannot return mixed.

I feel like the DelegatorFactoryInterface may benefit from a templated type, in future (not here, due to BC)

Since ContainerInterface#get can return mixed, this actually has to be mixed :-(

But yes, having a templated type would help for sure but until then, I've thought that mixed would be fine until the return type is object 🤷🏼‍♂️

Yeah, but the commented line is about @return object (wrong)

Not sure if I can follow.

Do you want me to change existing documentation that we end up having bc breaks? as of now, due to the lack of typing, callable factories can return mixed. When I enforce object now, this will be pain probably. Thats why I think that might be a BC break but I am not 100% sure tho.

Or do you want me to open up the return value of the delegator to be mixed?

I think I am fine with both as I don't have that much experience with adding psalm types on such a widely used component. As always, I trust your experience. :-)

src/ServiceManager.php

test/ConfigTest.php

test/ContainerTest.php

test/ServiceManagerTest.php

This reduces the overall baseline by 140 errors. Some errors still have to be suppressed inline. Signed-off-by: Maximilian Bösing <[email protected]>

Signed-off-by: Maximilian Bösing <[email protected]>

…e review Signed-off-by: Maximilian Bösing <[email protected]>

…thin a unit test Signed-off-by: Maximilian Bösing <[email protected]>

Signed-off-by: Maximilian Bösing <[email protected]>

…`factories` config entries Signed-off-by: Maximilian Bösing <[email protected]>

Signed-off-by: Maximilian Bösing <[email protected]>

…erface` Signed-off-by: Maximilian Bösing <[email protected]>

Signed-off-by: Maximilian Bösing <[email protected]>

This will prevent introducing BC breaks while still provide a soft return type. Signed-off-by: Maximilian Bösing <[email protected]>

Signed-off-by: Maximilian Bösing <[email protected]>

This will ensure the test does exactly what it supposed to do. Signed-off-by: Maximilian Bösing <[email protected]>

src/AbstractPluginManager.php

src/Config.php

Ocramius · 2021-09-18T11:26:59Z

src/Config.php

@@ -66,6 +67,8 @@ public function __construct(array $config = [])
                unset($config[$key]);
            }
        }
+
+        /** @psalm-suppress ArgumentTypeCoercion */


This coercion shouldn't apply anymore: merge() has the correct types? 🤔

ArrayUtils::merge returns array and thus, psalm loses track.

Not sure if something like this would help psalm to keep track:

/** * @template TArray1 of array * @template TArray2 of array * @param TArray1 $a * @param TArray2 $b * @return TArray1&TArray2 */

But this wont properly work for nested arrays (as they are here). There is absolutely no way but to trust that ArrayUtils::merge will return the same types.

Yes, but you are not calling ArrayUtils::merge(), but rather $this->merge(), which is well typed, and has its own suppressions

I'll leave this alone for now - I think it's not worth wasting effort on it after all the work you've already put in this, and it's in the internals anyway :)

I've added reportUnusedPsalmSuppress in the config and thats not reported so something might still be odd :-/

Ocramius · 2021-09-18T11:29:09Z

src/Factory/DelegatorFactoryInterface.php

@@ -23,6 +23,7 @@ interface DelegatorFactoryInterface
     * A factory that creates delegates of a given service
     *
     * @param  string             $name
+     * @psalm-param callable():mixed $callback
     * @param  null|array         $options
     * @return object


Yeah, but the commented line is about @return object (wrong)

src/PluginManagerInterface.php

src/ServiceManager.php

test/ContainerTest.php

Signed-off-by: Maximilian Bösing <[email protected]>

…eeded Signed-off-by: Maximilian Bösing <[email protected]>

Signed-off-by: Maximilian Bösing <[email protected]>

Ocramius · 2021-09-18T18:07:22Z

src/Config.php

@@ -66,6 +67,8 @@ public function __construct(array $config = [])
                unset($config[$key]);
            }
        }
+
+        /** @psalm-suppress ArgumentTypeCoercion */


I'll leave this alone for now - I think it's not worth wasting effort on it after all the work you've already put in this, and it's in the internals anyway :)

Ocramius · 2021-09-18T18:08:43Z

test/ServiceManagerTest.php

@@ -20,16 +21,23 @@
 use function get_class;

 /**
+ * @see ConfigInterface


I really like how you used @see to import types that are otherwise elided by the CS tooling :D

Dismissing feedback after multiple iterations highlighted a massive improvement in type detail/edge cases :)

Ocramius · 2021-09-18T18:10:20Z

🚢

will also release ASAP - it's a huge improvement :)

boesing added the Enhancement label Jul 31, 2021

boesing requested review from weierophinney and Ocramius July 31, 2021 20:21