-
-
Notifications
You must be signed in to change notification settings - Fork 824
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
Conversation
CRM/Mailing/BAO/Mailing.php
Outdated
$ids['mailing_id'] = $ids['id'] = $params['id']; | ||
|
||
if (empty($params['id']) && !empty($ids)) { | ||
$params['id'] = isset($ids['mailing_id']) ? $ids['mailing_id'] : $ids['id']; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
df917fe
to
eb6abcf
Compare
(this requires clean up of handling of ).
eb6abcf
to
5044f0f
Compare
@colemanw are you ok to merge this? |
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)) { |
There was a problem hiding this comment.
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))
?
There was a problem hiding this comment.
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
@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) |
Ok makes sense now. |
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.