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-21140 extend support for custom data to Mailing api #11608

Merged
merged 1 commit into from
Feb 17, 2018

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Jan 29, 2018

Overview

With this PR it is possible (per previous on CRM-21440) to extend Mailing with Custom data. This is primarily for the benefit of extension writers

Before

Mailing api does not support custom data.

After

Mailing api supports custom data.

Technical Details

Parameters more standardised. Deprecation on non-std params

Comments

There are a number of entities that are now possible but this is one that specifically failed tests due to mishandling of $ids.


$ids['mailing_id'] = $ids['id'] = $params['id'];

if (empty($params['id']) && !empty($ids)) {
$params['id'] = isset($ids['mailing_id']) ? $ids['mailing_id'] : $ids['id'];
Copy link
Contributor

Choose a reason for hiding this comment

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

@eileenmcnaughton getting error of undefined index id here

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 - I think I got it this time

@eileenmcnaughton eileenmcnaughton force-pushed the mailing_custom branch 3 times, most recently from df917fe to eb6abcf Compare January 30, 2018 22:12
(this requires clean up of handling of ).
@eileenmcnaughton eileenmcnaughton changed the title CRM-21140 extend support for custom data to Mailing. CRM-21140 extend support for custom data to Mailing api Feb 1, 2018
@eileenmcnaughton
Copy link
Contributor Author

@colemanw are you ok to merge this?

@MegaphoneJon
Copy link
Contributor

As a note: This is exciting because this makes it possible to write extensions that weren't previously possible to add features that Mailchimp/Constant Contact have. E.g. you could add a tweet to a mailing, which is automatically tweeted with a link to the "View on Website" version of the mailing at the time the mailing goes out.

$id = CRM_Utils_Array::value('mailing_id', $ids, CRM_Utils_Array::value('id', $params));
$id = CRM_Utils_Array::value('id', $params, CRM_Utils_Array::value('mailing_id', $ids));

if (empty($params['id']) && !empty($ids)) {
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this condition just be if (!empty($ids))?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

$ids actually won't be empty coming from the api - it will hold ['Mailing' => $params['id']] - but the goal is to make the $ids array first meaningless & then to remove it - as long as they are correctly setting the $params array they can pass anything or nothing in $ids

@eileenmcnaughton
Copy link
Contributor Author

@colemanw can we get this merged - I am hoping to work on the other entities & for 5.0.0 support custom data on all entities (so extension writers can declare 5.0 as their version & know the custom data will work)

@colemanw colemanw merged commit 817993f into civicrm:master Feb 17, 2018
@colemanw
Copy link
Member

Ok makes sense now.

@eileenmcnaughton eileenmcnaughton deleted the mailing_custom branch February 18, 2018 21:06
@mlutfy mlutfy added this to the 4.7.32 milestone Mar 6, 2018
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.

6 participants