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

Revert the array constant, use the private static array again. This d… #92

Merged
merged 3 commits into from
Oct 5, 2017
Merged

Revert the array constant, use the private static array again. This d… #92

merged 3 commits into from
Oct 5, 2017

Conversation

aaajeetee
Copy link

…oes not break the PHP 5.4 and 5.5 compatibility

…oes not break the PHP 5.4 and 5.5 compatibility
@Xon
Copy link

Xon commented Oct 4, 2017

@aaajeetee you might as well preserve the O(1) lookup behaviour instead of the in_array linear search behaviour.

The test matrix only went as far back as php 5.6 (which is the last 5.x version which is not yet EOL), as such the automatic testing didn't catch this.

@colinmollenhour
Copy link
Owner

@aaajeetee you might as well preserve the O(1) lookup behaviour instead of the in_array linear search behaviour.

Agreed, in_array is not efficient for something which will conceivably be used many many times. It might be a speculative micro-optimization but it is already there, just use a private static array as a hash with isset(). An optimized preg_match might be the fastest but would sacrifice code maintainability.

@aaajeetee
Copy link
Author

Thanks for the feedback. I have used the array_key_exists() function call now, as it is in version 1.9.0.

@aaajeetee
Copy link
Author

Also, is it an idea to update the travis file to also test PHP versions 5.4 and 5.5?

@colinmollenhour colinmollenhour merged commit 696f224 into colinmollenhour:master Oct 5, 2017
@colinmollenhour
Copy link
Owner

I added PHP 5.5 to the unit tests and got it passing. Is there a real use-case for officially supporting PHP 5.4? It actually passes currently but the number of places it is supported is dwindling..

https://travis-ci.org/colinmollenhour/credis/builds/283894536

@aaajeetee aaajeetee deleted the revert-array-constant branch October 6, 2017 05:54
@aaajeetee
Copy link
Author

There isn't really a use-case for supporting PHP 5.4 I suppose, it's more the fact that this project supports PHP 5.4, so might as well test it.

But in the end that's your decision.

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.

3 participants