Files: 
CommentFileSizeAuthor
#5 javascript-array-illegal-offset-2056737-5-HEAD-test-only.patch1.06 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 57,616 pass(es).
[ View ]
#5 javascript-array-illegal-offset-2056737-5-patch-test-only.patch2.07 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 57,838 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#5 javascript-array-illegal-offset-2056737-5.patch1.73 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 57,892 pass(es).
[ View ]
#1 javascript-array-illegal-offset-2056737-1.patch1.01 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 57,888 pass(es).
[ View ]

Comments

Berdir’s picture

Status:Active» Needs review
StatusFileSize
new1.01 KB
PASSED: [[SimpleTest]]: [MySQL] 57,888 pass(es).
[ View ]

This is green.

As often with this, this could actually indicate a real and weird bug with this shorthand version. Because what happens is basically this:

<?php
$string
= 'Hello there';
$string['group'] = -100;
print
$string;
-
ello there
?>

Not sure if there's a useful way to test this, though.

Berdir’s picture

Title:Fix Javascript illegal string offset test exceptions in PHP 5.4» Illegal string offset test exceptions in PHP 5.4 in drupal_add_library()
alexpott’s picture

Status:Needs review» Reviewed & tested by the community

I can confirm that this fixes the issue. I've tested on php 5.4 and 5.5 and the fix looks great.

webchick’s picture

Status:Reviewed & tested by the community» Needs review
Issue tags:+Needs tests

I think we need test coverage for this new chunk of code, no?

Berdir’s picture

StatusFileSize
new1.73 KB
PASSED: [[SimpleTest]]: [MySQL] 57,892 pass(es).
[ View ]
new2.07 KB
FAILED: [[SimpleTest]]: [MySQL] 57,838 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new1.06 KB
PASSED: [[SimpleTest]]: [MySQL] 57,616 pass(es).
[ View ]

Well, we already have test coverage for that case, that's what's causing the exception in 5.4. The problem is that the bug shows in a weird way, not sure how to write a useful test for this:

What happens in 8.x is this output:

<script src="http://d8/core/assets/vendor/jquery/jquery.js?v=2.0"></script>
<script src="http://d8/core/assets/vendor/jquery.ui/external/jquery.cookie.js?v=1.10.2"></script>
<script src="http://d8/core/assets/vendor/jquery-form/jquery.form.js?v=3.27"></script>
<script src="http://d8/core/assets/vendor/farbtastic/farbtastic.js?v=1.2"></script>
<script src="http://d8/core/modules/system/tests/modules/common_test/js/shorthand.js?v=0.8.3.37"></script>
<script src="http://d8/-ore/modules/system/tests/modules/common_test/js/shorthand.js?0"></script>

And with the fix

<script src="http://d8/core/assets/vendor/jquery/jquery.js?v=2.0"></script>
<script src="http://d8/core/assets/vendor/jquery.ui/external/jquery.cookie.js?v=1.10.2"></script>
<script src="http://d8/core/assets/vendor/jquery-form/jquery.form.js?v=3.27"></script>
<script src="http://d8/core/assets/vendor/farbtastic/farbtastic.js?v=1.2"></script>
<script src="http://d8/core/modules/system/tests/modules/common_test/js/shorthand.js?0"></script>

So in 8.x, we have two entries. One that is correct, including the correct version and a broken one with the -.

Looking into this, I noticed that the shorthand => array() conversion already happens in drupal_get_library(), but it doesnt' remove the old entry. And my change actually broke the correct version handling there. So removed that and added an unset() in drupal_get_library().

The assert once test addition is kinda weird, but this combined should fail both with my previous patch and HEAD.

webchick++

alexpott’s picture

Status:Needs review» Reviewed & tested by the community

So as we don't have any 5.4 or 5.5 bots we're not going to see any fails here.

I can confirm that on 5.4 and 5.5 javascript-array-illegal-offset-2056737-5-HEAD-test-only.patch fails with an exception and on the assertion that only 1 shorthand.js files in the array.

I can also confirm that javascript-array-illegal-offset-2056737-5.patch fixes the exception and passes the test. Nice work @berdir and @webchick. This look even better to me than the patch in #1 - so rtbc :)

Berdir’s picture

+++ b/core/includes/common.incundefined
@@ -2761,6 +2761,17 @@ function drupal_add_library($module, $name, $every_page = NULL) {
           if ($type == 'js' && !isset($options['group'])) {

Ah, the reason the first test doesn't fail on 5.3 is this.

This is already converted to !isset($options[0]) and $options is a string, so it is set and never goes into the if.

Crazy stuff.

Dries’s picture

Status:Reviewed & tested by the community» Fixed

Great work and RTBC per Alex. Committed to 8.x. Thanks!

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