Problem/Motivation
If you are using advagg_js_compress which is a submodule and run the cron then cron goes into infinite loop.
Steps to reproduce
- Install the advagg and advagg_js_compress
- Run the cron via UI or drush
- It will never come back
Proposed resolution
- Issue in sites/all/modules/contrib/advagg/advagg_js_compress/jshrink.inc line 327. In PHP 7 substr returns the false but in PHP 8 it returns the '' empty string.
Remaining tasks
- Create the patch.
User interface changes
- No
API changes
- No
Data model changes
- No
| Comment | File | Size | Author |
|---|---|---|---|
| #16 | advagg-php80-cron-stuck-issue-3277342-16.patch | 514 bytes | alex.skrypnyk |
| #6 | advagg-php80-cron-stuck-issue-3277342-2.patch | 518 bytes | Dmitrii_Zadorozhnyi |
| #2 | advagg-php80-cron-stuck-issue-3277342-1.patch | 493 bytes | asghar |
Issue fork advagg-3277342
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:
Comments
Comment #2
asghar commentedComment #5
alan-ps commentedHi @asghar, I think we should keep backward compatibility here. So, I would rather use explicit casting to solve an issue
Comment #6
Dmitrii_Zadorozhnyi commentedHi!
For backward compatibility with PHP 7, i would like to change patch, according to https://www.php.net/manual/en/function.substr.php
For PHP 8 - "The function returns an empty string where it previously returned false."
Comment #7
darkodev commentedComment #8
darkodev commentedAny reason we shouldn't take the approach used for D9 version and do a wholesale replacement of jshrink.inc? https://www.drupal.org/project/advagg/issues/3252394
Comment #9
darkodev commentedThe patch in #6 works for us and applies cleanly to latest stable.
Drupal 7.92
Advagg 7.x-2.35
PHP 8.0.21
I tried the replacement of the whole jshrink.inc file from the D9 fix, and it also seems to work.
The solution there is the avoidance of substr()
Comment #10
eugene bocharov commented+1 for #6. Just maybe
isset($char)can be removed, because $char is always defined there.Not sure we can use latest jshrink version while D7 minimum requirements is PHP 5.3, but for latest jshrink PHP 5.6.
About #9 - it seems that
$this->lenis undefined. The fragment can't be used without other code replacement.Comment #11
avpadernoComment #12
selinav commentedSame problem with Drup 9.4.8 and php 8.0.26
It works with the #2 patch.
Thank you.
Comment #13
wamwam commentedDrupal 7.94
PHP 8.0.21 or PHP 7.4.33
Advagg 7.x-2.35
After applying the patch, the Status report /admin/reports/status and Site configuration /admin/config stop responding. No problems seen on other pages.
If you cancel the patch everything works fine
Comment #14
joelpittetI agree with @darkodev in #8 we should use #3252394-31: JShrink incompatible with PHP 8 I have a comment in #31 with a better solution than backporting the hardcoded class changes
Comment #15
damienmckenna#6 is working for me, setting this to RTBC as a simple solution.
Comment #16
alex.skrypnykRe-roll against latest released version (#6 does not apply) 7.x-2.35
Comment #17
thalles@joelpittet, can you send us a patch with jShrink new version?
Comment #18
poker10 commentedThanks for working on this!
Unless we want to intentionally drop support for PHP < 5.6, I think I would prefer the simple fix in #6 (see the comment #10). I am also not sure if someone will be able to provide the more complicated patch as suggested by #8 in this D7 phase.
Comment #19
init90In my case, it leads to server lagging and interaction of site works. I've increased priority to higher.
The patch from #16 fixed the problem.
Comment #20
Talkless commented#16 worked for me!
Comment #21
hargobind#16 works for my sites as well.
Since this issue priority is Major, marking RTBC unless @poker10 or other maintainers have something to say about it.
The commit in #16 can go in now, and then someone can work on the backport for jShrink at a later date if it's still important.
Comment #23
poker10 commentedEven though there is a workaround mentioned in #3252394-31: JShrink incompatible with PHP 8, I think that it is worth fixing this as it is a small fix.
For others needing a newer jShrink, please check the approach mentioned in the comment #31 on the link above.
Committed and pushed this to 7.x-2.x, thanks!