I'm getting several JShrink related error messages concerning JShrink:

  • continue in break should target the while loop directly
  • strpos() doesn't longer accept non-string needles

These are already discussed & fixed upstream in the official JShrink library: https://github.com/tedious/JShrink/pull/78 .

CommentFileSizeAuthor
#3 advagg-replace_continue_with_break-3068558-3.patch1.03 KBrp7

Comments

rp7 created an issue. See original summary.

rp7’s picture

rp7 commented
Title: Deprecated function: strpos(): Non-string needles will be interpreted as strings in the future. » JShrink and PHP 7.3 issues
rp7’s picture

rp7 commented
StatusFileSize
new advagg-replace_continue_with_break-3068558-3.patch1.03 KB

Patch attached applies the fixes that were applied for the JShrink library @ https://github.com/tedious/JShrink/commit/21254058dc3ce6aba6bef458cff4bf... .

rp7’s picture

rp7 commented
Status: Active » Needs review
zrhoffman’s picture

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

I can reproduce the issue in a fresh install using PHP 7.3 and have verified rp7's patch in #3. `Drupal\Tests\advagg_js_minify\Kernel\Asset\JsMinifierUnitTest` fails before the patch and succeeds afterwards:

`continue` within `switch`
Without #3 With #3 
4cf827daa9d3:/app$ php web/core/scripts/run-tests.sh --verbose --non-html --php /usr/bin/php --sqlite /tmp/a/test.sqlite --dburl pgsql://dbuser:dbpass@dbhost/dbname advagg_js_minify

Drupal test run
---------------

Tests to be run:
  - Drupal\Tests\advagg_js_minify\Kernel\Asset\JsMinifierUnitTest

Test run started:
  Sunday, August 18, 2019 - 12:37

Test summary
------------

Drupal\Tests\advagg_js_minify\Kernel\Asset\JsMinifierUnitTes   0 passes             1 exceptions
FATAL Drupal\Tests\advagg_js_minify\Kernel\Asset\JsMinifierUnitTest: test runner returned a non-zero error code (2).
Drupal\Tests\advagg_js_minify\Kernel\Asset\JsMinifierUnitTes   0 passes   1 fails

Test run duration: 20 sec

Detailed test results
---------------------


---- Drupal\Tests\advagg_js_minify\Kernel\Asset\JsMinifierUnitTest ----


Status    Group      Filename          Line Function
--------------------------------------------------------------------------------
Exception Other      phpunit-3.xml        0 Drupal\Tests\advagg_js_minify\Kerne
    PHPunit Test failed to complete; Error: PHPUnit 6.5.14 by Sebastian
    Bergmann and contributors.

    Testing Drupal\Tests\advagg_js_minify\Kernel\Asset\JsMinifierUnitTest
    ..............EEE.                                                18 / 18
    (100%)

    Time: 19.3 seconds, Memory: 8.00MB

    There were 3 errors:

    1)
    Drupal\Tests\advagg_js_minify\Kernel\Asset\JsMinifierUnitTest::testMinifyJshrink
    with data set #0 (array('/app/web/modules/contrib/adva...pal.js'), '/**\n *
    @file\n * Defines the...ns);\n')
    "continue" targeting switch is equivalent to "break". Did you mean to use
    "continue 2"?

    /app/web/modules/contrib/advagg/advagg_js_minify/jshrink.inc:269
    /app/web/modules/contrib/advagg/advagg_js_minify/src/Asset/JsMinifier.php:244
    /app/web/modules/contrib/advagg/advagg_js_minify/src/Asset/JsMinifier.php:244
    /app/web/modules/contrib/advagg/advagg_js_minify/src/Asset/JsMinifier.php:54
    /app/web/modules/contrib/advagg/advagg_js_minify/tests/src/Kernel/Asset/JsMinifierUnitTest.php:202

    2)
    Drupal\Tests\advagg_js_minify\Kernel\Asset\JsMinifierUnitTest::testMinifyJshrink
    with data set #1 (array('/app/web/modules/contrib/adva...jax.js'), '/**\n *
    @file\n * Provides Aj...gs);\n')
    "continue" targeting switch is equivalent to "break". Did you mean to use
    "continue 2"?

    /app/web/modules/contrib/advagg/advagg_js_minify/jshrink.inc:269
    /app/web/modules/contrib/advagg/advagg_js_minify/src/Asset/JsMinifier.php:244
    /app/web/modules/contrib/advagg/advagg_js_minify/src/Asset/JsMinifier.php:244
    /app/web/modules/contrib/advagg/advagg_js_minify/src/Asset/JsMinifier.php:54
    /app/web/modules/contrib/advagg/advagg_js_minify/tests/src/Kernel/Asset/JsMinifierUnitTest.php:202

    3)
    Drupal\Tests\advagg_js_minify\Kernel\Asset\JsMinifierUnitTest::testMinifyJshrink
    with data set #2 (array('/app/web/modules/contrib/adva...iew.js'), '/**\n *
    @file\n * A Backbone ...e));\n')
    "continue" targeting switch is equivalent to "break". Did you mean to use
    "continue 2"?

    /app/web/modules/contrib/advagg/advagg_js_minify/jshrink.inc:269
    /app/web/modules/contrib/advagg/advagg_js_minify/src/Asset/JsMinifier.php:244
    /app/web/modules/contrib/advagg/advagg_js_minify/src/Asset/JsMinifier.php:244
    /app/web/modules/contrib/advagg/advagg_js_minify/src/Asset/JsMinifier.php:54
    /app/web/modules/contrib/advagg/advagg_js_minify/tests/src/Kernel/Asset/JsMinifierUnitTest.php:202

    ERRORS!
    Tests: 18, Assertions: 28, Errors: 3.
Fail      run-tests. Unknown              0 Unknown
    FATAL Drupal\Tests\advagg_js_minify\Kernel\Asset\JsMinifierUnitTest: test
    runner returned a non-zero error code (2).
 
4cf827daa9d3:/app$ php web/core/scripts/run-tests.sh --verbose --non-html --php /usr/bin/php --sqlite /tmp/a/test.sqlite --dburl pgsql://dbuser:dbpass@dbhost/dbname advagg_js_minify

Drupal test run
---------------

Tests to be run:
  - Drupal\Tests\advagg_js_minify\Kernel\Asset\JsMinifierUnitTest

Test run started:
  Sunday, August 18, 2019 - 12:38

Test summary
------------

Drupal\Tests\advagg_js_minify\Kernel\Asset\JsMinifierUnitTes  18 passes

Test run duration: 20 sec

Detailed test results
---------------------


---- Drupal\Tests\advagg_js_minify\Kernel\Asset\JsMinifierUnitTest ----


Status    Group      Filename          Line Function
--------------------------------------------------------------------------------
Pass      Other      JsMinifierUnitTes   41 Drupal\Tests\advagg_js_minify\Kerne

Pass      Other      JsMinifierUnitTes   95 Drupal\Tests\advagg_js_minify\Kerne

Pass      Other      JsMinifierUnitTes   95 Drupal\Tests\advagg_js_minify\Kerne

Pass      Other      JsMinifierUnitTes   95 Drupal\Tests\advagg_js_minify\Kerne

Pass      Other      JsMinifierUnitTes   95 Drupal\Tests\advagg_js_minify\Kerne

Pass      Other      JsMinifierUnitTes  141 Drupal\Tests\advagg_js_minify\Kerne

Pass      Other      JsMinifierUnitTes  141 Drupal\Tests\advagg_js_minify\Kerne

Pass      Other      JsMinifierUnitTes  141 Drupal\Tests\advagg_js_minify\Kerne

Pass      Other      JsMinifierUnitTes  161 Drupal\Tests\advagg_js_minify\Kerne

Pass      Other      JsMinifierUnitTes  161 Drupal\Tests\advagg_js_minify\Kerne

Pass      Other      JsMinifierUnitTes  161 Drupal\Tests\advagg_js_minify\Kerne

Pass      Other      JsMinifierUnitTes  177 Drupal\Tests\advagg_js_minify\Kerne

Pass      Other      JsMinifierUnitTes  177 Drupal\Tests\advagg_js_minify\Kerne

Pass      Other      JsMinifierUnitTes  177 Drupal\Tests\advagg_js_minify\Kerne

Pass      Other      JsMinifierUnitTes  199 Drupal\Tests\advagg_js_minify\Kerne

Pass      Other      JsMinifierUnitTes  199 Drupal\Tests\advagg_js_minify\Kerne

Pass      Other      JsMinifierUnitTes  199 Drupal\Tests\advagg_js_minify\Kerne

Pass      Other      JsMinifierUnitTes  208 Drupal\Tests\advagg_js_minify\Kerne

Note that the above test only covers the `continue` within `switch` bug. In order to reproduce the "`strpos()` with `false` as needle" bug at advagg_js_minify/jshrink.inc#L226 , one of the unminified JavaScript files provided by Drupal\Tests\advagg_js_minify\Kernel\Asset\JsMinifierUnitTest::providerTestMinification() (drupal.js, ajax.js, and ToolbarVisualView.js) would need to end with )<newline> instead of ;<newline> (or add an additional JavaScript file to test). Once that is done, and if advagg_js_minify/jshrink.inc#L269 is patched from #3 but advagg_js_minify/jshrink.inc#L226 is not, the `strpos()` with `false` as needle fix can be verified:

`strpos()` with `false` as needle
Without #3 With #3 
9e2fe70c44d4:/app$ php web/core/scripts/run-tests.sh --verbose --non-html --php /usr/bin/php --sqlite /tmp/a/test.sqlite --dburl pgsql://dbuser:dbpass@dbhost/dbname --class 'Drupal\Tests\advagg_js_minify\Kernel\Asset\JsMinifierUnitT
est'

Drupal test run
---------------

Tests to be run:
  - Drupal\Tests\advagg_js_minify\Kernel\Asset\JsMinifierUnitTest

Test run started:
  Sunday, August 18, 2019 - 16:37

Test summary
------------

Drupal\Tests\advagg_js_minify\Kernel\Asset\JsMinifierUnitTes   0 passes   1 fails

Test run duration: 6 sec

Detailed test results
---------------------


---- Drupal\Tests\advagg_js_minify\Kernel\Asset\JsMinifierUnitTest ----


Status    Group      Filename          Line Function
--------------------------------------------------------------------------------
Fail      Other      phpunit-28.xml       0 Drupal\Tests\advagg_js_minify\Kerne
    PHPunit Test failed to complete; Error: PHPUnit 6.5.14 by Sebastian
    Bergmann and contributors.

    Testing Drupal\Tests\advagg_js_minify\Kernel\Asset\JsMinifierUnitTest
    ..................                                                18 / 18
    (100%)

    Time: 6.13 seconds, Memory: 6.00MB

    OK (18 tests, 34 assertions)

    Unsilenced deprecation notices (1)

      1x: strpos(): Non-string needles will be interpreted as strings in the
    future. Use an explicit chr() call to preserve the current behavior
        1x in JsMinifierUnitTest::testMinifyJshrink from
    Drupal\Tests\advagg_js_minify\Kernel\Asset
 
9e2fe70c44d4:/app$ php web/core/scripts/run-tests.sh --verbose --non-html --php /usr/bin/php --sqlite /tmp/a/test.sqlite --dburl pgsql://dbuser:dbpass@dbhost/dbname --class 'Drupal\Tests\advagg_js_minify\Kernel\Asset\JsMinifierUnitT
est'

Drupal test run
---------------

Tests to be run:
  - Drupal\Tests\advagg_js_minify\Kernel\Asset\JsMinifierUnitTest

Test run started:
  Sunday, August 18, 2019 - 16:38

Test summary
------------

Drupal\Tests\advagg_js_minify\Kernel\Asset\JsMinifierUnitTes  18 passes

Test run duration: 6 sec

Detailed test results
---------------------


---- Drupal\Tests\advagg_js_minify\Kernel\Asset\JsMinifierUnitTest ----


Status    Group      Filename          Line Function
--------------------------------------------------------------------------------
Pass      Other      JsMinifierUnitTes   41 Drupal\Tests\advagg_js_minify\Kerne

Pass      Other      JsMinifierUnitTes   95 Drupal\Tests\advagg_js_minify\Kerne

Pass      Other      JsMinifierUnitTes   95 Drupal\Tests\advagg_js_minify\Kerne

Pass      Other      JsMinifierUnitTes   95 Drupal\Tests\advagg_js_minify\Kerne

Pass      Other      JsMinifierUnitTes   95 Drupal\Tests\advagg_js_minify\Kerne

Pass      Other      JsMinifierUnitTes  141 Drupal\Tests\advagg_js_minify\Kerne

Pass      Other      JsMinifierUnitTes  141 Drupal\Tests\advagg_js_minify\Kerne

Pass      Other      JsMinifierUnitTes  141 Drupal\Tests\advagg_js_minify\Kerne

Pass      Other      JsMinifierUnitTes  161 Drupal\Tests\advagg_js_minify\Kerne

Pass      Other      JsMinifierUnitTes  161 Drupal\Tests\advagg_js_minify\Kerne

Pass      Other      JsMinifierUnitTes  161 Drupal\Tests\advagg_js_minify\Kerne

Pass      Other      JsMinifierUnitTes  177 Drupal\Tests\advagg_js_minify\Kerne

Pass      Other      JsMinifierUnitTes  177 Drupal\Tests\advagg_js_minify\Kerne

Pass      Other      JsMinifierUnitTes  177 Drupal\Tests\advagg_js_minify\Kerne

Pass      Other      JsMinifierUnitTes  199 Drupal\Tests\advagg_js_minify\Kerne

Pass      Other      JsMinifierUnitTes  199 Drupal\Tests\advagg_js_minify\Kerne

Pass      Other      JsMinifierUnitTes  199 Drupal\Tests\advagg_js_minify\Kerne

Pass      Other      JsMinifierUnitTes  208 Drupal\Tests\advagg_js_minify\Kerne
acbramley’s picture

acbramley commented

Can confirm #5, I have triggered automated tests on the patch as well.

thalles’s picture

thalles
Primary language Portuguese, Brazil
Location Teófilo Otoni - MG
commented

Thanks everyone, looks good to me!

  • thalles committed 43bdd2e on 8.x-4.x authored by rp7 
    Issue #3068558 by rp7, zrhoffman, acbramley, thalles: JShrink and PHP 7....
thalles’s picture

thalles
Primary language Portuguese, Brazil
Location Teófilo Otoni - MG
commented
Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

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