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

Migrated Minds.com's engine to PHP v7.2.0. #31

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

hopeseekr
Copy link

@hopeseekr hopeseekr commented Nov 27, 2018

Note: The Twilio/SDK v3.13.1 is incompatible with PHP v7.2 because it uses create_function in several places. Only their latest v6 supports PHP v7.2 and since v3 has been EOL'd, they definitely aren't going to backport PHP 7.2 fixes.

So! Working with the Twilio devs, I backported the PHP 7.2 changes to v3.13.1 and updated this project's composer.json to use that specific project branch instead.


This change is Reviewable

PHP 7.2 made `object` a keyword, thus not allowing classes to be named as such.
Note: There is a bug in Email/Manager which this migration made visible.
The each() function will be removed in December with the release of PHP 7.3.
Prior to PHP 7.2.0, PHP merely ignored method signature discrepancies.
But now, they are Fatal Errors, as they should be.

Efforts at backward-compatibility were made, thus the `$unused` parameters.
So, the guys over at Twilio told me flat out that v3.13.1 of their SDK was very much EOL'd
and they just wouldn't ever support PHP 7.2.

They also told me that the first version to support PHP 7.2 was Twilio/SDK v6, which is a full
three major versions ahead. Not knowing anything about this app's usage of Twilio, I resorted
to forking Twilio and migrating v3.13.1 to PHP 7.2.

By using my repo's 3.13.1-create-function-fix branch, all of the PHPSpec tests now pass!
@@ -6,7 +6,7 @@
/**
* Carousel Entity
*/
class Carousel extends Entities\Object
class Carousel extends Entities\MindsObject
Copy link
Author

Choose a reason for hiding this comment

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

I went back and forth on whether to move all of the classes in the Object namespace to MindsObject as well, but in the interests of keeping changes to a minimum, I decided against doing so. Nothing seems to break by leaving it that way.

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.

1 participant