Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes #4397 Guard against preg_replace() errors when moving meta charset to head #4413

Merged
merged 1 commit into from
Oct 13, 2021

Conversation

Tabrisrp
Copy link
Contributor

@Tabrisrp Tabrisrp commented Oct 7, 2021

Description

This PR adds guards to move_meta_charset_to_head(), to always return a string in case one of the preg_replace() encounters an error and returns null instead of the expected string value.

Fixes #4397

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Is the solution different from the one proposed during the grooming?

No grooming done

How Has This Been Tested?

  • Automated tests

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • My changes generate no new warnings
  • Existing unit tests pass locally with my changes

@Tabrisrp Tabrisrp added type: bug Indicates an unexpected problem or unintended behavior module: delay JS labels Oct 7, 2021
@Tabrisrp Tabrisrp added this to the 3.10.1 milestone Oct 7, 2021
@Tabrisrp Tabrisrp requested a review from a team October 7, 2021 14:00
@Tabrisrp Tabrisrp self-assigned this Oct 7, 2021
@Tabrisrp Tabrisrp requested a review from a team October 8, 2021 15:39
@Mai-Saad Mai-Saad self-requested a review October 11, 2021 09:17
Copy link
Contributor

@Mai-Saad Mai-Saad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using the page source mentioned here #4397 (comment), can see now that no 500 error in console while delay js is enabled with/without default exclusions (no errors now in debug.log as well). but can see a console error when minify/combine js is enabled (on both trunk and the PR), @Tabrisrp do we need to handle anything for that? or is that expected from the test page?
Uncaught SyntaxError: Unexpected identifier for both wp-embed.min.js and jquery-migrate.min.js

@Tabrisrp
Copy link
Contributor Author

@Mai-Saad This is probably coming from the test page, it doesn't seem to be related to the change in this PR.

@piotrbak
Copy link
Contributor

piotrbak commented Oct 13, 2021

@Tabrisrp It works liek expected. I do have a question though:

  1. I manipulated pcre.backtrack_limit and set it to low limit
  2. Visited the page without RUCSS enabled
  3. Received this error: https://snippi.com/s/5lck17t

Should anything be happening related to RUCSS when the feature is disabled?

@piotrbak
Copy link
Contributor

The PR itself is fine:
testrail-report-275.pdf

@Tabrisrp Tabrisrp merged commit f782c7a into develop Oct 13, 2021
@Tabrisrp Tabrisrp deleted the fix/4397-delay-js-meta-error branch October 13, 2021 16:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module: delay JS type: bug Indicates an unexpected problem or unintended behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Delay JS - PREG_BACKTRACK_LIMIT_ERROR when moving meta to the head
5 participants