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

CRM-21574 NFC code cleanup of CRM_Friend_BAO_Friend class #12355

Conversation

mattwire
Copy link
Contributor

@mattwire mattwire commented Jun 22, 2018

Overview

Towards #11425. This PR contains the non-functional code changes that cleanup that class.

Before

Less easy to maintain code

After

More easy to maintain code

Comments

Seems to be impossible to get a reviewer for #11425 as I think virtually no-one uses the PCP classes so let's try breaking it down a little.


@civibot
Copy link

civibot bot commented Jun 22, 2018

(Standard links)

@mattwire
Copy link
Contributor Author

Anyone other than @eileenmcnaughton able to review this PR please - it's just code cleanup... @seamuslee001 @michaelmcandrew @monishdeb ??

@michaelmcandrew
Copy link
Contributor

not me at the moment, I'm afraid @mattwire :(

self::getValues($frndParams);
$friendParams = array();
$friendParams['entity_id'] = $params['entity_id'];
$friendParams['entity_table'] = $params['entity_table'];
Copy link
Member

Choose a reason for hiding this comment

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

Change to

$friendParams = [
 'entity_id' => $params['entity_id'],
 'entity_table' => $params['entity_table'],
];

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes

*
* @var int
*/
public $_friendId;
Copy link
Member

Choose a reason for hiding this comment

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

will it be protected instead of public?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's used in public static function buildFriendForm($form) which is called from two other classes (Contribute and Event) so it needs to be public unless we do a lot more work, at which point we would risk breaking things :-)

$mailParams['title'] = CRM_Utils_Array::value('title', $params);
$mailParams['general_link'] = CRM_Utils_Array::value('general_link', $frndParams);
$mailParams['general_link'] = CRM_Utils_Array::value('general_link', $friendParams);
$mailParams['message'] = CRM_Utils_Array::value('suggested_message', $params);

// get domain
$domainDetails = CRM_Core_BAO_Domain::getNameAndEmail();
Copy link
Member

@monishdeb monishdeb Jun 26, 2018

Choose a reason for hiding this comment

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

Can you change it to

-  $domainDetails = CRM_Core_BAO_Domain::getNameAndEmail();
+ list($_, $mailParams['email_from']) = CRM_Core_BAO_Domain::getNameAndEmail();
...
- // Default "from email address" is default domain address.
- // This is normally overridden by one of the if statements below
- $mailParams['email_from'] = $domainDetails[1];

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks - nice cleanup.

@mattwire mattwire force-pushed the CRM-21574_disable_contact_email_tellafriend_nfc branch from 5fda8ad to 4d0bbeb Compare June 26, 2018 21:01
@mattwire
Copy link
Contributor Author

Thanks for reviewing @monishdeb I've made changes as requested.

@monishdeb
Copy link
Member

(CiviCRM Review Template WORD-1.1)

@monishdeb monishdeb merged commit 067ec55 into civicrm:master Jun 28, 2018
@mattwire mattwire deleted the CRM-21574_disable_contact_email_tellafriend_nfc branch September 25, 2018 11:01
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.

4 participants