Skip to content

Super cache - fix 2 null/false warnings#32484

Merged
donnchawp merged 3 commits intotrunkfrom
super-cache/fix-null-warnings
Aug 16, 2023
Merged

Super cache - fix 2 null/false warnings#32484
donnchawp merged 3 commits intotrunkfrom
super-cache/fix-null-warnings

Conversation

@donnchawp
Copy link
Copy Markdown
Contributor

@donnchawp donnchawp commented Aug 15, 2023

As reported by @mcaskill in a comment on #29971 the get_permalink() function can return false which we didn't handle in the preload function. They provided a small patch to check which I added to this PR.

It was observed by @dilirity during testing that there was a NULL warning in trunk:

PHP Deprecated: str_replace(): Passing null to parameter #3 ($subject) of type array|string is deprecated in /usr/local/src/jetpack-monorepo/projects/plugins/super-cache/wp-cache-phase2.php on line 54

I noticed this only happens in wp-admin and so, this PR fixes this by disabling caching in "the backend". wp-cache-phase1.php is not loaded in wp-admin, where $wp_cache_request_uri is defined, which caused the problem. This also has the side effect of reducing the amount of extra logging done in the debug log in areas of a site that aren't cached any way.

Proposed changes:

  • Check that the return value from get_permalink() is a string, and handle it if not.
  • Disable the caching system if the current URL is in "the backend", which is wp-admin, wp-login.php and other areas.

Other information:

  • Have you written new tests for your changes, if applicable?
  • Have you checked the E2E test CI results, and verified that your changes do not break them?
  • Have you tested your changes on WordPress.com, if applicable (if so, you'll see a generated comment below with a script to run)?

Jetpack product discussion

p1692102560916519-slack-C016BBAFHHS

Does this pull request change what data or activity we track or use?

No

Testing instructions:

I'm not sure how to replicate the permalink issue with preloading. It might be caused by deleting a post while preloading, when the preload system has the post_id in the "to check" array in memory.
The issue found by Peter can be replicated by using trunk, going to wp-admin and looking at the debug log.
Apply this patch and the str_replace warning disappears because it has not been run.

@donnchawp
Disable caching if in the backend.
1e42705 
This fixes the problem where $wp_cache_query_uri is not set because it's
set in wp-cache-phase1.php, which is not loaded in wp-admin.
@donnchawp
Make sure this permalink is a string. NULL/false will cause problems
7aa8562
@donnchawp
changelog
b74babe
@donnchawp donnchawp requested a review from dilirity August 15, 2023 13:41
@donnchawp donnchawp self-assigned this Aug 15, 2023
@donnchawp donnchawp marked this pull request as ready for review August 15, 2023 13:41
@github-actions github-actions bot added [Plugin] Super Cache A fast caching plugin for WordPress. [Status] In Progress labels Aug 15, 2023
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Aug 15, 2023

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

  • ✅ Include a description of your PR changes.
  • ✅ All commits were linted before commit.
  • ✅ Add a "[Status]" label (In Progress, Needs Team Review, ...).
  • ✅ Add testing instructions.
  • ✅ Specify whether this PR includes any changes to data or privacy.
  • ✅ Add changelog entries to affected projects

This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖

The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available.

Once your PR is ready for review, check one last time that all required checks (other than "Required review") appearing at the bottom of this PR are passing or skipped.
Then, add the "[Status] Needs Team Review" label and ask someone from your team review the code. Once reviewed, it can then be merged.
If you need an extra review from someone familiar with the codebase, you can update the labels from "[Status] Needs Team Review" to "[Status] Needs Review", and in that case Jetpack Approvers will do a final review of your PR.

Super Cache plugin:

  • Next scheduled release: September 5, 2023.
  • Scheduled code freeze: August 28, 2023.

@donnchawp donnchawp added [Status] Needs Team Review Obsolete. Use Needs Review instead. and removed [Status] In Progress labels Aug 15, 2023
@donnchawp donnchawp requested a review from haqadn August 15, 2023 13:43
haqadn
haqadn approved these changes Aug 16, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@haqadn haqadn left a comment

Choose a reason for hiding this comment

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

👍🏼 Looks good!

@donnchawp donnchawp merged commit 628d780 into trunk Aug 16, 2023
@donnchawp donnchawp deleted the super-cache/fix-null-warnings branch August 16, 2023 06:03
@github-actions github-actions bot removed the [Status] Needs Team Review Obsolete. Use Needs Review instead. label Aug 16, 2023
@github-actions github-actions bot added this to the super-cache/1.9.5 milestone Aug 16, 2023
@donnchawp donnchawp mentioned this pull request Aug 16, 2023
Closed
donnchawp added a commit that referenced this pull request Aug 16, 2023
@donnchawp
[not verified] Super cache - fix 2 null/false warnings (#32484)
ca8acbe 
* Disable caching if in the backend.

This fixes the problem where $wp_cache_query_uri is not set because it's
set in wp-cache-phase1.php, which is not loaded in wp-admin.

* Make sure this permalink is a string. NULL/false will cause problems

* changelog
@donnchawp
Copy link
Copy Markdown
Contributor Author

donnchawp commented Aug 16, 2023

ca8acbe

@youbrokesomething youbrokesomething mentioned this pull request Aug 18, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@haqadn haqadn haqadn approved these changes

@dilirity dilirity Awaiting requested review from dilirity

Assignees

@donnchawp donnchawp

Labels

[Plugin] Super Cache A fast caching plugin for WordPress.

Projects

None yet

Milestone

super-cache/1.9.5

Development

Successfully merging this pull request may close these issues.

2 participants

@donnchawp @haqadn