wolbroadcast: adopt the non-destructive schema() contract - #24
Merged
Conversation
It was the last plugin here still building its table from install() directly, and install() called uninstall() first -- which is a DROP. So reinstalling the plugin, or repairing it after a failed install, threw away every broadcast address the user had entered. That was not a dormant path. Plugin::installdb() resolves the plugin's own manager, uses schema() when it is there, and falls back to calling install() when it is not. wolbroadcast had no schema(), so the fallback -- and the drop -- is what ran on every install. It now matches the other plugins: createSql() as step 0, an append-only schema(), and an install() that applies pending steps only. On an existing install step 0 is a CREATE TABLE IF NOT EXISTS, so it is a no-op that simply records pSchema = 1; no data moves. This also brings the table under the GH-1245 fix in a way that can be proven rather than argued: createSql() is public, so the probe can call it. It was the one table the live verification of #23 could not reach. Built against a real 1.6 install it now comes out with wbDesc defaulted to '' and wbName and wbBroadcast left bare, both being columns the model declares required -- and the run goes from 23 tables to 24, all accepted. The test grows three checks per table-building manager: it has createSql(), a plugin's OWN manager has schema(), and no install() calls uninstall(). Two things that check got wrong first, both worth recording because both produced confident wrong answers: - taking a fixed 600-character window after "function install(" runs past the closing brace of a two-line install() into the uninstall() that follows, so twenty-one plugins looked like they dropped their own tables. It matches braces now. - requiring schema() of every manager flags eleven files that are correct. installdb() only ever resolves <PluginName>Manager; a secondary manager -- an association table, a sub-table -- is reached as a STEP inside that one's schema(), by design. The check is scoped to the plugin's own manager, and a mutation confirms it stays silent for a secondary one. Four mutations, four behaving correctly. Co-Authored-By: Claude <noreply@anthropic.com>
mastacontrola
pushed a commit
to FOGProject/fogproject
that referenced
this pull request
Aug 22, 2026
…data wolbroadcast was building its table from install(), and install() began with uninstall() -- a DROP. Plugin::installdb() falls back to calling install() for any manager with no schema(), so every install of that plugin silently destroyed the user's rows. Fixed in FOGProject/fog-plugins#24 (v1.6.14). The static half of that contract is already covered in CI by fog-plugins' tests/tables-carry-column-defaults.test.php: createSql() exists, the plugin's own manager has schema(), no install() calls uninstall(). What no test in either repository can cover is the behaviour of the generated DDL against a real server -- that step 0 really is a CREATE TABLE IF NOT EXISTS, and that re-running it over a populated table leaves the rows alone. That needs a live database and a deployed plugin tree, which is exactly why this is in bin/ alongside schema-manifest.php rather than in tests/, where the standing convention is no framework and no database (tests/run-all.sh). Takes the web root and a manager class, so it checks the contract for any plugin rather than being a one-off named after the one that violated it. The real table is never touched: the statement is rewritten onto a zzclaude_ scratch name, the run aborts if that rewrite does not take, and the scratch table is dropped on every exit path. Verified against all three 1.6 servers -- 10.255.20.1, .22.2 and .25.2 -- and against LDAPManager (27 steps) as well as WolbroadcastManager (1). The missing-schema() arm was confirmed to fire by pointing it at a secondary manager, and no zzclaude_ table survived any run. Co-Authored-By: Claude <noreply@anthropic.com>
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
2 participants
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.
Follow-up to #23. wolbroadcast was the one table the live verification there could not reach, because it built its table from
install()rather than a publiccreateSql().It was worse than a coverage gap
install()calleduninstall()first — anduninstall()is a DROP. So reinstalling the plugin, or repairing it after a failed install, threw away every broadcast address the user had entered.That was not a dormant path.
Plugin::installdb()resolves the plugin's own manager, usesschema()when it is there, and falls back to callinginstall()when it is not. wolbroadcast had noschema()— so the fallback, and the drop, is what ran on every install.The change
It now matches every other plugin here:
createSql()as step 0, an append-onlyschema(), and aninstall()that applies pending steps only.On an existing install, step 0 is a
CREATE TABLE IF NOT EXISTS— a no-op that simply recordspSchema = 1. No data moves.And it closes the verification gap
createSql()being public means the probe can call it. Built against a real 1.6 install, the table now comes out withwbDescdefaulted to''andwbName/wbBroadcastleft bare — both columns the model declares required. The run goes from 23 tables to 24, all DDL accepted, 61 columns carrying a default.Test
Three new checks per table-building manager: it has
createSql(), a plugin's own manager hasschema(), and noinstall()callsuninstall(). 109 checks total.Two things that check got wrong first — both recorded in the commit, because both produced confident wrong answers:
function install(runs past the closing brace of a two-lineinstall()into theuninstall()that follows, so twenty-one plugins looked like they dropped their own tables. It matches braces now, and with that fixed wolbroadcast really was the only one.schema()of every manager flags eleven files that are correct.installdb()only ever resolves<PluginName>Manager; a secondary manager — an association table, a sub-table — is reached as a step inside that one'sschema(), by design. The check is scoped to the plugin's own manager, and a mutation confirms it stays silent for a secondary one.Four mutations, four behaving correctly (three firing, one correctly silent). Suite 8/8.
Shipping
Needs a release and a
FOG_PLUGINS_VERSIONbump in fogproject. No core change required — the seam is already onworking-1.6from #1275.🤖 Generated with Claude Code