Skip to content
This repository has been archived by the owner on Jan 8, 2020. It is now read-only.

php 5.6 compatibility #5957

Closed
wants to merge 2 commits into from
Closed

php 5.6 compatibility #5957

wants to merge 2 commits into from

Conversation

Maks3w
Copy link
Member

@Maks3w Maks3w commented Mar 12, 2014

No description provided.

@Maks3w Maks3w added this to the 2.3.1 milestone Mar 12, 2014
@@ -45,7 +45,9 @@ public function init($operand, $base = null)
}
}

set_error_handler(function () { /* Do nothing */}, \E_WARNING);
Copy link
Member

Choose a reason for hiding this comment

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

What exactly can go wrong here in 5.6?

Copy link
Member Author

Choose a reason for hiding this comment

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

"[math][php-56] PHP now throws E_WARNING when string is not a valid gm… …
…p integer."

Copy link
Member

Choose a reason for hiding this comment

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

@DASPRiD see https://travis-ci.org/zendframework/zf2/jobs/20643164#L695

@Maks3w is error suppression actually how we're supposed to deal with this?

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought in throw InvalidArgumentException but interface contract explictly use false for errors handling.
$res still having false as value.

Copy link
Member

Choose a reason for hiding this comment

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

For that error handler setting, don't we have something in Stdlib?

Copy link
Member

Choose a reason for hiding this comment

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

I second @DASPRiD - that's exactly why ErrorHandler was created, so I see no reason not to use.

Copy link
Contributor

Choose a reason for hiding this comment

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

Prefixing it with @ is not enough?

Copy link
Member

Choose a reason for hiding this comment

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

That's not allowed within ZF2.

Copy link
Member Author

Choose a reason for hiding this comment

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

There is another approach. Just silent the warning in the unit test and let it throw in production.

Copy link
Member

Choose a reason for hiding this comment

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

@Maks3w I'd go with the fix in the test itself (expect a warning)

@ezimuel any suggestions?

@Maks3w
Copy link
Member Author

Maks3w commented Mar 19, 2014

ping @zendframework/community-review-team

@jkavalik
Copy link

should be possible to check for warning thrown in phpunit (?convert to exception) for php 5.6+

@Maks3w
Copy link
Member Author

Maks3w commented Mar 19, 2014

As I said. false is the expected return type instead of throw exceptions.

@jkavalik
Copy link

@Maks3w I meant only in unit test - split them to good X bad and for bad ones something like

 if(phpver >= 5.6) phpunit.expect-warning(...);

or

if(phpver >= 5.6) {
    phpunit.warning-to-exception = true;
    phpunit.expect-exception(phpunit-warning-exception);
}

@Ocramius
Copy link
Member

@ezimuel what is your take on this? Is a warning expected?

@ezimuel
Copy link
Contributor

ezimuel commented Apr 14, 2014

I think the fix proposed by @Maks3w is ok in that specific case. The use of Zend\Stdlib\ErrorHandler seems to be verbose here. False is perfectly fine because it evidences that the number that you are trying to create with BigInteger is not valid. We don't need more info on that.

@Ocramius Ocramius assigned Ocramius and unassigned Ocramius Apr 14, 2014
@Ocramius
Copy link
Member

@ezimuel can you merge then? Or eventually I can do that as well.

@ezimuel
Copy link
Contributor

ezimuel commented Apr 14, 2014

@Ocramius go for it, thanks!

@Ocramius Ocramius self-assigned this Apr 14, 2014
@Ocramius Ocramius closed this in 8559caa Apr 14, 2014
Ocramius added a commit that referenced this pull request Apr 14, 2014
@Maks3w Maks3w deleted the hotfix/php-56-compatibility branch May 31, 2014 10:13
gianarb pushed a commit to zendframework/zend-math that referenced this pull request May 15, 2015
gianarb pushed a commit to zendframework/zend-math that referenced this pull request May 15, 2015
weierophinney pushed a commit to zendframework/zend-i18n-resources that referenced this pull request May 28, 2015
weierophinney pushed a commit to zendframework/zend-i18n-resources that referenced this pull request May 28, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants