BUGFIX: Don't create proxy classes for PHP enums by robertlemke · Pull Request #2702 · neos/flow-development-collection

robertlemke · 2022-02-21T16:16:47Z

This change introduces a check in the proxy class compiler which makes sure that PHP 8.1 enums are not considered as regular classes and are removed from the list of proxyable classes.

How to reproduce:

create an enum, for example in Packages/Acme/Classes/ExampleEnum.php:

namespace Acme;
enum ExampleEnum
{
    case FOO;
    case BAR;
}

let Flow compile classes, the following error will appear:

PHP Fatal error: Cannot declare class Acme\ExampleEnum, because the name is already in use in /application/Data/Temporary/Development/Cache/Code/Flow_Object_Classes/Acme_ExampleEnum.php on line 23

Addresses #2698

This change introduces a check in the proxy class compiler which makes sure that PHP 8.1 enums are not considered as regular classes and are removed from the list of proxyable classes. Addresses neos#2698

albe

Looks good, thanks! Can we do something like we did here https://github.com/neos/flow-development-collection/blob/master/Neos.Flow/Classes/ObjectManagement/CompileTimeObjectManager.php#L231 => https://github.com/neos/flow-development-collection/tree/master/Neos.Flow/Tests/Functional/ObjectManagement/Fixtures/PHP8 and add such an enum to the tests conditionally for PHP81?

robertlemke · 2022-02-22T07:47:46Z

Looks good, thanks! Can we do something like we did here https://github.com/neos/flow-development-collection/blob/master/Neos.Flow/Classes/ObjectManagement/CompileTimeObjectManager.php#L231 => https://github.com/neos/flow-development-collection/tree/master/Neos.Flow/Tests/Functional/ObjectManagement/Fixtures/PHP8 and add such an enum to the tests conditionally for PHP81?

Yes, more or less – we can't check if the enum was proxied, because it never will be, since the compiler would fail with a fatal error. But yes, we need to back this with a test, so I just pushed one.

robertlemke · 2022-02-22T07:49:24Z

Hmm. Now we need to find a good way to sneak this test in without breaking the < PHP 8.1 runs.

robertlemke · 2022-02-22T07:59:36Z

Okay, I think I found a solution an force-pushed the commit providing the functional test.

kdambekalns

👍

kdambekalns · 2022-02-22T08:20:19Z

Sadly it fails for PHP 8.0 with

 syntax error, unexpected identifier "BackedEnumWithMethod"
Type: ParseError
  File:
Packages/Framework/Neos.Flow/Tests/Functional/ObjectManagement/Fixtures/PHP
        8/BackedEnumWithMethod.php
  Line: 19

robertlemke · 2022-02-22T09:59:42Z

Pretty weird, works on my machine™️ …

But I'll try to fix it.

robertlemke · 2022-02-22T10:28:13Z

Oh and we should run tests for 8.1 I guess ;-)

nezaniel · 2022-02-22T17:15:41Z

@8.1 tests:
I've just merged #2695, here you go :)

robertlemke · 2022-02-23T15:55:24Z

StyleCI doesn't seem to like enums …

kdambekalns · 2022-02-23T21:52:54Z

StyleCI doesn't seem to like enums …

https://docs.styleci.io/standalone-php#version

kitsunet

Looks good, thanks! Good to have that fixed

bwaidelich

I think this makes sense, but as written in Slack IMO we should strive towards creating proxies only if they are really needed. And, if a proxy was to be created for a class that is not supported (e.g. an enum) an exception should be thrown.
Otherwise: What happens if you add an AOP aspect for an enum with this change? Will it be ignored?

bwaidelich · 2022-03-04T09:19:03Z

                if ($package instanceof FlowPackageInterface && $shouldRegisterFunctionalTestClasses) {
                    foreach ($package->getFunctionalTestsClassFiles() as $fullClassName => $path) {
-                        if (PHP_MAJOR_VERSION < 8 && strpos($fullClassName, '\\PHP8\\') !== false) {
+                        if (version_compare(PHP_VERSION, '8.0', '<=') && strpos($fullClassName, '\\PHP8\\') !== false) {


Not sure if it's much better, but my PHPStorm suggests to change this line to

Suggested change

if (version_compare(PHP_VERSION, '8.0', '<=') && strpos($fullClassName, '\\PHP8\\') !== false) {

if (PHP_VERSION_ID <= 80000 && strpos($fullClassName, '\\PHP8\\') !== false) {

But apart from that: I'm wondering what these checks actually do?
Could we add some inline comment to explain that?

bwaidelich · 2022-03-04T09:19:51Z

+/**
+ * A PHP 8.1 value-backed enum with methods
+ */
+enum BackedEnumWithMethod: string


I assume that this fixture is not loaded with PHP < 8.1 and doesn't cause any harm that way!?

Yes, exactly what you wondered about in CompileTimeObjectManager.php

robertlemke · 2022-03-15T13:50:46Z

What do we do with the failing psalm and styleci tests? They are false-positives, correct?

kdambekalns · 2022-03-15T16:55:15Z

What do we do with the failing psalm and styleci tests? They are false-positives, correct?

I'll take care…

kdambekalns · 2022-03-15T18:20:57Z

✅

albe · 2022-03-21T09:44:46Z

Post-merge 👍 Thanks everyone!

BUGFIX: Don't create proxy classes for PHP enums

c1b3dd0

This change introduces a check in the proxy class compiler which makes sure that PHP 8.1 enums are not considered as regular classes and are removed from the list of proxyable classes. Addresses neos#2698

robertlemke requested a review from kdambekalns February 21, 2022 16:22

albe reviewed Feb 21, 2022

View reviewed changes

Provide functional test for PHP 8.1 enum handling

536fdaa

robertlemke force-pushed the no-proxy-class-for-enums branch from 1797c67 to 536fdaa Compare February 22, 2022 07:59

kdambekalns approved these changes Feb 22, 2022

View reviewed changes

Load certain test fixtures only with PHP >= 8.1

6519b87

robertlemke requested a review from albe February 22, 2022 11:30

kdambekalns approved these changes Feb 22, 2022

View reviewed changes

robertlemke added P: Flow T: PHP labels Feb 22, 2022

robertlemke requested a review from kitsunet March 1, 2022 10:56

kitsunet approved these changes Mar 4, 2022

View reviewed changes

bwaidelich reviewed Mar 4, 2022

View reviewed changes

kdambekalns added 4 commits March 15, 2022 18:04

TASK: Switch StyleCI to PHP 8.1

55c9f66

TASK: Remove unused import

5090a12

Merge branch '7.3' into no-proxy-class-for-enums

c60384d

Merge branch '7.3' into no-proxy-class-for-enums

05738be

kdambekalns enabled auto-merge March 15, 2022 18:21

kdambekalns merged commit d08517b into neos:7.3 Mar 15, 2022

robertlemke deleted the no-proxy-class-for-enums branch August 29, 2022 09:55

mhsdesign mentioned this pull request Mar 16, 2025

BUGFIX: Enums throw ::__construct() does not exist on compilation #3458

Open

	if (version_compare(PHP_VERSION, '8.0', '<=') && strpos($fullClassName, '\\PHP8\\') !== false) {
	if (PHP_VERSION_ID <= 80000 && strpos($fullClassName, '\\PHP8\\') !== false) {

Uh oh!

Uh oh!

Conversation

robertlemke commented Feb 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

albe left a comment

Choose a reason for hiding this comment

Uh oh!

robertlemke commented Feb 22, 2022

Uh oh!

robertlemke commented Feb 22, 2022

Uh oh!

robertlemke commented Feb 22, 2022

Uh oh!

kdambekalns left a comment

Choose a reason for hiding this comment

Uh oh!

kdambekalns commented Feb 22, 2022

Uh oh!

robertlemke commented Feb 22, 2022

Uh oh!

robertlemke commented Feb 22, 2022

Uh oh!

nezaniel commented Feb 22, 2022

Uh oh!

robertlemke commented Feb 23, 2022

Uh oh!

kdambekalns commented Feb 23, 2022

Uh oh!

kitsunet left a comment

Choose a reason for hiding this comment

Uh oh!

bwaidelich left a comment

Choose a reason for hiding this comment

Uh oh!

bwaidelich Mar 4, 2022

Choose a reason for hiding this comment

Uh oh!

bwaidelich Mar 4, 2022

Choose a reason for hiding this comment

Uh oh!

kdambekalns Mar 4, 2022

Choose a reason for hiding this comment

Uh oh!

robertlemke commented Mar 15, 2022

Uh oh!

kdambekalns commented Mar 15, 2022

Uh oh!

kdambekalns commented Mar 15, 2022

Uh oh!

albe commented Mar 21, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

robertlemke commented Feb 21, 2022 •

edited

Loading