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

include tests in pecl archive, fix test with redis < 2.6.9, fix build warnings #333

Closed
wants to merge 6 commits into from
Closed

Conversation

remicollet
Copy link
Collaborator

fixes issue #322

@remicollet
Copy link
Collaborator Author

@@ -1651,6 +1651,9 @@ public function testPersist() {
}

public function testClient() {
if (version_compare($this->version, "2.6.9", "lt")) {
$this->markTestSkipped();
Copy link
Member

Choose a reason for hiding this comment

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

What is the point of this test?

@nicolasff
Copy link
Member

Oh right, just saw your follow-up. Thanks.

@remicollet
Copy link
Collaborator Author

This is the last. All build warnign fix.

Please be carefull on 5757df4
I think, as p was the '\n' or 'n' char, is_numeric test was never true.
I hope my tix is correct.

@remicollet
Copy link
Collaborator Author

Note, about is_numeric, I think it could be usefull to add a condition on strlen(value)<MAX_LENGTH_OF_LONG
As "1234567890123456789" is obvisouly not a number ;)

@nicolasff
Copy link
Member

Well that's not the best example :-)

php > $a = array();
php > $a['1234567890123456789'] = 42;
php > foreach($a as $k => $v) { var_dump($k, $v); }
int(1234567890123456789)
int(42)

"1234567890123456789" is indeed parsed as a number here. "12345678901234567891234567890123456789" shouldn't be, though.

Detecting the threshold purely on the number of characters might be too restrictive. I'll think about what we can do in this case...

@michael-grunder
Copy link
Member

@remicollet Thanks for the pull request. At a quick glance, I think you are correct regarding 5757df4. I'll take a closer look but nice find! :)

@remicollet
Copy link
Collaborator Author

deprecated

@remicollet remicollet closed this Dec 22, 2016
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.

None yet

3 participants