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

Closes #6661: safeguard exclusions for non valid path #7254

Merged
merged 18 commits into from
Feb 14, 2025

Conversation

Miraeld
Copy link
Contributor

@Miraeld Miraeld commented Jan 28, 2025

Description

Fixes #6661
Avoid potential fatal error if the string url is not valid.

Type of change

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

Detailed scenario

Refer to the issue.

Technical description

Documentation

This pull request includes updates to the get_path_for_exclusion function in the Controller.php file and adds a new test case in the addExclusions.php file. The changes aim to improve the handling of URL paths and ensure proper filtering of exclusions.

Enhancements to URL path handling:

  • inc/Engine/Media/AboveTheFold/Frontend/Controller.php: Modified the get_path_for_exclusion function to check if the path is set and is a string before returning it. If not set, it returns null. Additionally, it filters out null values from the exclusions array before returning it.

Testing improvements:

New dependencies

None

Risks

None

Mandatory Checklist

Code validation

  • I validated all the Acceptance Criteria. If possible, provide screenshots or videos.
  • I triggered all changed lines of code at least once without new errors/warnings/notices.
  • I implemented built-in tests to cover the new/changed code.

Code style

  • I wrote a self-explanatory code about what it does.
  • I protected entry points against unexpected inputs.
  • I did not introduce unnecessary complexity.
  • Output messages (errors, notices, logs) are explicit enough for users to understand the issue and are actionnable.

@Miraeld Miraeld added the type: bug Indicates an unexpected problem or unintended behavior label Jan 28, 2025
@Miraeld Miraeld requested a review from a team January 28, 2025 03:01
@Miraeld Miraeld self-assigned this Jan 28, 2025
Copy link

codacy-production bot commented Jan 28, 2025

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation Diff coverage
Report missing for 11744a11 86.67% (target: 50.00%)
Coverage variation details
Coverable lines Covered lines Coverage
Common ancestor commit (11744a1) Report Missing Report Missing Report Missing
Head commit (8793b1f) 38799 17165 44.24%

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details
Coverable lines Covered lines Diff coverage
Pull request (#7254) 15 13 86.67%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings    Change summary preferences

Codacy stopped sending the deprecated coverage status on June 5th, 2024. Learn more

Footnotes

  1. Codacy didn't receive coverage data for the commit, or there was an error processing the received data. Check your integration for errors and validate that your coverage setup is correct.

@Mai-Saad
Copy link
Contributor

Mai-Saad commented Feb 4, 2025

For testing:

@Mai-Saad
Copy link
Contributor

Mai-Saad commented Feb 6, 2025

  • same error isnot reproducible
  • e2e results (1 test failed, if rerun alone and manual scroll while cache clear , it will pass {need to keep and eye on and see if e2e needs change})
    Screenshot from 2025-02-06 10-35-00
    testrail-report-699.pdf

Manual wrong values in database will cause error (related discussion https://wp-media.slack.com/archives/D01HV2TGKQD/p1738767822946699) => correct link https://wp-media.slack.com/archives/C43T1AYMQ/p1716485105461249
-if lcp is []

[05-Feb-2025 10:24:07 UTC] PHP Fatal error:  Uncaught TypeError: WP_Rocket\Engine\Media\AboveTheFold\Frontend\Controller::generate_lcp_link_tag_with_sources(): Argument #1 ($lcp) must be of type object, array given, called in /var/www/new.rocketlabsqa.ovh/htdocs/wp-content/plugins/wp-rocket/inc/Engine/Media/AboveTheFold/Frontend/Controller.php on line 114 and defined in /var/www/new.rocketlabsqa.ovh/htdocs/wp-content/plugins/wp-rocket/inc/Engine/Media/AboveTheFold/Frontend/Controller.php:244
Stack trace:
#0 /var/www/new.rocketlabsqa.ovh/htdocs/wp-content/plugins/wp-rocket/inc/Engine/Media/AboveTheFold/Frontend/Controller.php(114): WP_Rocket\Engine\Media\AboveTheFold\Frontend\Controller->generate_lcp_link_tag_with_sources()
#1 /var/www/new.rocketlabsqa.ovh/htdocs/wp-content/plugins/wp-rocket/inc/Engine/Media/AboveTheFold/Frontend/Controller.php(93): WP_Rocket\Engine\Media\AboveTheFold\Frontend\Controller->preload_tag()
#2 /var/www/new.rocketlabsqa.ovh/htdocs/wp-content/plugins/wp-rocket/inc/Engine/Media/AboveTheFold/Frontend/Controller.php(66): WP_Rocket\Engine\Media\AboveTheFold\Frontend\Controller->preload_lcp()
#3 /var/www/new.rocketlabsqa.ovh/htdocs/wp-content/plugins/wp-rocket/inc/Engine/Common/PerformanceHints/Frontend/Processor.php(77): WP_Rocket\Engine\Media\AboveTheFold\Frontend\Controller->optimize()
#4 /var/www/new.rocketlabsqa.ovh/htdocs/wp-content/plugins/wp-rocket/inc/Engine/Common/PerformanceHints/Frontend/Subscriber.php(61): WP_Rocket\Engine\Common\PerformanceHints\Frontend\Processor->maybe_apply_optimizations()
#5 /var/www/new.rocketlabsqa.ovh/htdocs/wp-includes/class-wp-hook.php(324): WP_Rocket\Engine\Common\PerformanceHints\Frontend\Subscriber->maybe_apply_optimizations()
#6 /var/www/new.rocketlabsqa.ovh/htdocs/wp-includes/plugin.php(205): WP_Hook->apply_filters()
#7 /var/www/new.rocketlabsqa.ovh/htdocs/wp-content/plugins/wp-rocket/inc/Engine/Optimization/Buffer/Optimization.php(100): apply_filters()
#8 [internal function]: WP_Rocket\Engine\Optimization\Buffer\Optimization->maybe_process_buffer()
#9 /var/www/new.rocketlabsqa.ovh/htdocs/wp-includes/functions.php(5464): ob_end_flush()
#10 /var/www/new.rocketlabsqa.ovh/htdocs/wp-includes/class-wp-hook.php(324): wp_ob_end_flush_all()
#11 /var/www/new.rocketlabsqa.ovh/htdocs/wp-includes/class-wp-hook.php(348): WP_Hook->apply_filters()
#12 /var/www/new.rocketlabsqa.ovh/htdocs/wp-includes/plugin.php(517): WP_Hook->do_action()
#13 /var/www/new.rocketlabsqa.ovh/htdocs/wp-includes/load.php(1279): do_action()
#14 [internal function]: shutdown_action_hook()
#15 {main}
  thrown in /var/www/new.rocketlabsqa.ovh/htdocs/wp-content/plugins/wp-rocket/inc/Engine/Media/AboveTheFold/Frontend/Controller.php on line 244

if atf set to null in db

[05-Feb-2025 10:31:46 UTC] PHP Fatal error:  Uncaught TypeError: WP_Rocket\Engine\Media\AboveTheFold\Frontend\Controller::get_atf_sources(): Argument #1 ($atfs) must be of type array, null given, called in /var/www/new.rocketlabsqa.ovh/htdocs/wp-content/plugins/wp-rocket/inc/Engine/Media/AboveTheFold/Frontend/Controller.php on line 208 and defined in /var/www/new.rocketlabsqa.ovh/htdocs/wp-content/plugins/wp-rocket/inc/Engine/Media/AboveTheFold/Frontend/Controller.php:308
Stack trace:
#0 /var/www/new.rocketlabsqa.ovh/htdocs/wp-content/plugins/wp-rocket/inc/Engine/Media/AboveTheFold/Frontend/Controller.php(208): WP_Rocket\Engine\Media\AboveTheFold\Frontend\Controller->get_atf_sources()
#1 /var/www/new.rocketlabsqa.ovh/htdocs/wp-content/plugins/wp-rocket/inc/Engine/Media/AboveTheFold/Frontend/Subscriber.php(44): WP_Rocket\Engine\Media\AboveTheFold\Frontend\Controller->add_exclusions()
#2 /var/www/new.rocketlabsqa.ovh/htdocs/wp-includes/class-wp-hook.php(324): WP_Rocket\Engine\Media\AboveTheFold\Frontend\Subscriber->add_exclusions()
#3 /var/www/new.rocketlabsqa.ovh/htdocs/wp-includes/plugin.php(205): WP_Hook->apply_filters()
#4 /var/www/new.rocketlabsqa.ovh/htdocs/wp-content/plugins/wp-rocket/inc/Dependencies/RocketLazyload/Image.php(482): apply_filters()
#5 /var/www/new.rocketlabsqa.ovh/htdocs/wp-content/plugins/wp-rocket/inc/Dependencies/RocketLazyload/Image.php(292): WP_Rocket\Dependencies\RocketLazyload\Image->getExcludedSrc()
#6 /var/www/new.rocketlabsqa.ovh/htdocs/wp-content/plugins/wp-rocket/inc/Engine/Media/Lazyload/Subscriber.php(342): WP_Rocket\Dependencies\RocketLazyload\Image->lazyloadPictures()
#7 /var/www/new.rocketlabsqa.ovh/htdocs/wp-includes/class-wp-hook.php(324): WP_Rocket\Engine\Media\Lazyload\Subscriber->lazyload()
#8 /var/www/new.rocketlabsqa.ovh/htdocs/wp-includes/plugin.php(205): WP_Hook->apply_filters()
#9 /var/www/new.rocketlabsqa.ovh/htdocs/wp-content/plugins/wp-rocket/inc/Engine/Optimization/Buffer/Optimization.php(100): apply_filters()
#10 [internal function]: WP_Rocket\Engine\Optimization\Buffer\Optimization->maybe_process_buffer()
#11 /var/www/new.rocketlabsqa.ovh/htdocs/wp-includes/functions.php(5464): ob_end_flush()
#12 /var/www/new.rocketlabsqa.ovh/htdocs/wp-includes/class-wp-hook.php(324): wp_ob_end_flush_all()
#13 /var/www/new.rocketlabsqa.ovh/htdocs/wp-includes/class-wp-hook.php(348): WP_Hook->apply_filters()
#14 /var/www/new.rocketlabsqa.ovh/htdocs/wp-includes/plugin.php(517): WP_Hook->do_action()
#15 /var/www/new.rocketlabsqa.ovh/htdocs/wp-includes/load.php(1279): do_action()
#16 [internal function]: shutdown_action_hook()
#17 {main}
  thrown in /var/www/new.rocketlabsqa.ovh/htdocs/wp-content/plugins/wp-rocket/inc/Engine/Media/AboveTheFold/Frontend/Controller.php on line 308

if lcp manually edited in db to {}
[05-Feb-2025 10:20:14 UTC] PHP Warning: Undefined property: stdClass::$type in /var/www/new.rocketlabsqa.ovh/htdocs/wp-content/plugins/wp-rocket/inc/Engine/Media/AboveTheFold/Frontend/Controller.php on line 256

@Miraeld
Copy link
Contributor Author

Miraeld commented Feb 10, 2025

@Mai-Saad These errors are non regression as they are happening on develop branch, and they aren't link to exclusions.

@Miraeld
Copy link
Contributor Author

Miraeld commented Feb 11, 2025

@Mai-Saad I've added some checks to make sure lcp and viewport are valid before processing them.

@MathieuLamiot MathieuLamiot requested review from Khadreal and a team February 11, 2025 11:18
@Miraeld Miraeld requested a review from remyperona February 12, 2025 14:47
@Mai-Saad
Copy link
Contributor

Mai-Saad commented Feb 13, 2025

  • e2e results same as before, manually running failed test is working
    Screenshot from 2025-02-13 09-24-28
  • the wrong values in DB as {}, [], null is not causing error

If no further code change from CR then we can merge

@Miraeld Miraeld added this pull request to the merge queue Feb 14, 2025
Merged via the queue into develop with commit 0ee63da Feb 14, 2025
13 checks passed
@Miraeld Miraeld deleted the fix/6661-fatal-error-atf-pictures branch February 14, 2025 01:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug Indicates an unexpected problem or unintended behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PHP fatal error potentially related to ATF and picture tags
5 participants