Problem/Motivation

CKEditor test fails because it is trying to compare two arrays that are equal but have a different order of elements.

Proposed resolution

ksort both arrays.

Remaining tasks

Review patch

User interface changes

API changes

CommentFileSizeAuthor
#9 2465917-9.patch1.36 KBstefan.r
#9 Capture d’écran 2015-04-06 à 18.45.08.png156.3 KBstefan.r
#7 2465917-7.patch1.36 KBstefan.r
#3 2465917-3.patch1.59 KBstefan.r
#1 2465917-1.patch1.59 KBstefan.r

Comments

stefan.r’s picture

stefan.r commented
Status: Active » Needs review
StatusFileSize
new 2465917-1.patch1.59 KB

Before:

Pass      Other      CKEditorTest.php    46 Drupal\ckeditor\Tests\CKEditorTest-
    Installed system tables: {url_alias}.
Fail      Other      CKEditorTest.php   286 Drupal\ckeditor\Tests\CKEditorTest-
    "Internal" plugin configuration built correctly for default toolbar.
Fail      Other      CKEditorTest.php   293 Drupal\ckeditor\Tests\CKEditorTest-
    "Internal" plugin configuration built correctly for customized toolbar.
Pass      Other      CKEditorTest.php    43 Drupal\ckeditor\Tests\CKEditorTest-
    Enabled modules: system, user, filter, editor, ckeditor, filter_test.

After:

Pass      Other      CKEditorTest.php    46 Drupal\ckeditor\Tests\CKEditorTest-
    Installed system tables: {url_alias}.
Pass      Other      CKEditorTest.php   289 Drupal\ckeditor\Tests\CKEditorTest-
    "Internal" plugin configuration built correctly for default toolbar.
Pass      Other      CKEditorTest.php   299 Drupal\ckeditor\Tests\CKEditorTest-
    "Internal" plugin configuration built correctly for customized toolbar.
Pass      Other      CKEditorTest.php    43 Drupal\ckeditor\Tests\CKEditorTest-
    Enabled modules: system, user, filter, editor, ckeditor, filter_test.
aspilicious’s picture

aspilicious commented

Minor point:

The order of lines is different in both blocks. Can we fix that?
In the second block the two ksorts are just after each other in the first block they are not.
Pick one approach and use it for both.

stefan.r’s picture

stefan.r commented
StatusFileSize
new 2465917-3.patch1.59 KB
damien tournoud’s picture

damien tournoud commented

Isn't ->assertEqual() meant precisely for that?

aspilicious’s picture

aspilicious commented
Status: Needs review » Reviewed & tested by the community

Looking good :)

damien tournoud’s picture

damien tournoud commented
Status: Reviewed & tested by the community » Needs work

Also. Strict array comparisons (i.e. ->assertIdentical()) have always been order-sensitive. So the order here changed for some reason. Can we track out why, and make sure this is not the symptom of an actual bug somewhere else?

stefan.r’s picture

stefan.r commented
Status: Needs work » Needs review
StatusFileSize
new 2465917-7.patch1.36 KB

@Damien Tournoud well the thing that's changed is PHP7 builds its arrays differently (see https://github.com/bolt/bolt/issues/3275), I will dig a bit, isolate this issue and see if the different sorting introduces an actual bug.

Status: Needs review » Needs work

The last submitted patch, 7: 2465917-7.patch, failed testing.

stefan.r’s picture

stefan.r commented
Status: Needs work » Fixed
StatusFileSize
new Capture d’écran 2015-04-06 à 18.45.08.png156.3 KB
new 2465917-9.patch1.36 KB

assertEqual makes tests pass as well -- other tests do do assertIdentical + ksort($expected), but we need to sort the array with the actual result as well in this case, so may as well do assertEqual if we're going to deviate.

The only difference between the expected and the actual array is that the 'disallowedContent' and 'allowedContent' keys are inverted in PHP7, due to this line of code:

    list($config['allowedContent'], $config['disallowedContent']) = $this->generateACFSettings($editor);

Which works differently in PHP7:

stefan.r’s picture

stefan.r commented
Status: Fixed » Needs review
damien tournoud’s picture

damien tournoud commented
Status: Needs review » Reviewed & tested by the community

That's a known side effect of the introduction of the AST in PHP7. ->assertEqual() is what the test intended to use, so let's go with this.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Great sleuthing here, folks!

Committed and pushed to 8.0.x. Thanks!

  • webchick committed 3bd051f on 8.0.x 
    Issue #2465917 by stefan.r, Damien Tournoud: CKEditor test fails in PHP7

Status: Fixed » Closed (fixed)

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