Full support for PHP 7.4 property type declarations #2114

bwaidelich · 2020-09-08T16:29:22Z

Description

Since version 7.4 PHP supports type declarations, see RFC but we don't respect those in our ReflectionService, so the following won't work:

class SomeClass
{
    /**
     * @Flow\Inject
     **/
    protected ?LoggerInterface $logger = null;
}

bwaidelich · 2020-09-08T16:38:38Z

Something like this should do the trick (still missing backwards compatibility for PHP < 7.4):

Index: Neos.Flow/Classes/Reflection/ReflectionService.php
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- Neos.Flow/Classes/Reflection/ReflectionService.php	(date 1599581621814)
+++ Neos.Flow/Classes/Reflection/ReflectionService.php	(date 1599581621814)
@@ -89,6 +89,7 @@
     const DATA_METHOD_DECLARED_RETURN_TYPE = 25;
     const DATA_PROPERTY_TAGS_VALUES = 14;
     const DATA_PROPERTY_ANNOTATIONS = 15;
+    const DATA_PROPERTY_TYPE = 16;
     const DATA_PROPERTY_VISIBILITY = 24;
     const DATA_PARAMETER_POSITION = 16;
     const DATA_PARAMETER_OPTIONAL = 17;
@@ -977,6 +978,11 @@
         return (isset($this->classReflectionData[$className][self::DATA_CLASS_PROPERTIES][$propertyName][self::DATA_PROPERTY_TAGS_VALUES][$tag])) ? $this->classReflectionData[$className][self::DATA_CLASS_PROPERTIES][$propertyName][self::DATA_PROPERTY_TAGS_VALUES][$tag] : [];
     }
 
+    public function getPropertyType($className, $propertyName)
+    {
+        return $this->classReflectionData[$className][self::DATA_CLASS_PROPERTIES][$propertyName][self::DATA_PROPERTY_TYPE] ?? null;
+    }
+
     /**
      * Tells if the specified property is private
      *
@@ -1280,6 +1286,9 @@
 
         $visibility = $property->isPublic() ? self::VISIBILITY_PUBLIC : ($property->isProtected() ? self::VISIBILITY_PROTECTED : self::VISIBILITY_PRIVATE);
         $this->classReflectionData[$className][self::DATA_CLASS_PROPERTIES][$propertyName][self::DATA_PROPERTY_VISIBILITY] = $visibility;
+        if ($property->hasType()) {
+            $this->classReflectionData[$className][self::DATA_CLASS_PROPERTIES][$propertyName][self::DATA_PROPERTY_TYPE] = (string)$property->getType();
+        }
 
         foreach ($property->getTagsValues() as $tagName => $tagValues) {
             $tagValues = $this->reflectPropertyTag($className, $property, $tagName, $tagValues);
@@ -1289,7 +1298,7 @@
             $this->classReflectionData[$className][self::DATA_CLASS_PROPERTIES][$propertyName][self::DATA_PROPERTY_TAGS_VALUES][$tagName] = $tagValues;
         }
 
-        foreach ($this->annotationReader->getPropertyAnnotations($property, $propertyName) as $annotation) {
+        foreach ($this->annotationReader->getPropertyAnnotations($property) as $annotation) {
             $this->classReflectionData[$className][self::DATA_CLASS_PROPERTIES][$propertyName][self::DATA_PROPERTY_ANNOTATIONS][get_class($annotation)][] = $annotation;
         }
 
Index: Neos.Flow/Classes/ObjectManagement/Configuration/ConfigurationBuilder.php
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- Neos.Flow/Classes/ObjectManagement/Configuration/ConfigurationBuilder.php	(date 1599581727458)
+++ Neos.Flow/Classes/ObjectManagement/Configuration/ConfigurationBuilder.php	(date 1599581727458)
@@ -555,7 +555,13 @@
                 if (!array_key_exists($propertyName, $properties)) {
                     /** @var Inject $injectAnnotation */
                     $injectAnnotation = $this->reflectionService->getPropertyAnnotation($className, $propertyName, Inject::class);
-                    $objectName = $injectAnnotation->name !== null ? $injectAnnotation->name : trim(implode('', $this->reflectionService->getPropertyTagValues($className, $propertyName, 'var')), ' \\');
+                    $objectName = $injectAnnotation->name;
+                    if ($objectName === null) {
+                        $objectName = $this->reflectionService->getPropertyType($className, $propertyName);
+                    }
+                    if ($objectName === null) {
+                        $objectName = trim(implode('', $this->reflectionService->getPropertyTagValues($className, $propertyName, 'var')), ' \\');
+                    }
                     $configurationProperty =  new ConfigurationProperty($propertyName, $objectName, ConfigurationProperty::PROPERTY_TYPES_OBJECT, null, $injectAnnotation->lazy);
                     $properties[$propertyName] = $configurationProperty;
                 }

kdambekalns · 2020-09-08T16:46:28Z

But be careful when using typed properties: They cannot be lazy (the proxy never fits the required type) and they must not be accessed before being initialized. Both facts represent problems, as I learned in a project (when using @var with typed properties).

In short: There's more needed than simply reading the intended type from the declaration.

bwaidelich · 2020-09-08T17:19:31Z

Thanks for the heads-up!

There's more needed than simply reading the intended type from the declaration

You're right, I updated the example

albe · 2020-09-25T17:12:58Z

They cannot be lazy (the proxy never fits the required type)

Sure?
https://3v4l.org/8R1vD

kdambekalns · 2020-09-26T15:36:17Z

The LazyLoadingProxy, not the „usual“ proxy…

…

Am 25.09.2020 um 19:13 schrieb Alexander Berl ***@***.***>: They cannot be lazy (the proxy never fits the required type) Sure? https://3v4l.org/8R1vD — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe.

bwaidelich · 2020-10-13T14:10:01Z

This is the case Karsten was referring to (I assume): https://3v4l.org/4UtYu

-> The properties that are injected via Inject annotation need to be nullable: https://3v4l.org/BsgHD

kdambekalns · 2020-10-14T08:40:25Z

This is the case Karsten was referring to (I assume):

Well, that's one thing. The other is that for a lazy-loading objects we use a DependcyProxy: https://github.com/neos/flow-development-collection/blob/master/Neos.Flow/Classes/ObjectManagement/DependencyInjection/DependencyProxy.php

albe · 2021-03-07T12:38:49Z

Folks, can we do anything to progress this? i.e. start with a PR with the above diff and start adding helpful error message during compile step with injections into typed properties?

albe · 2021-04-11T12:22:26Z

Thinking some more about this, the issue with lazy DependencyProxy and typehints is, that the proxy does not extend the actual class. We could solve that by

building DI proxies for the injected classes like we do for the classes to be injected into, but add another implementation FooClassLazyProxy extends FooClass [extends FooClass_Original] that will behave much like the current LazyDependencyProxy
using ocramius/proxymanager LazyLoadingValueHolderProxy

The latter would complement #1835, which would also require our current DI proxies (the classes that are injected into) to be changed to use proxymanager (or use a proper PHP parser).

Drawback: we'd depend on proxymanager/lamanias-code to add support for e.g. new PHP8 features, but OTOH we gain some additional developer power in that department.

kdambekalns · 2021-07-27T09:26:39Z

Having the proxies fulfill instanceof <ProxiedClas> would be awesome. And with ocramius I wouldn't fear being stuck without support for new PHP versions, ever. So, yes, why not.

robertlemke · 2021-11-05T06:22:17Z

I'd also like to have a look at this, hopefully during the sprint next week in Dresden.

albe · 2021-11-11T11:38:48Z

As agreed upon during the sprint meeting, we will go with solution 1 for the reasons given in #1835 (comment)

Things noticed during the call:

we want to keep all the power of our current proxies, with new support and all the AOP
the proxy constructor can have a different signature than the actual class - unless it implements an interface with a constructor declaration (which is a bad idea in general) - so the proxy can be instanciated easily
proxy needs to implement all (public?) methods of original class to trigger activation correctly, can not replace itself then, but you shouldn't keep holding a reference to the proxy
public properties on the original class would only work if we could make them inaccessible and use __get magic, but we can't override the visibility to private in the proxy class

Robert has a base that he will create a PR for next and then we can work it out.

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

robertlemke · 2022-02-22T17:29:28Z

Good news, I think I have solved it (see related pull request).

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

robertlemke · 2022-03-28T07:41:50Z

In order to find a shortcut, I tried @bwaidelich's original suggestion again. But even if I disable lazy injection completely, I still get an error:

Typed property Acme\LazyTest\Command\TestCommandController_Original::$logger must not be accessed before initialization

  Type: Error
  File: Data/Temporary/Development/Cache/Code/Flow_Object_Classes/Acme_LazyTest_Com
        mand_TestCommandController.php
  Line: 36

Relevant code:

    /**
     * @Flow\Inject(lazy=false)
     */
    protected ?TestSingleton $testSingleton = null;

bwaidelich · 2022-03-28T07:46:59Z

You get the error with the = null initialization??

    /**
     * @Flow\Inject(lazy=false)
     */
    protected ?TestSingleton $testSingleton;

should even work if the dependency is resolved within the constructor call

robertlemke · 2022-03-28T07:48:34Z

Ah, sorry, it's the other property, logger:

    /**
     * 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
     **/
    protected LoggerInterface $logger;

    /**
     * @Flow\Inject(lazy=false)
     */
    protected ?TestSingleton $testSingleton = null;

robertlemke · 2022-03-28T07:51:30Z

It works this way:

    /**
     * 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(lazy=false)
     **/
    protected ?LoggerInterface $logger = null;

    /**
     * @Flow\Inject(lazy=false)
     */
    protected ?TestSingleton $testSingleton = null;

kitsunet · 2022-03-28T07:53:51Z

Interesting, how does the resulting proxy code look like?

mhsdesign · 2022-03-28T07:54:46Z

can we maybe work around that the lazy proxy needs to fulfill instanceof <ProxiedClass>

protected TestSingleton|Flow\Lazy $testSingleton;

bwaidelich · 2022-03-28T08:00:18Z

@mhsdesign that's what I suggested as well https://neos-project.slack.com/archives/C050KKBEB/p1648453577172009?thread_ts=1648224274.563979&cid=C050KKBEB :)

But IMO

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

should work, too (without proxy). @robertlemke in your example no inject annotation exists!?

kitsunet · 2022-03-28T08:02:00Z

should work, too (without proxy). @robertlemke in your example no inject annotation exists!?

oooh, good catch, is that injected via inject annotation?

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:04:06Z

I created a separate pull request (see above). Let's check if that is sufficient for now.

@var

FEATURE: Type declaration support for property injection 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 is disabled when a type is declared. To still use Lazy Property Injection, don't use a PHP type declaration but a `@var` annotation: ```php /** * @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

robertlemke · 2022-03-28T09:41:06Z

Alright, I guess we can close this and tackle lazy injection support for typed properties in a separate issue.

bwaidelich added Feature P: Flow T: PHP labels Sep 8, 2020

albe mentioned this issue Oct 12, 2021

Full PHP8 typesafe injectable properties #2601

Closed

robertlemke mentioned this issue Feb 21, 2022

FEATURE: Lazy injections for typed properties #2701

Draft

robertlemke linked a pull request Feb 22, 2022 that will close this issue

FEATURE: Lazy injections for typed properties #2701

Draft

robertlemke self-assigned this Feb 22, 2022

robertlemke changed the title ~~Fully support for PHP 7.4 property type declarations~~ Full support for PHP 7.4 property type declarations Feb 24, 2022

robertlemke mentioned this issue Mar 28, 2022

FEATURE: Type declaration support for property injection #2782

Merged

robertlemke closed this as completed Mar 28, 2022

This was referenced May 18, 2022

FEATURE: Support object serialization without @var annotation #2843

Closed

FEATURE: Support object serialization without var annotation #2844

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Full support for PHP 7.4 property type declarations #2114

Full support for PHP 7.4 property type declarations #2114

bwaidelich commented Sep 8, 2020 •

edited

Loading

bwaidelich commented Sep 8, 2020

kdambekalns commented Sep 8, 2020

bwaidelich commented Sep 8, 2020

albe commented Sep 25, 2020

kdambekalns commented Sep 26, 2020 via email

bwaidelich commented Oct 13, 2020

kdambekalns commented Oct 14, 2020

albe commented Mar 7, 2021

albe commented Apr 11, 2021

kdambekalns commented Jul 27, 2021

robertlemke commented Nov 5, 2021

albe commented Nov 11, 2021

robertlemke commented Feb 22, 2022 •

edited

Loading

robertlemke commented Mar 28, 2022

bwaidelich commented Mar 28, 2022

robertlemke commented Mar 28, 2022

robertlemke commented Mar 28, 2022

kitsunet commented Mar 28, 2022

mhsdesign commented Mar 28, 2022

bwaidelich commented Mar 28, 2022

kitsunet commented Mar 28, 2022

robertlemke commented Mar 28, 2022

robertlemke commented Mar 28, 2022

Full support for PHP 7.4 property type declarations #2114

Full support for PHP 7.4 property type declarations #2114

Comments

bwaidelich commented Sep 8, 2020 • edited Loading

Description

bwaidelich commented Sep 8, 2020

kdambekalns commented Sep 8, 2020

bwaidelich commented Sep 8, 2020

albe commented Sep 25, 2020

kdambekalns commented Sep 26, 2020 via email

bwaidelich commented Oct 13, 2020

kdambekalns commented Oct 14, 2020

albe commented Mar 7, 2021

albe commented Apr 11, 2021

kdambekalns commented Jul 27, 2021

robertlemke commented Nov 5, 2021

albe commented Nov 11, 2021

robertlemke commented Feb 22, 2022 • edited Loading

robertlemke commented Mar 28, 2022

bwaidelich commented Mar 28, 2022

robertlemke commented Mar 28, 2022

robertlemke commented Mar 28, 2022

kitsunet commented Mar 28, 2022

mhsdesign commented Mar 28, 2022

bwaidelich commented Mar 28, 2022

kitsunet commented Mar 28, 2022

robertlemke commented Mar 28, 2022

robertlemke commented Mar 28, 2022

bwaidelich commented Sep 8, 2020 •

edited

Loading

robertlemke commented Feb 22, 2022 •

edited

Loading