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

PHP 8 behaves slightly different with in_array and usort #2706

Merged
merged 1 commit into from
Jul 3, 2022

Conversation

mtmail
Copy link
Collaborator

@mtmail mtmail commented May 10, 2022

PHP 8.1 (default on Ubuntu-22) shows test suite failures we don't see with PHP 7.4

  1. php -r 'var_dump(in_array(0, array("option1","option2")));'. PHP 7 returns true, PHP 8 returns false. PHP 8 is probably better here. Using strict mode https://www.php.net/manual/en/function.in-array.php both return false.

  2. In SimpleWordList.php we use usort which can often have no winner. Thus nondeterministic sort outcome neither PHP 7 or PHP 8 can be said to be more correct than the other. I added a second clause to favor longer phrases. It's hopefully a relative cheap computation compared to hashing or full string comparison because the usort is called in a loop with potentially 100s of 1000s elements.

@mtmail mtmail requested a review from lonvia May 10, 2022 16:39
@lonvia
Copy link
Member

lonvia commented May 10, 2022

It's hopefully a relative cheap computation compared to hashing or full string comparison because the usort is called in a loop with potentially 100s of 1000s elements.

Have you tried if sorting by string comparison is really an issue? Sorting 1000 elements shouldn't be much of a problem if they have implemented a half-way decent sorting algorithm. And it might make the code quite a bit easier.

@mtmail
Copy link
Collaborator Author

mtmail commented May 11, 2022

String comparison is only done on the first element of the array. Shouldn't be an issue at.

@mtmail
Copy link
Collaborator Author

mtmail commented Jun 20, 2022

This fixes the remaining test failure of the ubuntu-22 PR #2691

@lonvia
Copy link
Member

lonvia commented Jun 20, 2022

I toke your comment that you wanted to switch to sorting by simple string comparison.

@mtmail
Copy link
Collaborator Author

mtmail commented Jul 2, 2022

String comparison would require converting the array to a string first which has extra cost.

I read through https://www.php.net/manual/en/language.types.array.php and https://www.php.net/manual/en/ref.array.php again and the implementation in this PR seems the most performant already.

@mtmail mtmail force-pushed the php-fixes-php7-vs-php8 branch from 88e4360 to ccf1192 Compare July 3, 2022 08:55
@lonvia lonvia merged commit 6799692 into osm-search:master Jul 3, 2022
@mtmail mtmail deleted the php-fixes-php7-vs-php8 branch April 23, 2024 14:07
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.

2 participants