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

[REF][PHP8.2] Declare properties directly on CRM_Core_Form_EntityFormTrait #25926

Merged
merged 1 commit into from
Mar 26, 2023

Conversation

braders
Copy link
Contributor

@braders braders commented Mar 26, 2023

Overview

Declare properties directly on CRM_Core_Form_EntityFormTrait, rather than (inconsistently) on the classes which use it.

Before

The entityFields property was declared individually on many classes which use CRM_Core_Form_EntityFormTrait, but not all of them. Additionally the large docblock was repeated many times, increasing the chance of inconsistent documentation occuring in future.

The metadata property was always being written to as a dynamic property, causing tests to fail on PHP 8.2.

After

The entityFields and metadata properties are explicitely declared on the CRM_Core_Form_EntityFormTrait.

Comments

I think there may be a small backwards compatiability risk here, if third-party extensions are using CRM_Core_Form_EntityFormTrait, but have declared entityFields or metadata as public properties (although I've not actually tested what would happen in this scenario)

There is still a _BAOName property to tackle, which I'll look at seperately.

@civibot
Copy link

civibot bot commented Mar 26, 2023

(Standard links)

@civibot civibot bot added the master label Mar 26, 2023
@colemanw colemanw merged commit 23df83f into civicrm:master Mar 26, 2023
@colemanw
Copy link
Member

Thanks @braders I think the risk you described is very low given the usual copy-paste development style, they would have been copied over as protected

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

Successfully merging this pull request may close these issues.

2 participants