Dependency injecting callable objects

[danslo]

Imagine having a third party library:

class MyLibrary {
    protected $callableClass;

    public function __construct(callable $callableClass) {
        $this->callableClass = $callableClass;
    }

    public function run() {
        call_user_func($this->callableClass);
    }
}

Now also imagine we define our own callable object:

class CallableClass {
    public function __invoke() {
        // We are now callable.
        echo 'Hello world';
    }
}

This works fine when injecting dependencies manually:

$lib = new MyLibrary(new CallableClass());
$lib->run();

However, this doesn't work when we use the Magento2 DI system:

<?xml version="1.0" encoding="UTF-8"?>
<config xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
        xsi:noNamespaceSchemaLocation="../../../../../lib/internal/Magento/Framework/ObjectManager/etc/config.xsd">
    <type name="MyLibrary">
        <arguments>
            <argument name="callableClass" xsi:type="object">CallableClass</argument>
        </arguments>
    </type>
</config>

Instead of an instance of CallableClass, Magento 2 will attempt to inject an array:

array (size=1)
      'instance' => string 'CallableClass' (length=13)

which obviously results in something bad:

Recoverable Error: Argument 1 passed to MyLibrary::__construct() must be callable, array given

I never gave you an array, I gave you an object!

I managed to fix this by checking for ReflectionParameter::isCallable in Magento\Framework\Code\Reader::getConstructor:

+++ lib/internal/Magento/Framework/Code/Reader/ClassReader.php
@@ -8,6 +8,23 @@ namespace Magento\Framework\Code\Reader;
 class ClassReader implements ClassReaderInterface
 {
     /**
+     * Determines type from reflection parameter.
+     *
+     * @param \ReflectionParameter $parameter
+     * @return string
+     */
+    protected function getParameterType(\ReflectionParameter $parameter)
+    {
+        $type = null;
+        if ($parameter->getClass() !== null) {
+            $type = $parameter->getClass()->getName();
+        } elseif ($parameter->isCallable()) {
+            $type = 'callable';
+        }
+        return $type;
+    }
+
+    /**
      * Read class constructor signature
      *
      * @param string $className
@@ -26,7 +43,7 @@ class ClassReader implements ClassReaderInterface
                 try {
                     $result[] = [
                         $parameter->getName(),
-                        $parameter->getClass() !== null ? $parameter->getClass()->getName() : null,
+                        $this->getParameterType($parameter),
                         !$parameter->isOptional(),
                         $parameter->isOptional()
                             ? ($parameter->isDefaultValueAvailable() ? $parameter->getDefaultValue() : null)

I am however having difficulties finding the appropriate place to create tests for this. Adding a unit test for this method will be easy, but the DI mechanism itself must be tested as well.

I also haven't exactly found why that object is being wrapped in an array. I assume this is default behavior for things it doesn't know (like callable)?

Input here would be much appreciated.

[antonkril]

Yes it doesn't instantiate things that have unknown type hinting. I't should be fixed, but it should also be fixed in compiler.

Did you try to run compiler?

We have integration tests: dev/tests/integration/Magento/Framework/ObjectManager

[joanhe]

@danslo Thanks for raising this issue. It will be great if you can submit a pull request. Unit test for this class can be added under directory: lib/internal/Magento/Framework/Code/Test/Unit/Reader.
Integration test can be added under directory: dev/tests/integration/testsuite/Magento/Framework/Code/Reader.

[danslo]

@joanhe @antonkril Yeah my diff was more meant as some insight, not as something expected to be merged. I need to do a bit more research before I feel confident to write some tests, maybe update the compiler, etc.

Give me a couple of days ;-)

[ilol]

Hi @danslo, do you have some updates?

[danslo]

@ilol I have a module of mine that depends on this functionality but I haven't gotten around to fixing it in Magento2 yet. I definitely do intend to come back to this soon though.

[danslo]

I will close this for now as I'm currently not finding time to look into this and I worked around this by using wrapper classes that just instantiates $handler['instance']. I'll probably come back to this later.

[ilol]

Ok, thank you, @danslo

[adragus-inviqa]

/cc #2026

[gabrieldagama]

Hi @danslo. Thank you for your report.
The issue has been fixed in #29715 by @Nazar65 in 2.4-develop branch
Related commit(s):

• 771dc31f5ef1236db3ac54c3062f663cfee7d8b0
• dd9bdda933e36b519a7f0a8d52579bfb48bf2a33
• 989139f0fc77bbb1c1eac28985955145b83ae3fb
• 239314d98653081f02752ee5742136b1f3ba07e2
• eaa20f2fe3e097b21fe31b5fd5e51c6b7d2c7fac
• 8cdcefd99c6d8d5ac20d6d430302eac97aaed6f2
• 43fd0573739a014277f99141f0cc58376fc8217a
• dfa67b0d6c6dd4050ad4b21b2f7c23c81ef8b7e5
• 8ef07f464a550f54d14428c438429a1df5597e1b
• 3af84161abd031f4bb9054f1efbcc4439efc9be5
• 7940f5952380c9f239b43940c6d607c9d2872177
• e690a8d113389a05659c83b0bf75f1dcf49186b8
• 4c085d7504691c2ac458edb47a73f8b8cf16c4a2
• 0e28c9181301034653ac1dc8a26bc728a36fcc3b
• 80dae458e82ab8f55f502d5149bddfae8c722b99
• 67636e7e840923ddbe7cf58812a553239367b59e
• 739e6dc64cff5b7c90f219c103a7469f3dbcf071
• 68bc010fc93a332082cfee97b700fa689161dd84
• 0ad0630ee1c426be5468fa223ecd012f06d339b1
• 9ccfdc9ca4eb2f20ad9ac1b40f999f3f302c9951
• 7b58d977248acce8c373e31d20e909a886adde61
• 90c9dac7173cc7755daf94eef37673473c97fed7
• 88a01504901fd477b0d880a6208475172747d467
• 748eb4f13da880c022203fca256a1cfebd96b778
• b3d52559d3529b4ad1f8d28dfb200d6c579a94e7
• 0a6156f038ff20ba31ada254baf0855b4f045f9d
• 253bafbc254692717a8e090b67da129a72b4f061
• 2448fd80391fb14d184bf9d2490587aeb15c9755
• cf288e60225e79e236455f632c0f3872e91e9060
• 7296cdfd222561023571c1a6294115a0d5d512e2

The fix will be available with the upcoming 2.4.2 release.