Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BUGFIX: No usage of dynamic properties #3032

Merged

Conversation

kitsunet
Copy link
Member

@kitsunet kitsunet commented May 2, 2023

This patch resolves deprecation notices on PHP 8.2:

Dynamic properties in proxy classes

There are some properties that are dynamically declared in proxy classes:

  • Flow_Injected_Properties
  • Flow_Object_PropertiesToSerialize
  • Flow_Persistence_RelatedEntities: this is mostly used inside of the ObjectSerializationTrait, so I thought it might make more sense to declare it there instead of adding a property using the ProxyClassBuilder

Resolves #2946

parent inside of closures

There are some methods that will be checked against is_callable with parent:::

  • parent::Flow_Aop_Proxy_buildMethodsAndAdvicesArray
  • parent::__wakeup

Review instructions

  • Set up a flow distribution on PHP 8.2
  • Run tests and make sure no deprecation warnings are thrown

Checklist

  • Code follows the PSR-2 coding style
  • Tests have been created, run and adjusted as needed
  • The PR is created against the lowest maintained branch
  • Reviewer - PR Title is brief but complete and starts with FEATURE|TASK|BUGFIX
  • Reviewer - The first section explains the change briefly for change-logs
  • Reviewer - Breaking Changes are marked wit !!! and have upgrade-instructions

@github-actions github-actions bot added the 8.0 label May 2, 2023
@kitsunet kitsunet changed the title Bugfix/2946 no usage of dynamic properties BUGFIX: No usage of dynamic properties May 2, 2023
@github-actions github-actions bot added the Bug label May 2, 2023
@kitsunet
Copy link
Member Author

kitsunet commented May 2, 2023

See also #2950

@fcool
Copy link
Contributor

fcool commented May 9, 2023

Shouldn't that target 7.3, too?

Copy link
Member

@kdambekalns kdambekalns left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good by reading.

@kdambekalns
Copy link
Member

Shouldn't that target 7.3, too?

Well, Flow 7.3 supports PHP 8.0, that's true. But what this addresses is more for PHP 8.2, see https://www.php.net/releases/8.2/en.php#deprecate_dynamic_properties – so I guess it's fine to target Flow 8.0…

Copy link
Member

@robertlemke robertlemke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks fine and I tested the code contained in the traits at least. However, I can imagine that there will be some merge conflicts when upmerging to 9.0 due to the little clean ups you added.

@kitsunet
Copy link
Member Author

@robertlemke would you prefer I remove any cleanups before merging?

@robertlemke
Copy link
Member

@robertlemke would you prefer I remove any cleanups before merging?

I think they are fine, but whoever does the upmerge needs to know a little what he/she is doing.

@kitsunet kitsunet merged commit a664380 into neos:8.0 May 26, 2023
@robertlemke robertlemke deleted the bugfix/2946-no-usage-of-dynamic-properties branch May 26, 2023 16:03
@robertlemke
Copy link
Member

I hope I knew what I was doing, but for the record: I just did the upmerges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants