[CI smoke test - do not merge] verify the workflow runs - #1
Closed
coraislovely-code wants to merge 9 commits into
Closed
[CI smoke test - do not merge] verify the workflow runs#1coraislovely-code wants to merge 9 commits into
coraislovely-code wants to merge 9 commits into
Conversation
- Replace deprecated ${var} string interpolation with {$var} (deprecated
in PHP 8.2) in ceo-import.php and widgets/comic-calendar.php
- Remove default value from optional-before-required parameter $empty in
ceo_chapters_add_column_value() (deprecated in PHP 8.0)
- Pass explicit $flags to html_entity_decode() in displaycomic.php, since
the default changed in PHP 8.1
- Convert PHP4-style constructor in functions/casthover.php to
__construct() (PHP4-style constructors removed in PHP 8.0)
All files now pass php -l on PHP 8.5 and a PHPCompatibility scan with
testVersion 8.0- with zero errors and zero warnings.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Fix PHP 8.x deprecations and incompatibilities
- comiceasel.php Version header: 1.15 -> 1.17 (was never bumped for 1.16, so installed sites self-reported as 1.15) - readme.txt Stable tag: 1.16 -> 1.17 - Add 1.17 changelog entry for the PHP 8.x compatibility fixes (Frumph#36) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Bump version to 1.17
The plugin has never had a test of any kind. The obstacle has always been that testing a WordPress plugin appears to require WordPress, and therefore a database. It does not, for the part that matters. The plugin's logic -- escaping decisions, SQL construction, payment validation -- depends only on its arguments plus a small set of WordPress helpers. Stubbing those gives a suite that runs in milliseconds with no database, no Docker and no WordPress checkout. Three of the stubs have to be faithful or the tests they support quietly stop meaning anything, and this is documented at the top of tests/stubs.php: - esc_html()/esc_attr() call _wp_specialchars() with $double_encode = false, so they leave existing entities alone, while esc_textarea() double-encodes. Much of this plugin's escaping behaviour turns on that difference, so a naive htmlspecialchars() stub would give the wrong answer. - wp_kses_post() is a recorder returning a sentinel rather than a reimplementation. The question worth asking is "was filtering applied", not "what did it strip". - apply_filters() passes through by default but is overridable, because the plugin's filters are the seams the tests need. Also included is a $wpdb spy that records the SQL it is handed. That is what allows the query construction to be tested without a database at all -- which matters here, because the plugin's queries use MySQL-only date functions that a SQLite test database would misreport. HarnessTest.php asserts these properties of the harness itself. If it fails, nothing else in the suite should be trusted. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Security fixes to output escaping carry a specific risk: the fix silently changes what users see. These tests pin the behaviour that must NOT change, so that any such drift shows up as a failing assertion rather than as a bug report. Chosen deliberately to be stable across the fixes that follow, so they stay green throughout rather than needing to be rewritten by each one. The security assertions -- that a shortcode attribute cannot reach the SQL string, that a javascript: URI cannot be stored as a URL -- ship with the commits that make them true, so each fix arrives with the test that would have caught its absence. The two most valuable assertions here encode properties rather than values: - Rendering a transcript must be stable across saves: a value stored the way the save handler stores it, through esc_textarea(), must render back as exactly those bytes. The case that discriminates is an author who typed a literal entity -- esc_textarea() stores "<b>" as "&lt;b&gt;", and an escape that declines to re-encode existing entities hands back one level short, so the author's literal text decays into a live tag a save at a time. - The same round-trip property for widget link labels, fed through update() twice. ArchiveQueryTest uses the $wpdb spy rather than a database, and asserts that table names come from $wpdb rather than a hardcoded prefix -- a query with the prefix baked in silently returns nothing on any site that did not accept the installer default. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three jobs, each earning its place: - php -l across PHP 7.4 through 8.4. The cheapest useful signal here: the plugin's recent history is PHP 8 compatibility work and it advertises support in readme.txt, but nothing has ever verified that on more than whichever interpreter the maintainer happened to run. - PHPUnit on 8.2-8.4. - PHPCS with the WordPress security and prepared-SQL sniffs. The PHPCS job is scoped to the files a pull request actually changes. This is the difference between the job being useful and being ignored: an unscoped run reports thousands of pre-existing findings across decade-old code, which would make it permanently red and train everyone to skip past it. Scoped, it only speaks up about code someone is touching now. The ruleset is likewise narrow on purpose -- WordPress.Security, PreparedSQL, PreparedSQLPlaceholders and EnqueuedResources -- rather than the full WordPress-Extra. Those are the sniffs matching the bug classes this plugin has actually had. Widening it is a separate decision from turning it on. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The unit suite covers the plugin's logic. It cannot cover the settings forms, the comic meta boxes, the widget forms or the admin list columns, because those are templates with no function boundary to call -- and they are exactly where a broken escaping change shows up as something a user notices. Testing those needs a real WordPress, and getting this plugin into a state where its interesting code paths actually run turns out to be full of traps that make working features look broken: the plugin directory has to be named comic-easel or one widget loses its assets; it has to be activated through the Plugins screen because activation adds a column that several queries depend on; chapter Order defaults to 0 and 0 makes one archive mode render empty; a comic with no featured image renders as nothing but an HTML comment; any value in refer-only hides the comic from everybody; enabling transcripts in posts makes the transcript shortcode return nothing by design. None of that was written down anywhere. It is now, along with how to reach the PayPal IPN endpoint locally -- which otherwise cannot be exercised at all, since it exits unless PayPal returns VERIFIED. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The first version of this job ran PHPCS over every file a pull request touched. Running it for real showed why that does not work here. This plugin predates the WordPress coding standards by about a decade, and its files carry hundreds of pre-existing findings apiece -- 406 on the files one of the follow-up branches touches. Scoping by file means the job fails the moment a pull request edits a legacy file, whether or not that change made anything worse, and a job that is red by default is one everyone learns to scroll past. That is worse than not having it. So report per changed LINE instead: findings on lines a pull request adds are shown, and the historical backlog stays quiet until someone chooses to work on it. The diff is taken against the merge base rather than the base branch tip, so a base branch that has moved on since the pull request opened does not drag unrelated commits in. Even scoped that way it is advisory rather than a gate, which the first real run also made clear. Of the findings on one branch's added lines, most cannot reasonably be fixed: the sniffs cannot see through apply_filters() to an escaper inside it, cannot know that an interpolated ORDER BY direction was whitelisted against ASC/DESC, and object to the $before_title/$after_title arguments that every WordPress widget emits. Gating on those would mean scattering phpcs:ignore annotations through unrelated changes, which makes the code worse to read in exchange for a green tick. Pass --strict to make it blocking if that tradeoff ever looks worthwhile. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Owner
Author
|
Done — this was only to make Actions run the workflow for the first time. It surfaced a real problem (the PHPCS job was scoped by file rather than by changed line, so it would have failed on any PR touching legacy code); fixed in 3d207ed and all ten jobs now pass. Closing. |
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.
Throwaway PR against my own fork, purely to make GitHub Actions execute
.github/workflows/ci.ymlfor the first time and confirm the jobs pass.Not for merging. The real PR is Frumph#38.