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

Renamed Mailing Component entity #12728

Merged
merged 3 commits into from
Sep 25, 2018

Conversation

pradpnayak
Copy link
Contributor

Overview

Mailing component was using entity name as 'Component' which overrides CRM/Core/DAO/Component.php since both were using same entity name. This PR renames the Mailing Component entity from 'Component' to 'MailingComponent'

@civibot
Copy link

civibot bot commented Aug 24, 2018

(Standard links)

@mattwire
Copy link
Contributor

This looks like it makes sense. @pradpnayak Was this breaking something specific? @totten This is probably something you may understand/have a view on?

@pradpnayak
Copy link
Contributor Author

pradpnayak commented Aug 24, 2018

Actually we have an extension that implements Component api and since the Component entity was over-ridden by Mailing Component it failed to retrieve results.

@eileenmcnaughton
Copy link
Contributor

@colemanw @mattwire @seamuslee001 what do you think. I feel a bit nervous about this just because of risks of unknown interactions but I feel like my concerns would be mitigated by keeping a copy of CRM_Mailing_DAO_Component that is an empty class extending the new one (& at some future point it could go)

@@ -50,7 +50,7 @@ public function __construct() {
* @return CRM_Core_BAO_LocationType.
*/
public static function retrieve(&$params, &$defaults) {
$component = new CRM_Mailing_DAO_Component();
$component = new CRM_Mailing_BAO_Component();
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't we put the new DAO here - given the BAO needs renaming too but perhaps in a later iteration?

Copy link
Member

Choose a reason for hiding this comment

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

I agree. @pradpnayak can you update?

@@ -71,7 +71,7 @@ public static function retrieve(&$params, &$defaults) {
* true if we found and updated the object, else false
*/
public static function setIsActive($id, $is_active) {
return CRM_Core_DAO::setFieldValue('CRM_Mailing_DAO_Component', $id, 'is_active', $is_active);
return CRM_Core_DAO::setFieldValue('CRM_Mailing_BAO_Component', $id, 'is_active', $is_active);
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

Copy link
Member

Choose a reason for hiding this comment

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

I agree. @pradpnayak can you update?

@@ -86,7 +86,7 @@ public static function setIsActive($id, $is_active) {
*/
public static function add(&$params, $ids = array()) {
$id = CRM_Utils_Array::value('id', $params, CRM_Utils_Array::value('id', $ids));
$component = new CRM_Mailing_DAO_Component();
$component = new CRM_Mailing_BAO_Component();
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

@colemanw
Copy link
Member

I have updated the PR to also rename the BAO and to leave a deprecated copy of it to cover any backward-compat issue in extensions (although extensions really should not call BAOs directly for this reason).

@eileenmcnaughton eileenmcnaughton merged commit 65ae421 into civicrm:master Sep 25, 2018
@eileenmcnaughton
Copy link
Contributor

test fails unrelated

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants