TASK: Support PHP8 types and class property promotion #2404

albe · 2021-03-01T16:22:23Z

This change makes sure that PHP8 union types, nullable types and promoted properties can be used with proxied classes.

Depends on #2287
Related to #2468
Related to #2233

albe · 2021-03-01T16:24:44Z

Apparently StyleCI does not yet support PHP 8 syntax 🤷‍♂️

Neos.Utility.ObjectHandling/Classes/TypeHandling.php

Neos.Flow/Tests/Functional/ObjectManagement/Fixtures/ClassWithUnionTypes.php

.github/workflows/build.yml

Neos.Flow/Classes/ObjectManagement/Proxy/ProxyClass.php

Neos.Flow/Classes/ObjectManagement/Proxy/ProxyMethod.php

Neos.Flow/Classes/ObjectManagement/CompileTimeObjectManager.php

albe · 2021-03-22T18:54:33Z

Neos.Flow/Classes/ObjectManagement/Proxy/Compiler.php

+        if (count($attribute->getArguments()) > 0) {
+            $argumentsAsString = [];
+            foreach ($attribute->getArguments() as $argumentName => $argumentValue) {
+                $renderedArgumentValue = var_export($argumentValue, true);


Wondering why we didn't use var_export() in the other method where we render an array(x => y, ...) manually?

Which "other method" do you mean?

Neos.Flow/Classes/ObjectManagement/Proxy/ProxyClass.php

Neos.Flow/Classes/Persistence/Doctrine/Mapping/Driver/FlowAnnotationDriver.php

Neos.Flow/Classes/Reflection/ReflectionService.php

albe · 2021-03-22T19:00:49Z

Neos.Flow/Tests/Unit/ObjectManagement/Proxy/CompilerTest.php

                '@\Neos\Flow\Annotations\Validate(type="bar1", argumentName="foo1")'
            ],
            [
-                new Validate(['type' => 'bar1', 'options' => ['minimum' => 2]]),
+                new Validate(null, 'bar1', ['minimum' => 2]),


This shows that the choice to have argumentName as the anonymous argument was probably suboptimal. I would rather consider the validate type to be anonmyous - maybe something to consider for next major?

Question basically is: Are Validate annotations more commonly used on method arguments, or on properties? If on method arguments, is more straightforward to use @Flow\Validate("NotEmpty", argumentName="$myArg") rather than @Flow\Validate("$myArg", type="NotEmpty")?

No idea on where validation is used more. But I have no issue with @Flow\Validate("$myArg", type="NotEmpty"), TBH. What is not straighforward with that?

Neos.Utility.ObjectHandling/Classes/TypeHandling.php

Neos.Utility.ObjectHandling/Tests/Unit/TypeHandlingTest.php

Neos.Utility.ObjectHandling/Classes/TypeHandling.php

Neos.Flow/Classes/I18n/Cldr/CldrParser.php

mficzel · 2021-04-16T07:01:36Z

I generally like this but consider it a bit breaky especially since the annotation api changed. Also it would be great to be able to convert the doctrine annotations at the same time.

Not sure about how fast Doctrine will be to support annotations but imho this would be great for Flow8.

albe · 2021-06-08T20:58:29Z

As of doctrine/orm 2.9.x PHP attributes are supported, BUT their changes are breaking for us - see doctrine/orm#8753

albe · 2021-07-02T17:37:51Z

Update: As of doctrine/orm 2.9.3+ - which is allowed now in 7.0+ as of #2496 - PHP 8 attributes work now

Neos.Flow/Tests/Functional/ObjectManagement/Fixtures/PHP8/ClassWithPhpAttributes.php

Neos.Flow/Tests/Functional/ObjectManagement/Fixtures/PHP8/SampleAttribute.php

albe · 2021-08-11T11:52:02Z

Note: I should propably rebase this PR to clean up the commit history in relation to #2468 and then start with a clean review cycle

Also avoids registering those classes if not on PHP8 or later.

albe · 2021-08-18T13:57:18Z

Rebased cleanly now and also extracted the docblock-preserving commit into #2533

albe · 2021-08-18T18:43:45Z

Given the psalm tests fail with the PHP8 methods - maybe it makes more sense to run psalm with latest PHP version, rather than lowest?

Regarding the lowest-deps build:

Argument 1 passed to Neos\Flow\Annotations\Scope::__construct() must be of the type string or null, array given, called in /home/runner/work/flow-development-collection/flow-development-collection/flow-development-distribution/Packages/Libraries/doctrine/annotations/lib/Doctrine/Common/Annotations/DocParser.php on line 825

After some digging it seems to be due to doctrine/annotations and the newly used @NamedArgumentConstructor, which only exists in doctrine/annotations 1.12+ - see doctrine/annotations#391 - that might also explain a similar error that @mficzel saw and mentioned in slack. I noticed we do not yet have a direct dependency on doctrine/annotations at all, though we definitely directly use it. So we should add that.
See #2535

kitsunet

Looks fine to me. Wondering a bit if any of the new features will have impact on eg AOP and if we need to do something about it. but I guess it should be fine for now.

sorenmalling

From reading it looks fine to me.

I am a bit/very much in doubt how to properly test it deeper, to give valuable feedback

sorenmalling · 2021-08-20T08:00:01Z

@albe With the new NamedConstructorAnnotation annotation you mention in #2535 - doesn't it mean that you need to have one merge before the other, and we can't do a release with only one of these merged?

albe · 2021-08-20T09:25:39Z

@albe With the new NamedConstructorAnnotation annotation you mention in #2535 - doesn't it mean that you need to have one merge before the other, and we can't do a release with only one of these merged?

We should have both in optimally, but it only is an issue if you can't install latest doctrine/annotations in your setup for some reason. In that case you will see an error like above (in DocParser) instead of composer rejecting to install things in the worst case.

I am a bit/very much in doubt how to properly test it deeper, to give valuable feedback

Unless you have some code base using PHP8 types (union / nullable) and/or classes with property promotion, it's impossible to test deeper really :) So no worries. If you have some idea for test fixtures, i.e. exactly such classes (like https://github.com/neos/flow-development-collection/pull/2404/files#diff-d7acfbcf9e4274f911c5c5f5f23ae6f8c8cecf4d64ebcc52d40b04bc050779b3), then that would be a great addition to cover potential cases that break compilation

Edit: PS: the open TODO actually belongs to the PHP8 Attributes issue... PPS: Created #2537

Neos.Utility.ObjectHandling/Classes/TypeHandling.php

kdambekalns · 2021-08-30T16:49:53Z

7.3 / lowest fails due to Guzzle vs. Behat…

This was referenced Mar 1, 2021

Support for PHP 8 #2233

Closed

TASK: Add basic support for PHP8 #2287

Merged

albe commented Mar 5, 2021

View reviewed changes

Neos.Utility.ObjectHandling/Classes/TypeHandling.php Show resolved Hide resolved

Neos.Flow/Tests/Functional/ObjectManagement/Fixtures/ClassWithUnionTypes.php Outdated Show resolved Hide resolved

.github/workflows/build.yml Outdated Show resolved Hide resolved

albe force-pushed the albe-php8 branch from b3410c3 to 57070ea Compare March 22, 2021 12:06

albe commented Mar 22, 2021

View reviewed changes

albe commented Mar 23, 2021

View reviewed changes

Neos.Utility.ObjectHandling/Tests/Unit/TypeHandlingTest.php Show resolved Hide resolved

albe commented Mar 23, 2021

View reviewed changes

Neos.Utility.ObjectHandling/Classes/TypeHandling.php Show resolved Hide resolved

albe requested review from kdambekalns, bwaidelich, kitsunet, robertlemke and sorenmalling March 23, 2021 18:46

albe changed the base branch from 7.0 to master March 23, 2021 18:46

albe changed the title ~~TASK: Add some tests for PHP8 features~~ TASK: Support PHP8 attributes and property promotion Mar 23, 2021

albe commented Apr 2, 2021

View reviewed changes

Neos.Flow/Classes/I18n/Cldr/CldrParser.php Show resolved Hide resolved

albe mentioned this pull request Apr 18, 2021

FEATURE: Support PHP8 attributes #2468

Merged

albe mentioned this pull request Jul 17, 2021

TASK: Update doctrine/orm requirement from ^2.7, <2.9 to ^2.9.3 #2495

Merged

albe commented Aug 11, 2021

View reviewed changes

Neos.Flow/Tests/Functional/ObjectManagement/Fixtures/PHP8/ClassWithPhpAttributes.php Outdated Show resolved Hide resolved

Neos.Flow/Tests/Functional/ObjectManagement/Fixtures/PHP8/SampleAttribute.php Outdated Show resolved Hide resolved

albe changed the title ~~TASK: Support PHP8 attributes and property promotion~~ TASK: Support PHP8 types and class property promotion Aug 11, 2021

albe added 5 commits August 17, 2021 02:46

TASK: Add some tests for PHP8 features

fd96e48

TASK: Reflect nullable in type handling

cbb2eb1

TASK: Move PHP8 test classes to own folder

793217e

Also avoids registering those classes if not on PHP8 or later.

TASK: Help psalm with type errors

da1746d

TASK: Remove PHP7.1 condition

6501b62

albe added 9 commits August 17, 2021 02:46

TASK: Fix ClassSchemaTest for missing union types

2a84428

TASK: Fix missing nullable type information in test

e0f89d2

TASK: Support ? as nullable type hint

2fd95d8

TASK: Add isPropertyNullable method to ClassSchema

1554910

TASK: Fix parseType nullable hint return

047ebef

TASK: Fix ProxyMethod test

4c2ab71

TASK: Enable PHP8 Support for StyleCI

d73ac10

TASK: Add psalm ProxyConstructor typehint

ec189e5

TASK: Improve mock declaration

417f3c1

albe force-pushed the albe-php8 branch 2 times, most recently from b96eb37 to 78818d8 Compare August 18, 2021 13:54

albe force-pushed the albe-php8 branch from e9ec810 to 417f3c1 Compare August 18, 2021 18:37

kitsunet approved these changes Aug 19, 2021

View reviewed changes

sorenmalling approved these changes Aug 20, 2021

View reviewed changes

kdambekalns added 2 commits August 30, 2021 18:36

Merge branch 'master' into albe-php8

4a707e7

TASK: Add nullable to Doctrine annotations example

d827262

kdambekalns approved these changes Aug 30, 2021

View reviewed changes

Neos.Utility.ObjectHandling/Classes/TypeHandling.php Show resolved Hide resolved

kdambekalns merged commit 95e8edb into master Aug 30, 2021

kdambekalns deleted the albe-php8 branch August 30, 2021 17:04

daniellienert mentioned this pull request Oct 21, 2021

The result of MethodRefelection::getTagValues() changed from Neos 7.1 to Neos 7.2 #2612

Closed

albe mentioned this pull request Dec 13, 2022

BUG: Use of union types breaks proxy building, creates broken PHP code #2941

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TASK: Support PHP8 types and class property promotion #2404

TASK: Support PHP8 types and class property promotion #2404

albe commented Mar 1, 2021 •

edited

Loading

albe commented Mar 1, 2021

albe Mar 22, 2021

kdambekalns Aug 30, 2021

albe Mar 22, 2021 •

edited

Loading

kdambekalns Aug 30, 2021

mficzel commented Apr 16, 2021

albe commented Jun 8, 2021

albe commented Jul 2, 2021

albe commented Aug 11, 2021

albe commented Aug 18, 2021

albe commented Aug 18, 2021 •

edited

Loading

kitsunet left a comment

sorenmalling left a comment

sorenmalling commented Aug 20, 2021

albe commented Aug 20, 2021 •

edited

Loading

kdambekalns commented Aug 30, 2021

TASK: Support PHP8 types and class property promotion #2404

TASK: Support PHP8 types and class property promotion #2404

Conversation

albe commented Mar 1, 2021 • edited Loading

albe commented Mar 1, 2021

albe Mar 22, 2021

Choose a reason for hiding this comment

kdambekalns Aug 30, 2021

Choose a reason for hiding this comment

albe Mar 22, 2021 • edited Loading

Choose a reason for hiding this comment

kdambekalns Aug 30, 2021

Choose a reason for hiding this comment

mficzel commented Apr 16, 2021

albe commented Jun 8, 2021

albe commented Jul 2, 2021

albe commented Aug 11, 2021

albe commented Aug 18, 2021

albe commented Aug 18, 2021 • edited Loading

kitsunet left a comment

Choose a reason for hiding this comment

sorenmalling left a comment

Choose a reason for hiding this comment

sorenmalling commented Aug 20, 2021

albe commented Aug 20, 2021 • edited Loading

kdambekalns commented Aug 30, 2021

albe commented Mar 1, 2021 •

edited

Loading

albe Mar 22, 2021 •

edited

Loading

albe commented Aug 18, 2021 •

edited

Loading

albe commented Aug 20, 2021 •

edited

Loading