Fix correctness issues in the showcase plugin code#39
Merged
Conversation
GaryJones
force-pushed
the
GaryJones/plugin-correctness
branch
from
fda2e06 to
9c01267
Compare
WordPress core only recognises 'Requires at least' for the minimum WordPress version, so the non-standard 'Requires WP' header left the 6.9 floor silently unenforced. Rename it, fill the Plugin URI and GitHub Plugin URI placeholders with the real repository URL, note that the latter is read by Git Updater (formerly GitHub Updater), and point the License URI at the canonical HTTPS GPL-2.0 page.
Building the Config and assigning a Plugin instance to $GLOBALS at file load ran code as a side effect of inclusion, and the __() calls in config/defaults.php executed before init, which triggers the _load_textdomain_just_in_time notice on WordPress 6.7+. The plugin now boots on plugins_loaded behind a plugin_slug() accessor, and translatable labels are closures resolved during the admin_menu and admin_init callbacks. Plugin::run() previously did nothing, so most of the config was dead: it now registers the admin page, setting, section and field from the config, meaning every remaining key is consumed by real code. The unused js/admin-page.js and languages_dir entries go away (no js/ or languages/ directory exists), along with the import of the uninstalled BrightNucleus Settings package, the never-assigned static $instance property, and the docblock advertising a non-existent get_instance() method. The now-unused PLUGIN_SLUG_URL constant is dropped too. Each field passes label_for matching the input id in its view, so the field title labels the control, and the previously hard-coded English strings in the section and field views are now translatable.
Without an uninstall handler, the plugin left its setting behind in the options table after deletion. Guarded on WP_UNINSTALL_PLUGIN so it only runs as part of a genuine uninstall.
Nothing in the codebase references the package, so requiring it only added install weight and sent a misleading signal about how the plugin is architected.
The Rector php84 set arriving in the code-standards branch converts array callables to the native form, so adopt it here and keep the combined rector dry-run clean.
The testing branch gates Infection at 100% MSI, measured before run() gained real behaviour. Exact-argument expectations and invoked render closures kill every mutant the new registration code introduces, and double as a worked example of testing hooked methods with Brain Monkey.
GaryJones
force-pushed
the
GaryJones/plugin-correctness
branch
from
9c01267 to
c2f3e02
Compare
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
As a reference repository, the boilerplate should not teach patterns that are subtly wrong, and a few had crept in.
The plugin headers used
Requires WP, which WordPress core does not recognise, so the WordPress 6.9 floor was never enforced; it is nowRequires at least. The Plugin URI and GitHub Plugin URI placeholders point at the real repository (the latter annotated as a Git Updater header), and the License URI uses the canonical HTTPS GPL-2.0 page.The bootstrap built the Config and assigned a Plugin instance to
$GLOBALSat file include time — exactly the side-effect-on-load anti-pattern the plugin standards forbid — and the__()calls inconfig/defaults.phpran beforeinit, triggering the_load_textdomain_just_in_timenotice on WordPress 6.7+. The plugin now boots onplugins_loadedbehind aplugin_slug()accessor, with translatable labels held as closures that only resolve insideadmin_menuandadmin_initcallbacks.Plugin::run()previously did nothing, leaving most of the config dead and the class carrying an import for an uninstalled package, a never-assigned$instanceproperty, and a docblock for aget_instance()that did not exist. It now genuinely registers the options page, setting, section and field from the config, so every remaining config key is consumed by real code; dead entries (js/admin-page.js,languages_dir) are gone. Each field passes alabel_formatching the input id in its view, so the rendered title accessibly labels the control, and the hard-coded English in the section and field views is now translatable.Rounding out the housekeeping: a new guarded
uninstall.phpremoves the plugin's option on deletion,Foogains its missing@sinceand@returntags, and the never-referencedcedaro/wp-pluginComposer dependency is removed (verified by grep;brightnucleus/configremains in use).Verified with
composer lint, a fullvendor/bin/phpcsrun, the unit test suite, and a stubbed-WordPress smoke test confirming the boot path registers the page, setting, section and field and renders the views correctly.Since
run()now does real work, a newPluginTestcovers the registration end to end — exact arguments to the WordPress registration functions, and every captured render closure invoked against view fixtures — so the 100% mutation score gate arriving in #40 holds once both PRs land.