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-20972 Attempt to handle new type of Exception thrown in php7.1 wh… #10772

Merged
merged 2 commits into from
Jul 27, 2017

Conversation

seamuslee001
Copy link
Contributor

@seamuslee001 seamuslee001 commented Jul 26, 2017

@seamuslee001
Copy link
Contributor Author

@totten this passed by php7.1 set up and Jenkins has given it the nod. @eileenmcnaughton any thoughts on this?

@seamuslee001 seamuslee001 changed the title CRM-20972 Attempt to handle new type of Execption thrown in php7.1 wh… CRM-20972 Attempt to handle new type of Exception thrown in php7.1 wh… Jul 27, 2017
@eileenmcnaughton
Copy link
Contributor

eileenmcnaughton commented Jul 27, 2017

Interesting, and kinda cool. Since it's in the test suite jenkins is the ultimate arbiter of this patch. However, I think the main benefit of adding it is to show people what can be done (& perhaps later they will in their own extensions) so it probably at least needs to be flagged as a php 7.1 thing (ie. with code comments)

@eileenmcnaughton
Copy link
Contributor

oh I see - it is required for the tests - still think it would be nice to note it as 7.1 support

@seamuslee001
Copy link
Contributor Author

@eileenmcnaughton how would you see that it should be noted as php7.1 support?

@seamuslee001
Copy link
Contributor Author

@eileenmcnaughton I have added a code comment in each function now about the new Exception

@eileenmcnaughton eileenmcnaughton merged commit 9adcfb2 into civicrm:master Jul 27, 2017
@eileenmcnaughton
Copy link
Contributor

Fantastic

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.

2 participants