Since moving my server over to PHP 7.3, I get the following warnings in my PHP error log:

PHP Warning: "continue" targeting switch is equivalent to "break". Did you mean to use "continue 2"? in /{path-to-modules}/advagg/advagg_js_compress/jshrink.inc on line 276

See https://www.php.net/manual/en/migration73.incompatible.php, section headed "Continue Targeting Switch issues Warning"

CommentFileSizeAuthor
#10 advagg_replace_continue_with_break-3058949-10.patch590 bytesjamesoakley
#2 advagg_replace_continue_with_break-3058949-2.patch596 bytesjamesoakley

Comments

JamesOakley created an issue. See original summary.

jamesoakley’s picture

jamesoakley
Primary language English
Location North Yorkshire, UK
commented
Status: Active » Needs review
StatusFileSize
new advagg_replace_continue_with_break-3058949-2.patch596 bytes

Looking at the code, this happens in a "default" section of the switch block. So of the two alternatives ("continue 2", "break"), "continue 2" would make no sense. So it seems safe to me to replace this with the "break" statement that the PHP devs are steering us towards.

thalles’s picture

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

But drupal 7 dont support php 7.3, @see: https://www.drupal.org/docs/7/system-requirements/php-requirements#php_r...

jamesoakley’s picture

jamesoakley
Primary language English
Location North Yorkshire, UK
commented

But we should presumably work to make as much core / contrib code 7.3 compatible as possible. It only becomes an issue if 7.3 compliance would then break the ability to work with older versions of PHP that are officially supported.

Making the change I'm proposing in the patch would not cause any problems for people running on older versions (unless someone with better PHP than me says otherwise). So there's nothing lost in fixing this.

thalles’s picture

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

Make sense for me, thanks for your explanations!

  • thalles committed c303086 on 7.x-2.x authored by JamesOakley 
    Issue #3058949 by JamesOakley, thalles: "continue" statements should not...
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.

rafal.sereda’s picture

rafal.sereda commented
Version: 7.x-2.x-dev » 8.x-4.0

Hello, do you know that this issue still exists in the 8.x module version? I think there is no reason to not apply this patch to 8.x versions, WDYT?

jamesoakley’s picture

jamesoakley
Primary language English
Location North Yorkshire, UK
commented
Version: 8.x-4.0 » 8.x-4.x-dev
StatusFileSize
new advagg_replace_continue_with_break-3058949-10.patch590 bytes

You raise a good point, Rafal, because the corresponding line of code is there in 8.x-4.x, so will trigger the same warnings in PHP 7.3 and above.

Here's the equivalent patch. But, the continue statement to replace there was continue 3, not just plain continue, so it may be that there are unwanted side effects of replacing it with break in this case.

tobiasb’s picture

tobiasb
Primary language German
Location Berlin
commented

This already fixed in 8.x. See #3068558: JShrink and PHP 7.3 issues.

tobiasb’s picture

tobiasb
Primary language German
Location Berlin
commented
Related issues: +#3068558: JShrink and PHP 7.3 issues
jamesoakley’s picture

jamesoakley
Primary language English
Location North Yorkshire, UK
commented

Thanks, yes - I've re-read https://www.php.net/manual/en/migration73.incompatible.php#migration73.i... and it's only vanilla "continue" that triggers warnings inside a switch block. continue 2, (or in this case continue 3) is fine. So the section of code targeted by my patch above has already been fixed, as you say.

Rafal - happy?