Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

php 5.6 compatibility #5957

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
7 participants
Member

Maks3w commented Mar 12, 2014

No description provided.

@Maks3w Maks3w added this to the 2.3.1 milestone Mar 12, 2014

@DASPRiD DASPRiD commented on the diff Mar 12, 2014

library/Zend/Math/BigInteger/Adapter/Gmp.php
@@ -45,7 +45,9 @@ public function init($operand, $base = null)
}
}
+ set_error_handler(function () { /* Do nothing */}, \E_WARNING);
@DASPRiD

DASPRiD Mar 12, 2014

Member

What exactly can go wrong here in 5.6?

@Maks3w

Maks3w Mar 12, 2014

Member

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

@Ocramius

Ocramius Mar 12, 2014

Member

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

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

@Maks3w

Maks3w Mar 12, 2014

Member

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

@DASPRiD

DASPRiD Mar 12, 2014

Member

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

@Maks3w

Maks3w Mar 12, 2014

Member

Yep, I didn't use stdlib because the component don't have any dependency.

@Thinkscape

Thinkscape Mar 13, 2014

Member

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

@danizord

danizord Mar 13, 2014

Contributor

Prefixing it with @ is not enough?

@DASPRiD

DASPRiD Mar 13, 2014

Member

That's not allowed within ZF2.

@Maks3w

Maks3w Mar 13, 2014

Member

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

@Ocramius

Ocramius Apr 3, 2014

Member

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

@ezimuel any suggestions?

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

Member

Maks3w commented Mar 19, 2014

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

@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);
}
Member

Ocramius commented Mar 19, 2014

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

@Ocramius Ocramius added Math labels Apr 3, 2014

Owner

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

Member

Ocramius commented Apr 14, 2014

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

Owner

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 Maks3w:hotfix/php-56-compatibility branch May 31, 2014

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 join this conversation on GitHub. Already have an account? Sign in to comment