[REF][PHP8.2] Only set properties which exist within ArrayFormatTrait->loadArray()

[braders]

Overview

Only set properties which exist within the loadArray method on Civi\Schema\Traits\ArrayFormatTrait.

Before

A large number of the remaining PHP 8.2 test failures (approx 10%) are resulting from api\v4\Entity\ConformanceTest:

This is because in the test there is this code:

$class = empty($field['custom_field_id']) ? FieldSpec::class : CustomFieldSpec::class;
$spec = (new $class($field['name'], $field['entity']))->loadArray($field, TRUE);

$field always contains custom_field_id, but only CustomFieldSpec has getters and setters for customFieldId. Therefore, when loadArray is called on FieldSpec, custom_field_id is passed in as null, which loadArray tries to set as a dyamic property on the object. As dynamic properties are deprecated in PHP 8.2, this therefore throws a deprecation error.

After

loadArray only writes properties if they actually exist; in other words property_exists($this, $field).

Technical Details

Civi\Schema\Traits\ArrayFormatTrait::loadArray is also used in Civi\WorkflowMessage\Traits\ReflectiveWorkflowTrait, but without setting $strict to true. I think this ReflectiveWorkflowTrait is also likely to cause problems on PHP 8.2, although this is not causing test failures at the moment and I've not spent much time looking into how this WorkflowMessage functionality works.

For now, there is only a functionality change when $strict is true. I expect Civi\WorkflowMessage\FieldSpec may at some stage need to be marked with the #[AllowDynamicProperties] attribute to ensure PHP 8.2 compatiability there.

[civibot]

(Standard links)

• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[seamuslee001]

@colemanw thoughts on this?

[eileenmcnaughton]

Am I misreading this? I feel like it should be if strict not if not strict

[braders]

From the function's docblock:

In strict mode, properties are only accepted if they are formally defined on the current class.

So this is saying:

• If we're not in strict mode, always set the property, regardless of if it's being dynamically set (this will cause issues on PHP 8.2, but I think is probably necessary for the WorkflowMessage functionality for now)
• Else if we're not in strict mode, only set the property if the property exists.

I do agree this isn't particuarly readable, but I am struggling to see a better way of structuring it at the moment.

[eileenmcnaughton]

OK thanks @braders - I'll try to get input from @colemanw. I'm not quite sure why it's needed for WorkflowMessage - I think possibly for an approach within WorkflowMessage that's not really in use (ie whereever I've used them I've defined the properties)

[braders]

Thanks @eileenmcnaughton. The actual relevant WorkflowMessage code is this:

$parsed = ReflectionUtils::getCodeDocs($property, 'Property');
$field = new \Civi\WorkflowMessage\FieldSpec();
$field->setName($property->getName())->loadArray($parsed);

so the properties are all being set on FieldSpec, not on an actual instance of GenericWorkflowMessage which would have the properties. (This isn't an area of Civi I'm very familiar with though, so I may be misundertanding what's going on)

[eileenmcnaughton]

Hmm @totten might be able to comment then - I don't really understand that code either

FWIW I have been building out the WorkflowTemplate classes using only properties with a declared get method or that are declared in the annotations. From memory there was also the idea that 'any' property that was on the class might have a pseudo-get on it & I feel like this code might be to support that?

[totten]

Yeah, the FieldSpec classes aren't things you'd patch every day. ;)

To clarify what it does, I believe it's for this kind of code:

civicrm-core/Civi/Api4/Action/Contact/MergeDuplicates.php

Lines 46 to 55 in 92400d7

	/**
	* Whether to run in "Safe Mode".
	*
	* Safe Mode skips the merge if there are conflicts. Does a force merge otherwise.
	*
	* @var string
	* @required
	* @options safe,aggressive
	*/
	protected $mode = 'safe';

civicrm-core/CRM/Contribute/WorkflowMessage/ContributionTrait.php

Lines 18 to 22 in 8193ed7

	/**
	* @var int
	* @scope tokenContext as contributionId
	*/
	public $contributionId;

Note the special annotations like @required, @options and @scope. A "field spec" is a list of all those annotations/metadata that you'd put on a field. For example:

• MergeDuplicates is an APIv4 action-class. The field $mode has a corresponding field-spec (per Civi\Api4\Service\Spec\FieldSpec) with info like $fieldSpec->required and $fieldSpec->options.
• ContributionInvoiceReceipt is a workflow-message class. The field $contributionId has a corresponding field-spec (per Civi\WorkflowMessage\FieldSpec) with info like $fieldSpec->scope

The process of loading annotations is roughly:

1. Get the annotations as a big string ("@var int\n@required\n@options a,b,c")
2. Convert the string to array (["var" => "int", "required" => true, "options" => "a,b,c"])
3. Convert the array to object properties (new FieldSpec() and $fieldSpec->setRequired($arr['required']))

I've focused on annotations here because that's a common data-source which is (comparatively) easy to visualize, but I think there may be other sources of field-specs (e.g. database/extensions/hooks).

(I haven't read the rest of the PR or got my head into the actual warnings/edge-cases. This comment is 50% explaining and 50% trying-to-remember for myself...)

[eileenmcnaughton]

Just noting that the tests that hit this on WorkflowMessage include

Civi\WorkflowMessage\ExampleWorkflowMessageTest::testValidateFail and Civi\WorkflowMessage\ExampleWorkflowMessageTest::testImpromptuImportExport

I believe the reason is that the properties don't have setters - in practice, as I mentioned, I have created setters for all properties on the workflow templates that are in use - so having properties without setters in WorkflowMessage classes feels more like an experiment than a requirement to me

[totten]

$field always contains custom_field_id, but only CustomFieldSpec has getters and setters for customFieldId. Therefore, when loadArray is called on FieldSpec, custom_field_id is passed in as null, which loadArray tries to set as a dyamic property on the object. As dynamic properties are deprecated in PHP 8.2, this therefore throws a deprecation error.

Ah, that is a peculiar edge-case - the field is undefined, and the caller specifies a value of null.

My feeling is that it's probably OK to skip in that case. And the tests are passing. ✔️ Additionally, IIRC, there is a good amount of test-coverage that implicitly relies on these classes. But... it is a funny area, and there were couple pings for Coleman to feedback too.

IMHO, "merge ready" (unless anyone comes back with a problem in, say, 24-36hr).

Civi\Schema\Traits\ArrayFormatTrait::loadArray is also used in Civi\WorkflowMessage\Traits\ReflectiveWorkflowTrait, but without setting $strict to true.

Yeah, I don't know why it's in non-strict mode. My general sense is that the modeling for it was pretty formal, so I would hope be for strict mode to work. But let's see: #25818

[eileenmcnaughton]

Merging per #25818 (comment)