FEATURE: Type declaration support for property injection #2782

robertlemke · 2022-03-28T08:03:40Z

This change introduces basic support for PHP 7.4 class property types
for property injection.

It allows for using types as follows:

/**
 * @Flow\Inject
 **/
protected LoggerInterface $logger;

In order to allow this syntax, Lazy Property Injection is disabled
when a type is declared. To still use Lazy Property Injection, don't
use a PHP type declaration but a @var annotation:

/**
 * @Flow\Inject
 * @var LoggerInterface
 **/
protected $logger;

If using both (type declaration and @var), it is handled non-lazily.

Lazy Property Injection on properties with type declarations may
be re-implemented as part of a future release.

#2114

This change introduces basic support for PHP 7.4 class property types for property injection. It allows for using types as follows: ```php /** * @flow\Inject **/ protected ?LoggerInterface $logger; ``` In order to allow this syntax, Lazy Property Injection was disabled completely. It may be re-implemented as part of a future release. neos#2114

robertlemke · 2022-03-28T08:09:45Z

This change is a shortcut / alternative to #2701

bwaidelich · 2022-03-28T08:09:45Z

Awesome!
I don't (yet) understand the full code, but I can confirm that this fixes injection with type hints (and without @var annotation).

I would suggest, not to communicate the nullable version though but:

/**
 * @Flow\Inject
 */
protected LoggerInterface $logger;

to make code more resilient and clarify that this is currently not lazily injected.

This is great, and will bring me back to using inject annotations in application code. Thanks! <3

bwaidelich

+1 by manual testing (not so much by reading yet)

Neos.Flow/Classes/ObjectManagement/Configuration/ConfigurationBuilder.php

kitsunet · 2022-03-28T08:18:55Z

I am not sure why we would sacrifice one feature (lazy injections) for another (PHP 7.4 property type hint support).

bwaidelich · 2022-03-28T08:23:38Z

I am not sure why we would sacrifice one feature (lazy injections) for another (PHP 7.4 property type hint support).

Hang on. I expected this to work as before if I decide to omit the type hint (which is still allowed in all PHP versions)
If this disables lazy DI in any case I would change my vote to -1

Sorry, I misinterpreted the change

kitsunet

Cool! This seems like a sensible tradeoff I guess, will be interesting to compare performance of a code base written with property type hints and one without.

albe

Looks good by reading. Only left a comment for type-hinting the new ReflectionService method

Neos.Flow/Classes/Reflection/ReflectionService.php

kdambekalns

Awesome step into the right direction!

kdambekalns · 2022-03-28T09:20:08Z

Neos.Flow/Classes/Reflection/ReflectionService.php

@@ -1315,7 +1331,7 @@ public function reflectClassProperty($className, PropertyReflection $property)
            $this->classReflectionData[$className][self::DATA_CLASS_PROPERTIES][$propertyName][self::DATA_PROPERTY_TAGS_VALUES][$tagName] = $tagValues;
        }

-        foreach ($this->annotationReader->getPropertyAnnotations($property, $propertyName) as $annotation) {


Nice one… that parameter was passed since 2013 but not used? Ever? 🤪

albe · 2022-03-28T09:23:52Z

Another question:

It allows for using types as follows:

/**
 * @Flow\Inject
 **/
protected ?LoggerInterface $logger;

What was the exact reason the nullability was required? It wasn't just the lazyness, because that would mean we don't need it for typed properties since the injection is eager (for now).
"Typed property {x} must not be accessed before initialization" was the PHP error and it's happening when a typed property is accessed that wasn't initialized (in the constructor or declaration). So my question is: do we somewhere in the proxy code or somewhere else attempt to bypass the constructor, or not set the property in the constructor?
Does anyone have the full proxy code for such a class around right now?

kdambekalns · 2022-03-28T09:24:04Z

I adjusted the PR description, is that fine for you, @bwaidelich and @robertlemke?

bwaidelich

Love it, and I can confirm that

    /**
     * @Flow\Inject
     * @var LoggerInterface
     */
    protected $logger;

injects the DependencyProxy and

    /**
     * @Flow\Inject
     * @var LoggerInterface
     */
    protected LoggerInterface $logger;

(or without the @var annotation) injects the actual logger instance!

Thank you so much! <3

bwaidelich · 2022-03-28T09:27:22Z

@kdambekalns

To still use Lazy Property Injection, declare the type as nullable

That's not correct though. When the type hint is specified, we now always fall back to eager injection.

robertlemke · 2022-03-28T09:31:09Z

What was the exact reason the nullability was required? It wasn't just the lazyness, because that would mean we don't need it for typed properties since the injection is eager (for now).
"Typed property {x} must not be accessed before initialization" was the PHP error and it's happening when a typed property is accessed that wasn't initialized (in the constructor or declaration). So my question is: do we somewhere in the proxy code or somewhere else attempt to bypass the constructor, or not set the property in the constructor?
Does anyone have the full proxy code for such a class around right now?

The error should only occur with lazy loading, so nullability is not required at the moment. And when we have support for union types, it will probably not even be allowed or at least does not make sense.

robertlemke · 2022-03-28T09:33:07Z

@kdambekalns @bwaidelich I updated the description to correct that.

kdambekalns · 2022-03-28T09:34:44Z

@albe asked

What was the exact reason the nullability was required?

It was needed for protected ?LoggerInterface $logger = null; – something not in the example. Obviously I thought about that but missed that this isn't needed anymore…

kdambekalns · 2022-03-28T09:37:02Z

I added a hint on using type and @var to the description. Just to be very clear.

robertlemke · 2022-03-28T09:38:05Z

So, we're all set?

albe · 2022-03-28T09:38:32Z

So who wants to have the honor of pushing the merge button on this PR we have been all longing for? :) @robertlemke maybe, as he has put in all the hard grunt work.
We (Martin and I) will then branch off 8.0 this (late) afternoon

robertlemke · 2022-03-28T09:39:24Z

So who wants to have the honor of pushing the merge button on this PR we have been all longing for? :) @robertlemke maybe, as he has put in all the hard grunt work.
We (Martin and I) will then branch off 8.0 this afternoon

LOL, this PR is just a couple of minute's work ;-) Anyway, I'll take the chance.

albe · 2022-03-28T09:49:11Z

LOL, this PR is just a couple of minute's work ;-) Anyway, I'll take the chance.

It sometimes requires a lifetime of work to finally come to the solution that only takes 5 minutes worth of work. That doesn't devalue the lifetime spent before :)

bwaidelich previously approved these changes Mar 28, 2022

View reviewed changes

kitsunet reviewed Mar 28, 2022

View reviewed changes

Neos.Flow/Classes/ObjectManagement/Configuration/ConfigurationBuilder.php Outdated Show resolved Hide resolved

Disable lazy DI only if class property type is used

1ef7376

kitsunet approved these changes Mar 28, 2022

View reviewed changes

robertlemke requested a review from bwaidelich March 28, 2022 08:45

robertlemke changed the title ~~!!! Basic support for PHP 7.4 class property types~~ Basic support for PHP 7.4 class property types Mar 28, 2022

albe approved these changes Mar 28, 2022

View reviewed changes

Neos.Flow/Classes/Reflection/ReflectionService.php Show resolved Hide resolved

robertlemke changed the title ~~Basic support for PHP 7.4 class property types~~ Type declaration support for property injection Mar 28, 2022

kdambekalns approved these changes Mar 28, 2022

View reviewed changes

bwaidelich approved these changes Mar 28, 2022

View reviewed changes

albe changed the title ~~Type declaration support for property injection~~ FEATURE: Type declaration support for property injection Mar 28, 2022

robertlemke merged commit 2abbebf into neos:master Mar 28, 2022

robertlemke deleted the class-property-fixes-shortcut branch March 28, 2022 09:40

albe mentioned this pull request Apr 3, 2022

Full PHP8 typesafe injectable properties #2601

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FEATURE: Type declaration support for property injection #2782

FEATURE: Type declaration support for property injection #2782

robertlemke commented Mar 28, 2022 •

edited by kdambekalns

Loading

robertlemke commented Mar 28, 2022

bwaidelich commented Mar 28, 2022

bwaidelich left a comment

kitsunet commented Mar 28, 2022

bwaidelich commented Mar 28, 2022

kitsunet left a comment

albe left a comment

kdambekalns left a comment

kdambekalns Mar 28, 2022

albe commented Mar 28, 2022

kdambekalns commented Mar 28, 2022

bwaidelich left a comment

bwaidelich commented Mar 28, 2022

robertlemke commented Mar 28, 2022

robertlemke commented Mar 28, 2022

kdambekalns commented Mar 28, 2022

kdambekalns commented Mar 28, 2022

robertlemke commented Mar 28, 2022

albe commented Mar 28, 2022 •

edited

Loading

robertlemke commented Mar 28, 2022

albe commented Mar 28, 2022

FEATURE: Type declaration support for property injection #2782

FEATURE: Type declaration support for property injection #2782

Conversation

robertlemke commented Mar 28, 2022 • edited by kdambekalns Loading

robertlemke commented Mar 28, 2022

bwaidelich commented Mar 28, 2022

bwaidelich left a comment

Choose a reason for hiding this comment

kitsunet commented Mar 28, 2022

bwaidelich commented Mar 28, 2022

kitsunet left a comment

Choose a reason for hiding this comment

albe left a comment

Choose a reason for hiding this comment

kdambekalns left a comment

Choose a reason for hiding this comment

kdambekalns Mar 28, 2022

Choose a reason for hiding this comment

albe commented Mar 28, 2022

kdambekalns commented Mar 28, 2022

bwaidelich left a comment

Choose a reason for hiding this comment

bwaidelich commented Mar 28, 2022

robertlemke commented Mar 28, 2022

robertlemke commented Mar 28, 2022

kdambekalns commented Mar 28, 2022

kdambekalns commented Mar 28, 2022

robertlemke commented Mar 28, 2022

albe commented Mar 28, 2022 • edited Loading

robertlemke commented Mar 28, 2022

albe commented Mar 28, 2022

robertlemke commented Mar 28, 2022 •

edited by kdambekalns

Loading

albe commented Mar 28, 2022 •

edited

Loading