FEATURE: Lazy injections for typed properties

[robertlemke]

This change adjusts the proxy building and dependency injection process to properly handle PHP 7.4 class property type declarations.

It resolves #2114
Also resolve #2601

How to verify it

What did not work before this change was using property type declarations for lazily injected properties.

Consider the following code: injecting $testSingleton is what this fix makes possible.

<?php
namespace Acme\LazyTest\Command;

use Acme\LazyTest\TestSingleton;
use Neos\Flow\Annotations as Flow;
use Neos\Flow\Cache\CacheManager;
use Neos\Flow\Cli\CommandController;
use Psr\Log\LoggerInterface;

/**
 * @Flow\Scope("singleton")
 */
class TestCommandController extends CommandController
{
    /**
     * LoggerInterface doesn't have to be nullable, because the proxy class
     * builder will not even try to use lazy injection, since it is an interface
     *
     * @Flow\Inject
     **/
    protected LoggerInterface $logger;

    /**
     * CacheManager must be nullable, because it can be injected lazily
     *
     * @Flow\Inject
     */
    protected ?CacheManager $cacheManager;

    /**
     * @Flow\Inject
     */
    protected ?TestSingleton $testSingleton;

    /**
     * Test command
     *
     * @return void
     */
    public function testCommand(): void
    {
        # Will be the actual logger, because an interface is injected,
        # which results in an eager dependency:

        var_dump(get_class($this->logger)); # Neos\Flow\Log\Psr\Logger

        # Will be the actual Cache Manager, because as soon as the
        # Cache Manager is used (instantiated) elsewhere, it is also
        # injected eagerly into this controller:

        var_dump(get_class($this->cacheManager));                      # Neos\Flow\Cache\CacheManager

        # The TestSingleton is not injected into any other class and
        # therefore is a real Lazy Proxy before it is used for the
        # first time by calling hello():

        var_dump(get_class($this->testSingleton));                     # Acme\LazyTest\TestSingleton_LazyProxy
        var_dump($this->testSingleton instanceof TestSingleton); # bool(true)
        $this->testSingleton->hello('world');
        var_dump(get_class($this->testSingleton));                     # Acme\LazyTest\TestSingleton
    }
}

In order to verify this change, you can use the test package (see zip file) and play a bit with the injected properties. Things to try out (at least):

• verify the output in Development context after initial run of ./flow test:test with an empty cache
• verify the output on a second rund
• verify that it works when you make a little change, so that proxy classes are built
• check that all this works in Production

Acme.LazyTest.zip

[robertlemke]

The code works now and should solve the problem at hand.

What's still missing are the respective test adjustments.

[robertlemke]

This is now ready for reviews.

[robertlemke]

Hmm, that's weird – all functional and unit tests succeeded in Phpstorm using PHP 7.4.

[robertlemke]

Sigh, PHP 7.3 fails. So I either need to scrub this more, so that it doesn't fail with PHP 7.3, or don't release this in Flow 7.3 but only 8.0.

Let's discuss this here: https://discuss.neos.io/t/rfc-drop-php-7-3-support-in-all-future-flow-versions/5844

[albe]

I'll take a look at the 7.3 failures this evening. Looks as if the generated code for the proxies is just slightly off, though not sure why only on 7.3? Maybe a different laminas version is installed there with slightly different behaviour?

[robertlemke]

This change is now targeted and rebased for master.

[robertlemke]

Thanks, @kdambekalns!

At this point all pending questions are answered and code suggestions were committed.

[mhsdesign]

@robertlemke i really want to approve this since the syntax is much much much much fancier ... and cooler ^^

but i found out in my tests that this really increases the time to get flow ready from 0 - 100.
eg. its common practice to do a rm -rf Data/Temporary or ./flow flow:cache:flush every once and then if something isnt working that great and since caches often dont cover every scenario when they should be invalidated.

so i directly noticed a performance increase while doing functional Fusion tests (i was trying out your new functionality there)
from a flushed cache it took flow@master ~11.5 seconds to start the bootstrap and run one test.
with flow@robert it took ~ 18 seconds...

to make this more comparable i flushed the caches and manually timed how long ./flow flow:core:compile took (made a few iterations, result is pie mal daumen):

	flow@master	flow@robert
./flow flow:core:compile	2.6s	5s

[mhsdesign]

imagine i have this code: (DslFactory is a singelton)

    /**
     * @Flow\Inject
     * @var DslFactory
     */
    protected $dslFactory;

i now want to use typed properties, so i naively do so first:

    /**
     * @Flow\Inject
     */
    protected DslFactory $dslFactory;

and get some cryptic exception about: code line:

Cannot access uninitialized non-nullable property Neos\Fusion\Core\Parser_Original::$dslFactory by reference
DependencyInjection/PropertyInjectionTrait.php:32

then i go back to your nice PR body and ahh i need to use ? so i do so.

    /**
     * @Flow\Inject
     */
    protected ?DslFactory $dslFactory;

i think we should have a better exception message than the of php. -> thats easy.

but the problem is my code doesnt work now directly, i needed to flush the caches - what takes some time longer and then it works.

---

in short:

do this:

    /**
     * @Flow\Inject
     * @var DslFactory
     */
    protected $dslFactory;

run flow ...

    /**
     * @var DslFactory
     */
    protected $dslFactory;

run flow ... (doesnt work)

    /**
     * @Flow\Inject
     * @var DslFactory
     */
    protected $dslFactory;

run flow ... flow@master works here, but flow@robert doesnt... in my case the DslFactory had no working @Flow\InjectConfiguration and the $objectManger wasnt injected too (so no working DI) after a subtle change in the DslFactory File the injection was working again too (or after cache flush).

[mhsdesign]

so at the moment this feels sometimes buggy - just go around fx. in the neos dev collection and try to change to typed properties here and there -> you will often find yourself in need to make a cache flush so the proxybuilding is properly working again.

but @robertlemke i would like to help tommorow to make this working!

[robertlemke]

@mhsdesign very good catch!

Here are my timings for comparison:

	flow@master	flow@robert
neos/flow unit tests	780ms	782ms
neos/flow functional tests	46s	46s
neos/flow functional tests (only object management)	115ms	112ms
rm -rf Data/Temporary && time ./flow	1.39s	2.4s ⚠️

I did a quick first profiling and apparently, the Reflection Service is the culprit. From a total wall time of 15.2 seconds (it takes longer because of the profiling engine), my branch is more than 6 seconds slower while reflecting classes.

I'll take a closer look and see where code can be optimized.

[robertlemke]

As I feared … the slow part is the new Laminas Class Building code, which doesn't use our optimized Reflection Service. Let's see if I can work around this a bit.

[robertlemke]

If someone is reviewing this right now – please postpone your review a little. I need to rework some of the internals due to the performance issues.

[mhsdesign]

do i see this right that using Laminas is not crucial and just bewares us from manually string concatenating everything together?

[robertlemke]

do i see this right that using Laminas is not crucial and just bewares us from manually string concatenating everything together?

Mostly, yes. But it's also a handy API which allows you to pass an existing class reflection and then modify code through certain setters. The idea was to transition to a library (Laminas) instead of doing all that on our own (like we did before). But unfortunately, it doesn't work (fast enough) out of the box.

[robertlemke]

PS: There are more problems than that, because the Laminas Code Builder tries to use our doc comment type hints in certain cases, which may be invalid, like in this example:

I'll work around that, but you see, it's a diligent but boring piece of work …

[mhsdesign]

We dont allow prototypes lazly injected - but on my pc there are tousands of lazy proxies for prototypes generated ... isnt this a waste of performance?

[albe]

Yes, definitely a thing to optimize before merging.

[mhsdesign]

this doesnt work for static methods ... (Using '$this' when not in object context)

noticed when looking at some random proxy files:

    public static function renderStatic(array $arguments, \Closure $renderChildrenClosure, \Bar $bar)
    {
        $arguments = func_get_args();
            return $this->_activateDependency()->renderStatic(...$arguments);
    }

what should we do in that case?
this?

    public static function renderStatic(array $arguments, \Closure $renderChildrenClosure, \Bar $bar)
    {
        $arguments = func_get_args();
            return self::renderStatic(...$arguments);
    }

[mhsdesign]

do we allow with this change less lazy dependency injection that before?
related: interfaces fx. cant be injected lazily anymore? #2701 (comment)

[albe]

No, it just make sure that if a dependency can not be injected lazily (createLazyDependency returns null), it falls back to eager injection

[mhsdesign]

i finally kindof found the reson for this buggy behavior that on the fly di changes dont work correctly:
#2701 (review)

it seems that this method createLazyDependency is returning null and the next call: $staticObjectManager->get($propertyObjectName);

doesnt return an object of the proxied class but rather an object based of the class in my working dir...

[mhsdesign]

so #2701 (review) even shorter:
go to a lazy dependency injection (in the old doc comment style - so you can compare this exact behaviour with flow@master)

• remove the @Flow\Inject and run a flow
• add the @Flow\Inject again and run flow again -> now the property should be injected.

but at flow@robert as i seems this code:

flow-development-collection/Neos.Flow/Classes/ObjectManagement/ObjectManager.php

Line 420 in aea746a

// If proxy classes were partially flushed in Development context, it might happen that

is triggered but flow@master is working in this scenario

[jonnitto]

I think I can't help much, but I want to thank you for all your effort you put into this ❤️

[mhsdesign]

@fcool thought me that it works already like this:

#[Flow\Inject(name: BlaInterface::class, lazy: true)]
protected BlaInterface|DependencyProxy|null $bla;

it would be great of course if i could leave out the (redundant) name: BlaInterface::class
(and idk if lazy: true is needed i dont remember ^^)

[mficzel]

I kindoff like this style. Makes the lazyness very explicit and opt in. For me it would be fine to document this.

[mhsdesign]

I kindoff like this style. Makes the lazyness very explicit and opt in. For me it would be fine to document this.

me too (especially cases, where youd need to pass the dependency it becomes clear why youd need to activate it first), but i would like to get rid of the extra arguments so we can write:

#[Flow\Inject()]
protected BlaInterface|DependencyProxy|null $bla;