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

Display deprecation notices in api output and api explorer #4340

Merged
merged 5 commits into from
Oct 13, 2014

Conversation

colemanw
Copy link
Member

@eileenmcnaughton
Copy link
Contributor

very cool. Nice solution.

Makes me wonder if we should think about making our change log part of the api in a similar way (ie. some function that advises when an api function was introduced)

@colemanw colemanw force-pushed the deprecatedApi branch 2 times, most recently from 29255ba to b55db16 Compare October 10, 2014 15:51
@colemanw
Copy link
Member Author

Thanks eileen... but now the question is, why is this causing the test suite to fall over?

@colemanw colemanw force-pushed the deprecatedApi branch 2 times, most recently from 3887a4b to d97370c Compare October 11, 2014 18:26
@eileenmcnaughton
Copy link
Contributor

I'd like to see if it will fail the same way a second time - it kind of looks like it is failing because of the skipped test threshhold - what does it do locally?

@eileenmcnaughton
Copy link
Contributor

test this please

* @return string to indicate this entire api entity is deprecated
*/
function _civicrm_api3_mailing_group_deprecation() {
return 'The mailing_group api is deprecated. Use the mailing_event apis instead.';
Copy link
Contributor

Choose a reason for hiding this comment

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

I just spotted a possible reason for test problem. The test failed just before showing evidence of running the checkTitle on this api. I can see that mailing_deprecation returns an array & this returns a string - maybe related?

@yashodha yashodha added the 4.5.2 label Oct 13, 2014
@colemanw
Copy link
Member Author

Thanks for looking at this Eileen. I'm not quite sure where your train of thought is going wrt test threshold - do you have a theory there?

Wrt string vs array - both are valid return values and they get interpreted differently by the api wrapper. As I tried to indicate in the comments: If the function returns a string, then the entire api entity is reported as deprecated. If it returns an array, then each array key represents a deprecated action.

If you are not able to make sense of the jenkins output any better than me and we are getting into the realm of guessing, then I could just try disabling random parts of this PR and re-running the tests on it.

@colemanw
Copy link
Member Author

One more thing to try before I resort to random guessing - the MailingEvent tests don't work for me locally (something about my version of php I think) but @totten would you would be willing to check the tests around that spot and see if you can get any clues?

@totten
Copy link
Member

totten commented Oct 13, 2014

Running the test locally and hacking in some debug statements shows infinite recursion on an API call for mailing_group.getfields -- https://gist.github.com/totten/85514fd98b1c7dbcb2dd

@eileenmcnaughton
Copy link
Contributor

was it so easy to spot my random guessing?

@colemanw
Copy link
Member Author

I think @totten just saved us both from a lot of random guessing. Fingers crossed for this fix...

@colemanw
Copy link
Member Author

Hooray!

@eileenmcnaughton
Copy link
Contributor

never let him go on holiday again!

colemanw added a commit that referenced this pull request Oct 13, 2014
Display deprecation notices in api output and api explorer
@colemanw colemanw merged commit c2d01cb into civicrm:4.5 Oct 13, 2014
@colemanw colemanw deleted the deprecatedApi branch October 13, 2014 23:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants