Problem/Motivation

The version of JShrink included with 8.x-4.1 is broken when used with PHP 8 (https://github.com/tedious/JShrink/issues/96). It hangs because of a change made to the return value of substr(), eventually timing out depending on your maximum execution time.

Steps to reproduce

- install on a site using PHP 8
- choose the JShrink minifier
- watch the site either whitescreen or hang, depending on your server setup

Proposed resolution

Update JShrink to 1.4.0 or later.

Remaining tasks

Review patch.

User interface changes

None.

API changes

None.

Data model changes

None.

CommentFileSizeAuthor
#27 3252394screenshot.png120.29 KBcharginghawk
#25 jshrink-update-d7.patch5 KBThangaraj Moorthi
#2 advagg-jshrink-update.3252394-1.patch33.35 KBksenzee

Issue fork advagg-3252394

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

ksenzee created an issue. See original summary.

ksenzee’s picture

ksenzee
she/her
commented
Status: Active » Needs review
StatusFileSize
new advagg-jshrink-update.3252394-1.patch33.35 KB

I copied and pasted from https://github.com/tedious/JShrink/blob/v1.4.0/src/JShrink/Minifier.php, and put the codingstandards @ignores and license info at the top of the file back to what they were before. I'm not at all convinced this is the right way to include this library in 2021, but this patch will make JShrink stop whitescreening and actually work under PHP 8, so it's an improvement.

kburakozdemir’s picture

kburakozdemir commented

#2 is working without any problem under PHP 8.0.13.

vasike’s picture

vasike
Location Ramnicu Valcea
commented
Status: Needs review » Reviewed & tested by the community

i'm confirming the issue, BUT for PHP 7.4
and also that the patch ... put back the site "online"

boulaffasae’s picture

boulaffasae commented

+1 RTBC PHP 8.1

tetem’s picture

tetem commented

I have same issue with drupal 9 and PHP 8

Any potential solutions for fixing this problem?

Thanks.

bobooon’s picture

bobooon commented

+1 RTBC

heddn’s picture

heddn
Primary language English
Location Illinois
commented

RTBC++

klaasvw’s picture

klaasvw commented

Another + for RTBC, fixed a max_execution_time error on PHP 8.1 for us.

charginghawk’s picture

charginghawk commented

+1 RTBC

eduardo morales alberti’s picture

eduardo morales alberti
Primary language Spanish
Location Spain, 🇪🇺
commented

+1 RTBC PHP 8.1

akalam’s picture

akalam commented

+1 RTBC

platinum1’s picture

platinum1 commented

+1 RTBC

tzt20’s picture

tzt20 commented

+1 RTBC

the.tai.pen@gmail.com’s picture

the.tai.pen@gmail.com commented

+1 RTBC

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Version: 8.x-4.x-dev » 5.0.0

Moving to 5.0.0 version (but probably should have a branch)

Eduardo Morales Alberti opened merge request !17

Eduardo Morales Alberti opened merge request !18

fathima.asmat’s picture

fathima.asmat commented

+1 RTBC. would be good to get this over to. anew release :)

Sivakumaran’s picture

Sivakumaran commented
Version: 5.0.0 » 7.x-2.35

I'm updated my site to PHP 8.0
Using drupal version of 7
Facing the same issue "Maximum execution time of 240 seconds exceeded"
Is there any patch available for D7 ?
Thanks in advance.

Thangaraj Moorthi’s picture

Thangaraj Moorthi
Location coimbatore
commented

Drupal version: 7.91
Advagg version: 7.x-2.35.
Recently i upgraded my site to PHP8.0
Now i am facing this same error: Maximum execution time of 240 seconds exceeded sites/all/modules/contrib/advagg/advagg_js_compress/jshrink.inc
Is there any fixes or patch available for advagg 7.x-2.35 ?

gisle’s picture

gisle
he/him
Primary language Norwegian Bokmål
Location Norway
commented
Version: 7.x-2.35 » 5.0.0
Issue tags: +Needs backport to D7

Fixes always goes to latest version (i.e. 5.0.x).

Based on comments #20 and #23, it shall need backport to Drupal 7.

Thangaraj Moorthi’s picture

Thangaraj Moorthi
Location coimbatore
commented
StatusFileSize
new jshrink-update-d7.patch5 KB

Hey Guys,

Here is the patch form Drupal 7,

It'll be the fixes for "JShrink incompatible with PHP 8" and "Maximum execution time".

Thanks.

thalles’s picture

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

Is the merge request in #18 the same as the patch in #2?

charginghawk’s picture

charginghawk commented
StatusFileSize
new 3252394screenshot.png120.29 KB

@thalles They're the same. You can download the diff from the #18 commit here:

https://git.drupalcode.org/project/advagg/-/merge_requests/18

Screenshot of Gitlab commit interface, highlighting the "Code > Download > Plain Diff" option

The diff between that and the patch is trivial:

% diff advagg-jshrink-update.3252394-1.patch 18.diff                           
2c2
< index 4d2400a..0b9cd18 100644
---
> index 4d2400a27db5111fa4c9127fe7f63ec8ba639efa..0b9cd1894caa2af1cc46c6cbe822e38adbd45291 100644
ronino’s picture

ronino commented

#25 fixed it for me on D7. Thanks a lot!

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented

Hiding the D7 patch from #25 because it needs work but we need to get this into D9 before that and I don't want to have those patches conflate this issue or hold it up. The patch in #25 has too many short array syntax changes that are unneeded and would need a PHP requirement for Drupal 7. Maybe it would be worth creating a separate issue for this on D7?

saschaeggi’s picture

saschaeggi
he/him
commented

+1 RTBC

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented

Regarding @ksenzee comment in #2, and for all D7 users it does already look for libraries, so you can add the v1.4.0 of this tedivm/jshrink PHP library to that directory in a sites/all/libraries/jshrink directory using composer (I'm using dealerdirect/phpcodesniffer-composer-installer composer plugin) or just copy the files there and it will load.
So then you don't need this patch to be fixed in D7, but a project page note would help.

nickdickinsonwilde’s picture

nickdickinsonwilde
He/Him/His
Primary language English
Location Victoria, BC (T'So-uke lands)
commented
Version: 5.0.0 » 6.0.x-dev

Committed to 6.0.x, thanks everyone!

nickdickinsonwilde’s picture

nickdickinsonwilde
He/Him/His
Primary language English
Location Victoria, BC (T'So-uke lands)
commented
Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

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

dpagini’s picture

dpagini commented

Any chance this can be backported to 5.x?