Skip to content

Defer Services_Json inclusion/initialization until really needed#91

Merged
tobio merged 1 commit into
campaignmonitor:masterfrom
sserbin:defer-services-json-inclusion
Jul 31, 2019
Merged

Defer Services_Json inclusion/initialization until really needed#91
tobio merged 1 commit into
campaignmonitor:masterfrom
sserbin:defer-services-json-inclusion

Conversation

@sserbin

@sserbin sserbin commented Jul 12, 2019

Copy link
Copy Markdown
Contributor

Alternatively, I'd consider dropping Services_Json altogether. Since php
5.2 php comes in with bundled json ext so it should be available everywhere.

The motivation for this is that requireing this file always not only registers a class definition but also produces some undesired side-effects - namely, global constants definitions

@sserbin sserbin force-pushed the defer-services-json-inclusion branch from c1aad92 to 29d8e06 Compare July 12, 2019 09:56
@sserbin

sserbin commented Jul 12, 2019

Copy link
Copy Markdown
Contributor Author

Tests pass on 5.6 and 7, other travis envs seem to be not working at all

Alternatively, I'd consider dropping Services_Json altogether. Since php
5.2 php comes in with bundled json ext so it should be available everywhere
@sserbin sserbin force-pushed the defer-services-json-inclusion branch from 29d8e06 to 288adf3 Compare July 31, 2019 09:42
@sserbin

sserbin commented Jul 31, 2019

Copy link
Copy Markdown
Contributor Author

Tests pass on 5.6 and 7, other travis envs seem to be not working at all

rebased on master (which has updated travis config), so tests pass now

@tobio tobio merged commit d8ae5ba into campaignmonitor:master Jul 31, 2019
@tobio

tobio commented Jul 31, 2019

Copy link
Copy Markdown
Contributor

Thanks for this, I'd agree that this serialiser should be dropped. Hopefully someone from CM can make that happen. In the interim this change makes sense.

@sserbin

sserbin commented Jul 31, 2019

Copy link
Copy Markdown
Contributor Author

Thanks, @tobio

@sserbin

sserbin commented Aug 28, 2019

Copy link
Copy Markdown
Contributor Author

@taledo @markstaples any chance we can get a release for this? 🙏 I know there hasn't been anything else merged into master since the past release, but would be great to have this released, thanks!

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