Updated: Comment #N

Problem/Motivation

This is part of #2165377: [meta] HHVM compatibility

fetchBaseTables is supposed to return its results ordered by weight and then title. Internally, it uses uasort, which according to PHP's documentation is not stable.

The test case in HEAD has got 4 elements with equal weight, but 2 elements with equal titles AND weight. The order for both elements are thus undefined after uasort. However, the test case assumes that the order is preserved.

Proposed resolution

This patch will change the comparison of the array keys to check for the proper order of the array instead of specifically checking the keys of the array for equality.

Remaining tasks

User interface changes

API changes

Original report by @lowjoel

fetchBaseTables is supposed to return its results ordered by weight and then title. Internally, it uses uasort, which according to PHP's documentation is not stable.

The test case in HEAD has got 4 elements with equal weight, but 2 elements with equal titles AND weight. The order for both elements are thus undefined after uasort. However, the test case assumes that the order is preserved.

This patch will change the comparison of the array keys to check for the proper order of the array instead of specifically checking the keys of the array for equality.

CommentFileSizeAuthor
#5 drupal-fix-array-order-checking-2191533-5-D8.patch1.15 KBberdir
#3 drupal-fix-array-order-checking-2191533-3-D8.patch1.16 KBlowjoel
#1 drupal-fix-array-order-checking-2191533-1-D8.patch1.15 KBlowjoel

Comments

lowjoel’s picture

lowjoel commented
StatusFileSize
new drupal-fix-array-order-checking-2191533-1-D8.patch1.15 KB

Upload patch v1

dawehner’s picture

dawehner
Primary language German
commented

Good catch!

+++ b/core/modules/views/tests/Drupal/views/Tests/ViewsDataTest.php
@@ -137,7 +137,12 @@ public function testFetchBaseTables() {
+    	$prev =  $base_tables[$base_tables_keys[$i - 1]];
...
+    	$this->assertTrue($prev['weight'] <= $current['weight'] && $prev['title'] <= $prev['title'], 'The tables are sorted as expected.');
...
+    	$current = $base_tables[$base_tables_keys[$i]];

we chould better use spaces instead of tabs.

lowjoel’s picture

lowjoel commented
StatusFileSize
new drupal-fix-array-order-checking-2191533-3-D8.patch1.16 KB

Woops, editorfail.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented
Status: Needs review » Needs work
Issue tags: +VDC
berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Issue summary: View changes
Status: Needs work » Reviewed & tested by the community
Parent issue: » #2165377: [meta] HHVM compatibility
StatusFileSize
new drupal-fix-array-order-checking-2191533-5-D8.patch1.15 KB

Almost perfect, we use two spaces instead of 4. Here's a fix.

Confirmed this locally on HHVM, the test is passing again.

I'm apparently currently unable to compile the latest version (master) of HHVM due to the following error:

[  1%] make[2]: *** No rule to make target `hphp/third_party/folly/folly/SpookyHash.cpp', needed by `hphp/third_party/folly/CMakeFiles/folly.dir/folly/SpookyHash.cpp.o'.  Stop.
make[1]: *** [hphp/third_party/folly/CMakeFiles/folly.dir/all] Error 2

So I can't verify if the other tests (I currently have two failing tests with my version) are fixed but even if they weren't, it would be a separate issue.

Also updated the issue summary.

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.