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
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

ksenzee created an issue. See original summary.

ksenzee’s picture

ksenzee
she/her
CreditAttribution: ksenzee at Smartsheet commented
Status: Active » Needs review
FileSize
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 CreditAttribution: kburakozdemir as a volunteer and at binbiriz commented

#2 is working without any problem under PHP 8.0.13.

vasike’s picture

vasike
Location Ramnicu Valcea
CreditAttribution: vasike as a volunteer 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 CreditAttribution: boulaffasae as a volunteer and at Fullwave commented

+1 RTBC PHP 8.1

tetem’s picture

tetem CreditAttribution: tetem commented

I have same issue with drupal 9 and PHP 8

Any potential solutions for fixing this problem?

Thanks.

robphillips’s picture

robphillips CreditAttribution: robphillips commented

+1 RTBC

heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn at MTech, LLC, Tag1 Consulting commented

RTBC++

klaasvw’s picture

klaasvw CreditAttribution: klaasvw at Randstad Digital commented

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

charginghawk’s picture

charginghawk CreditAttribution: charginghawk at Pegasystems commented

+1 RTBC

Eduardo Morales Alberti’s picture

Eduardo Morales Alberti
Primary language Spanish
Location Spain, 🇪🇺
CreditAttribution: Eduardo Morales Alberti as a volunteer and at Metadrop commented

+1 RTBC PHP 8.1

akalam’s picture

akalam CreditAttribution: akalam at Dropsolid commented

+1 RTBC

platinum1’s picture

platinum1 CreditAttribution: platinum1 commented

+1 RTBC

tzt20’s picture

tzt20 CreditAttribution: tzt20 as a volunteer commented

+1 RTBC

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

the.tai.pen@gmail.com CreditAttribution: the.tai.pen@gmail.com at FFW commented

+1 RTBC

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet at The University of British Columbia 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 CreditAttribution: fathima.asmat at CTI Digital commented

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

Sivakumaran’s picture

Sivakumaran CreditAttribution: Sivakumaran as a volunteer 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
CreditAttribution: Thangaraj Moorthi at Dialed In Design 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
CreditAttribution: gisle at Hannemyr Nye Medier AS for Ramsalt Lab 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
CreditAttribution: Thangaraj Moorthi at Dialed In Design commented
FileSize
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
CreditAttribution: thalles at MidiaUAI, Dexa commented

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

charginghawk’s picture

charginghawk CreditAttribution: charginghawk at Pegasystems commented
FileSize
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 CreditAttribution: Ronino commented

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

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet at The University of British Columbia 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
CreditAttribution: saschaeggi as a volunteer commented

+1 RTBC

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet at The University of British Columbia 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)
CreditAttribution: NickDickinsonWilde as a volunteer and at Taoti Creative 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)
CreditAttribution: NickDickinsonWilde as a volunteer and at Taoti Creative 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 CreditAttribution: dpagini at Voya Financial commented

Any chance this can be backported to 5.x?