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

Make framework PHP 7.4 compatible #16

Merged
merged 4 commits into from
Dec 16, 2019
Merged

Make framework PHP 7.4 compatible #16

merged 4 commits into from
Dec 16, 2019

Conversation

Megatherium
Copy link

There were a bunch a minor changes needed like trimming slashes and
octothorpes of strings before passing them into hexdec and removing all
array accesses via curly braces.

I have no idea how Zend_Crypt_Math::rand ever worked under unixlikes
because we expect it to return a number but it outputs directly from
/dev/urandom if it exists.

Serialization of ReflectionMethods is also illegal since 7.4

A lot of docblocks were updated, assertions changed to more obvious ones
and grimey things like dirname(FILE) were properly removed whenever
I encountered them for most files that I touched.

There were a bunch a minor changes needed like trimming slashes and
octothorpes of strings before passing them into hexdec and removing all
array accesses via curly braces.

I have no idea how Zend_Crypt_Math::rand ever worked under unixlikes
because we expect it to return a number but it outputs directly from
/dev/urandom if it exists.

Serialization of ReflectionMethods is also illegal since 7.4

A lot of docblocks were updated, assertions changed to more obvious ones
and grimey things like dirname(__FILE__) were properly removed whenever
I encountered them for most files that I touched.
@Megatherium Megatherium changed the title All tests pass on PHP 7.4 Make framework PHP 7.4 compatible Dec 6, 2019
@falkenhawk
Copy link
Member

@Megatherium could you modify the travis config and replace 7.4snapshot with 7.4, and remove 7.4 from allow_failures, please?

Copy link
Member

@falkenhawk falkenhawk left a comment

Choose a reason for hiding this comment

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

Thanks for your work on this! Did you use a tool or did you do all the changes manually?
Just one test to fix (a different expected error message for php 7.4+) and we should probably get rid of that broken /dev/urandom implementation.

The method returned random bytes if /dev/urandom exists even though a
random integer was expected.
@Megatherium
Copy link
Author

I basically just used PHPStorm with some plugins and spammed a lot of Alt+Enter for the docblocks. Most of the changes to the code (that weren't necessitated by failing tests) were also suggested by the IDE but I was rather conservative in what I actually changed especially when it came to changing == to ===.

@Megatherium Megatherium reopened this Dec 14, 2019
@Megatherium
Copy link
Author

Generalized the expected exception text so we don't need any version compares. Holler if you need anything else done.

Copy link
Member

@falkenhawk falkenhawk left a comment

Choose a reason for hiding this comment

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

Thank you once again!

@falkenhawk falkenhawk merged commit a88e745 into zf1s:master Dec 16, 2019
@falkenhawk
Copy link
Member

Released version 1.13.1 of all packages.

@@ -104,11 +104,11 @@ public function __construct($service, $data)
// require_once 'Zend/Service/Rackspace/Servers/Exception.php';
throw new Zend_Service_Rackspace_Servers_Exception(self::ERROR_PARAM_CONSTRUCT);
}
if (!array_key_exists('name', $data)) {
if (!isset($data['name'])) {
Copy link
Contributor

@holtkamp holtkamp Dec 16, 2019

Choose a reason for hiding this comment

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

Note that array_key_exists will return true if an array entry is NULL while isset will return false.

So this is a functional change....

https://3v4l.org/6llcH

Copy link
Member

Choose a reason for hiding this comment

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

You are right @holtkamp ... @Megatherium could you elaborate on this change, please? Did it even make sense to allow NULL values in id and name before? But I tend to agree this should not go like that into a patch release...

Copy link
Author

Choose a reason for hiding this comment

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

I have to admit that I was overeager and ignorant of the minute difference.

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.

5 participants