Skip to content

Conversation

@felixarntz
Copy link
Member

This is a new attempt for #3556, which after commit resulted in a fatal error due to load-styles.php only loading a very small subset of WordPress, thus not having the wp_cache_*() functions available.

This PR addresses the problem by introducing a concrete check for whether to use the cache (the previous code was doing a cache lookup before even checking whether to use the cache (based on ! WP_DEBUG), and by including a new condition ! is_admin() in that check. This way the load-styles.php request succeeds.

To test this fix, set the CONCATENATE_SCRIPTS constant to true, and load WP Admin, which will then load load-styles.php as a CSS file. Without the ! is_admin() condition, you get the fatal error, and therefore styles are not loading. With the condition, it is working as expected.

Trac ticket: https://core.trac.wordpress.org/ticket/56975


This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

Copy link
Member

@mukeshpanchal27 mukeshpanchal27 left a comment

Choose a reason for hiding this comment

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

@felixarntz Left feedback related to comment format

Copy link
Contributor

@hellofromtonya hellofromtonya left a comment

Choose a reason for hiding this comment

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

I like this implementation and the refactoring of the function for it. Much better than wrapping with function_exists(). Great job @felixarntz 🏆

Tested:

  • breaks before patch
  • works after patch 👍

Ready commit! I'll prep and get committed.

@hellofromtonya
Copy link
Contributor

Pausing commit for the option of stubbing in noop.php. Waiting for consensus.

@azaozz
Copy link
Contributor

azaozz commented Jan 20, 2023

Pausing commit for the option of stubbing in noop.php.

Imho stubbing is the better option. Same as existing code, and perhaps a bit more readable/easier to understand.

@felixarntz
Copy link
Member Author

Closing in favor of #3887, which was committed as the solution 🎉

@felixarntz felixarntz closed this Jan 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants