Reduce use of undeclared properties (php8.x errors) , use trait to track entities #27146
Conversation
|
Thank you for contributing to CiviCRM! ❤️ We will need to test and review the PR. 👷 Introduction for new contributors
Quick links for reviewers |
4d80053 to
89887f6
Compare
| * @return mixed | ||
| */ | ||
| public function getEntityValue(string $entity, int $id, string $key) { | ||
| if (!isset($this->entities[$entity][$id]) || !array_key_exists($key, $this->entities[$entity][$id])) { |
There was a problem hiding this comment.
so I suppose $identifier in L34 may not be an actual int but here we are assuming it will be an int right?
There was a problem hiding this comment.
hmm yeah - I think I go back & forth on whether we should only support ids or whether we should support string indentifiers too...
There was a problem hiding this comment.
ok - all ints now
There was a problem hiding this comment.
Actually I think it's quite useful to be able to look up stuff by name as well as id.
| */ | ||
| public function getParticipantValue(string $fieldName, ?int $id = NULL) { | ||
| $id = $id ?? $this->getParticipantID(); | ||
| return $this->getEntityValue('Participant', $id, $fieldName); |
There was a problem hiding this comment.
@eileenmcnaughton this doesn't seem to be exactly the same as the original as the original could find the value in participant_$fieldname if it wasn't present in $fieldName` right?
There was a problem hiding this comment.
@seamuslee001 true - but the actual usage of this function means that isn't an issue -
| */ | ||
| private $entities = []; | ||
|
|
||
| public function setEntity($entity, $identifier, $values): void { |
There was a problem hiding this comment.
Some type hints would help. I assume $entity is a string and $values is an array?
|
We at CiviCRM sure do enjoy reinventing wheels! Not that this isn't a nice shiny wheel or that we shouldn't use it. But it might be worthwhile to take stock of all the other wheels on the shelf and see if there's anything we can do about our excess inventory...
|
A lot of the undeclared properties on this form & similar are just tracking values of various relevant entities. This adds some functionality for that tracking onto a trait & uses that. This reduces the need to pass around values whose only importance is that they have been loaded from the database & would require another db call to access them from elsewhere. Note I have swapped out the interaction with _contributorEmail & _contributorDisplayName in getStatusMessage and send email receipts - these are the last 2 functions called in the submit routine & the only reason the receipt function needs to set it is to pass them to getStatusMessage. There are a couple of earlier interactions which are odd so I ignored them for now. The loop for sending out receipts is pretty messed up - so the first step is getting the inputs & outputs for that loop clarified - but this property is bang in the middle
89887f6 to
4a5f9ea
Compare
|
@colemanw yeah good point but the others store one entity per class - ie not contact + event + participant & use different property names on every form + I can't standardise prperties across multiple forms cos they have inconsisten visibility & naming & also those that don't use apiv4 don't support related contact emails (I expect we would also want to add support to load pseudoconstants to) |
| $select = ['email_primary.email', 'email_primary.on_hold', '*', 'custom.*']; | ||
| } | ||
| else { | ||
| $select = ['*', 'custom.*']; |
There was a problem hiding this comment.
I'm 👎🏻 on loading custom data by default. This class is a lazy loader so could fetch it if you request the value of a custom field but I think loading the "kitchen sink" with potentially dozens of joins on custom tables is a waste of cpu & memory(and invites getting bitten by that max join limit in SQL).
|
Question: is it really necessary for a typical form to track > 1 type of entity? Because |
|
@colemanw yep - because forms have, for example,
Currently all of these are being tracked & accessed in a weird & wonderful array of ways & there is no 'right' way to get any of the values internally or from an extension - so the goal will be to be able to retrieve any of these - ideally etc Note the second one - in unit tests we have a similar class that accepts a string that the form interprets eg. |
|
Ok @eileenmcnaughton I had a long think about this & I was able to take your idea here and generalize it a bit more: #27257 |
|
@colemanw I had a go at using it & it replaced the need for some of this but I wound up having to write other stuff to use it - I will put it up separately |
|
Closing this in favour of the new trait + the merge-ready PR |
Overview
Reduce use of undeclared properties (php8.x errors) , use trait to track entities
Before
Undefined property errors on useDisplayName
After
Still errors - but the genesis of an approach
Technical Details
A lot of the undeclared properties on this form & similar are just tracking values of various relevant entities. This adds some functionality for that tracking onto a trait & uses that. This reduces the need
to pass around values whose only importance is that they have been loaded from the database & would require
another db call to access them from elsewhere.
I've kept the change to use this pretty small but there are a bunch of places where properties that may or may not exist are set just to hold the values on the form 'somewhere'
I'm thinking these get functions could have the
@apiflag, allowing / encouraging their use from outside of coreComments
@seamuslee001