(REF) Civi/Schema - Extract MagicGetterSetterTrait. Add test coverage.

[totten]

Overview

Creates a trait, MagicGetterSetterTrait FluentGetterSetterTrait, which captures the magic getter/setter behavior from APIv4's AbstractAction class. This makes it easier to implement similar behavior in other classes, and it ensures that certain rules (like handling of _) are standardized.

For example:

class Foo {
  use MagicGetterSetterTrait;

  protected $bar;
}

$foo = new Foo();
$foo->setBar(100);
echo $foo->getBar();

Before

• AbstractAction implements __call() (to handle getFoo() and setFoo()) and does scanning/filtering of the property-list.

After

• AbstractAction uses MagicGetterSetterTrait.
• MagicGetterSetterTrait implements __call() (to handle getFoo() and setFoo())and does scanning/filtering of the property-list.
• There's a test for MagicGetterSetterTrait which touches on the naming and string-manipulation.

Comment

The methods AbstractAction::getParams() (public) and the MagicGetterSetterTrait::getMagicProperties() (private) may sound similar, but they differ:

• getParams() returns all of the parameter data. In effect, it converts $this from an object to an array (but filtered by the property-list).
• getMagicProperties() only returns the property-list.

(*Updated description to reflect name change: FluentGetterSetterTrait => MagicGetterSetterTrait)

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[eileenmcnaughton]

Fail does not relate to this PR

[eileenmcnaughton]

@totten I tested this & have a couple of observations

1. it is only discoverable if you also add annotations like this
   

I feel like we should do some sort of testing or other noise to ensure that people ARE adding the annotations or it will get fugly quickly

2. I was rather hoping to see errors for lines 3 & 4 for the below. The parameter is described as an integer but still accepts and returns a string (I think accepting and returning NULL as a strongly favourable thing but strings should fail IMHO)

[mattwire]

@totten On first glance I read this as "Effluent" GetterSetterTrait and now can't get it out of my head. Would a better name be MagicGetterSetterTrait?

Otherwise seems like a good idea.

[eileenmcnaughton]

OK - let's give it a few days to see if we can put effluent behind us (gufaw) or whether we need to rename .....

[totten]

(@mattwire) On first glance I read this as "Effluent" GetterSetterTrait and now can't get it out of my head. Would a better name be MagicGetterSetterTrait?

Lol, I've never seen the word "Effluent" before 🚰. Agree that MagicGetterSetterTrait is better - since the technique is usually called magic....

(@eileenmcnaughton) it is only discoverable if you also add annotations like this

Yeah, agree that's a little annoying. FWIW, I've updated the docblock to mention this and point to some examples... so at least now it's mentioned somewhere...

I feel like we should do some sort of testing or other noise to ensure that people ARE adding the annotations...

It's almost easier to use real getter+setters with PHPStorm's code-generator than to add those docblocks. But the magic approach is still probably a bit better (ie the class files are less noisy overall, and the magic implementations are consistently fluent - PHPStorm codegen doesn't use a fluent template.)

Adding some kind of test could be nice. Maybe a phpcs rule - or maybe a phpunit test which loops through all the core classes and does some assertions based on the traits/interfaces/properties/docblocks. (foreach ($class) { if $class has trait MagicGetterSetter { assert docblock has @methods for each property } })

I was rather hoping to see errors for lines 3 & 4 for the below. The parameter is described as an integer but still accepts and returns a string

Interesting. I had been angling (in low-priority/future-maybe way) to include a type-check as part of the WorkflowMessage::validate() implementation.

I guess it's conceivable to implement at the magic-setter-level. In the current regime, that would require a bit more docblock parsing - but it's fine if a request only has a handful of these classes. (It'd also be fine if only activated in some kind of strict-mode or test-mode.)

FWIW, PHP 7.4 adds built-in support for type-checking of properties (e.g. https://stitcher.io/blog/typed-properties-in-php-74)... the issue might resolve itself whenever we get to use php74 typehints...

[colemanw]

@totten - @eileenmcnaughton has reported a regression caused by this PR in https://lab.civicrm.org/dev/core/-/issues/2768#note_63239