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

Add propertyBag handling for getEmail when incoming uses email-5 #17267

Merged
merged 1 commit into from
May 10, 2020

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented May 9, 2020

Overview

Update to PropertyBag to enable it to handle the fact that calling functions use some legacy params for billing fields not yet handled - ie

$params['email-5'] for Billing email.

With this change we encourage the use of setEmail but if the param was email-5 it can still be retrieved with getEmail

Before

$params['email-5'] not returned when calling getEmail

After

$params['email-5'] returned when calling getEmail

Also any other fields which match the correct field with an appended '-5' (Where 5 is the billing location ID)

Technical Details

Calling code is often setting email-5. The property bag should iron out this inconsistency, and getEmail (and similar)
should handle the appended billing id. We can't do this in the static property map as it is a calculated variable

Comments

@artfulrobot @mattwire

Calling code is often setting email-5. The property bag should iron out this inconsistency, and getEmail (and similar)
should handle the appended billing id. We can't do this in the static property map as it is a calculated variable
@civibot
Copy link

civibot bot commented May 9, 2020

(Standard links)

@civibot civibot bot added the master label May 9, 2020
$localPropertyBag = new PropertyBag();
$localPropertyBag->mergeLegacyInputParams(['email-' . \CRM_Core_BAO_LocationType::getBilling() => 'a@b.com']);
$this->assertEquals('a@b.com', $localPropertyBag->getEmail());
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps it would be a good idea to also do

$this->assertEquals('a@b.com', $localPropertyBag['email-' . \CRM_Core_BAO_LocationType::getBilling()]);

...to check the getter too. it should work because it uses the same legacy code but I assume that old code that sets it this way will also get it this way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah - I'm on the fence about adding that - it implies it's supported but the goal is one day to only support a sensible set...

Copy link
Contributor

Choose a reason for hiding this comment

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

I think adding a getter test just further confirms and documents the expectation. This PR is specifically about adding support for another legacy use case. All get/set that use array access issue a deprecation notice anyway, so I think it's still clear that it's not The Right Way, but while we're supporting hacks for legacy calls I think tests would have been an improvement. Not worth the effort of a separate PR now though.

@artfulrobot
Copy link
Contributor

This looks ok to me. It would be good to list the places where this is used in an issue so we can weed them out one day.

@eileenmcnaughton
Copy link
Contributor Author

test this please

@mattwire mattwire merged commit 9210ed3 into civicrm:master May 10, 2020
@eileenmcnaughton eileenmcnaughton deleted the bag branch May 10, 2020 22:22
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.

3 participants