FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Ensure default modules are loaded regardless of setting registration by felixarntz · Pull Request #248 · WordPress/performance · GitHub

/ performance Public

Ensure default modules are loaded regardless of setting registration - #248

Merged
felixarntz merged 1 commit into
release/1.0.0-beta.3from
fix/default-timing
Mar 22, 2022
ConversationCommits (1)ChecksFiles changed
Merged

Ensure default modules are loaded regardless of setting registration#248
felixarntz merged 1 commit into
release/1.0.0-beta.3from
fix/default-timing

Conversation

felixarntz commented Mar 21, 2022

Copy link
Copy Markdown
Member

Summary

Fixes #247

Relevant technical choices

  • By passing the default as second parameter to get_option() in perflab_get_module_settings(), it is ensured the default value is always used, even when the setting is not registered yet.
  • Since the plugin consistently relies on the perflab_get_module_settings() function to get the setting value, this ensures the default is also taken into consideration early (e.g. when the active modules are being loaded).
  • The default is still also being registered, which is still necessary to support potential other plugins that may need it, e.g. when calling get_option() directly.
  • Tests are included to verify the expected behavior.

Checklist

  • PR has either [Focus] or Infrastructure label.
  • PR has a [Type] label.
  • PR has a milestone or the no milestone label.

felixarntz added [Type] Bug An existing feature is broken Infrastructure Issues for the overall performance plugin infrastructure Needs Review labels Mar 21, 2022
felixarntz added this to the 1.0.0-beta.3 milestone Mar 21, 2022
felixarntz changed the title Ensure default modules setting value is always used regardless of setting registration Ensure default modules are loaded regardless of setting registration Mar 21, 2022
mitogh approved these changes Mar 21, 2022
View reviewed changes

mitogh left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM 🎉

felixarntz changed the base branch from trunk to release/1.0.0-beta.3 March 21, 2022 18:47

felixarntz commented Mar 21, 2022

Copy link
Copy Markdown
Member Author

I've tested this PR on a fresh site and can confirm that all non-experimental modules are now loaded there right away (i.e. without updating settings under Settings > Performance), when before they weren't:

  • WebP images are being generated.
  • The Site Health check for WebP shows up.
  • The Site Health check for a persistent object cache shows up.

felixarntz requested a review from ThierryA March 22, 2022 00:10
felixarntz merged commit 803a031 into release/1.0.0-beta.3 Mar 22, 2022
felixarntz deleted the fix/default-timing branch March 22, 2022 15:14
This was referenced Mar 22, 2022
Closed
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

eugene-manuilov eugene-manuilov approved these changes
jjgrainger jjgrainger approved these changes
ThierryA ThierryA approved these changes
mitogh mitogh approved these changes
kirtangajjar kirtangajjar approved these changes
adamsilverstein Awaiting requested review from adamsilverstein
JustinyAhin Awaiting requested review from JustinyAhin

Assignees

No one assigned

Labels

Infrastructure Issues for the overall performance plugin infrastructure [Type] Bug An existing feature is broken

Projects

None yet

Milestone

1.0.0-beta.3

Development

Successfully merging this pull request may close these issues.

Default value on settings is not set on new installations

6 participants

Back | FazBrowse Home | New Git URL