Storage Abstraction and DB Drivers#29
Open
widoz wants to merge 29 commits into
Open
Conversation
…lementations to `MetaStorage`. Updated modules and tests to reflect changes.
- Added `Post\TableStorage` and `User\TableStorage` classes implementing the `Storage` interface, using the `{prefix}konomi_interactions` table.
- Implemented schema management via a new `Database\SchemaManager` class, handling table creation (`dbDelta`) and deletion (`DROP TABLE IF EXISTS`) on plugin lifecycle hooks.
- Created a shared `konomi_interactions` table schema to store user-post interactions with indexes to optimize both user-side and post-side queries.
- Rewired `Post\Module` and `User\Module` to use `TableStorage` as the default backend.
- Updated tests to validate `TableStorage` behavior, including transactional writes, schema management, and data querying for both posts and users.
…orage` for cleaner namespace imports.
…bleStorage`, consolidating it in `MetaStorage`, and retiring `StorageKeyParser`.
…ed readability and separation of concerns, preserving existing behavior and quirks in both `Post` and `User` implementations.
…`User\TableStorage`
…ase interactions and payload shaping into separate private helpers, preserving all existing functionality and quirks.
…a new unified `Storage` interface and documentation.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR extracts a shared, axis-parameterized ChangesShared Storage Module Extraction
Estimated code review effort: 4 (Complex) | ~60 minutes Plugin Activation Lifecycle Refactor
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Repository
participant TableStorage
participant wpdb
Repository->>TableStorage: read(id, groupKey)
TableStorage->>wpdb: SELECT rows by axis column
wpdb-->>TableStorage: rows
TableStorage-->>Repository: list of Record
Repository->>TableStorage: write(id, groupKey, records)
TableStorage->>wpdb: DELETE existing rows
TableStorage->>wpdb: INSERT each Record
wpdb-->>TableStorage: success or failure
TableStorage-->>Repository: bool result (rollback on failure)
sequenceDiagram
participant konomi_php
participant ActivationExecute
participant Modules as Activable Modules
participant WordPress
konomi_php->>ActivationExecute: prepare(modules)
ActivationExecute->>Modules: activate(tasks, container)
Modules-->>ActivationExecute: register tasks
konomi_php->>ActivationExecute: registerActivationLogic()
konomi_php->>ActivationExecute: registerDeactivationLogic()
konomi_php->>ActivationExecute: registerUninstallLogic()
ActivationExecute->>WordPress: register hooks
Related Issues: None referenced. Related PRs: None referenced. Suggested labels: enhancement, storage, breaking-change Suggested reviewers: None specified. Poem 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/integration/php/PostRepositoryTest.php (1)
90-111:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReplace arrow functions with
functionclosures in test callbacks.The
array_filtercallbacks on lines 92 and 110 use arrowfn (...) => ...syntax, which conflicts with the repository's test style requirements.♻️ Proposed fix
$stored = $this->postStorage->get(1, 'reaction'); $matching = array_values(array_filter( $stored, - fn (Storage\Record $record) => $record->userId === $this->wpUser->ID + function (Storage\Record $record): bool { + return $record->userId === $this->wpUser->ID; + } ));$matching = array_filter( $this->postStorage->get(1, 'reaction'), - fn (Storage\Record $record) => $record->userId === $this->wpUser->ID + function (Storage\Record $record): bool { + return $record->userId === $this->wpUser->ID; + } );As per coding guidelines: "Use
functioninstead of arrow functions in PHP tests".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integration/php/PostRepositoryTest.php` around lines 90 - 111, The test uses PHP arrow functions in the array_filter callbacks (e.g. the closures filtering Storage\Record instances when building $matching and when filtering $this->postStorage->get(...)), which violates the test style rule; replace each arrow fn (...) => ... with an equivalent named anonymous function closure using function (Storage\Record $record) { return <same boolean expression>; } so the callbacks explicitly type-hint Storage\Record and return the same comparison (record->userId === $this->wpUser->ID), keeping surrounding calls (array_values(array_filter(...)) and array_filter($this->postStorage->get(...), ...)) unchanged.
🧹 Nitpick comments (5)
openspec/changes/archive/2026-05-08-extract-storage-interface/design.md (1)
9-17: 💤 Low valueConsider adding language specifications to fenced code blocks.
The fenced code blocks at lines 9-17 and 23-39 lack language specifications, which would improve syntax highlighting and markdown linting compliance.
📝 Suggested improvement
Add language identifiers to the code blocks:
For the block at line 9:
-``` +```php Post\Storage (concrete)For the block at line 23:
-``` +```php Post\Storage (interface)Also applies to: 23-39
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openspec/changes/archive/2026-05-08-extract-storage-interface/design.md` around lines 9 - 17, The fenced code blocks showing Post\Storage (concrete), User\Storage (concrete) and the later Post\Storage (interface) lack language identifiers; update each triple-backtick fence to include a language tag (e.g., ```php) so the blocks read like ```php ... ``` to enable proper syntax highlighting and markdown linting for those code sections.openspec/changes/extract-shared-storage-module/proposal.md (1)
93-93: ⚡ Quick winAdd language specifiers to fenced code blocks.
The proposal contains several fenced code blocks without language specifiers. Adding appropriate language identifiers (
text,php, ormermaid) improves syntax highlighting and documentation clarity.📝 Suggested language specifiers
Line 93 (file structure):
-``` +```textLine 121 (component diagram):
-``` +```textLine 168 (UML class):
-``` +```textLine 236 (read flow):
-``` +```textLine 270 (write flow):
-``` +```textAlso applies to: 121-121, 168-168, 236-236, 270-270
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openspec/changes/extract-shared-storage-module/proposal.md` at line 93, Add explicit language specifiers to each fenced code block in the proposal to enable proper syntax highlighting: update the fenced blocks labelled "file structure", "component diagram", "UML class", "read flow", and "write flow" to use appropriate identifiers (e.g., ```text for plain text blocks, ```mermaid for diagrams, or ```php where code is PHP) so each triple-backtick fence includes the correct language token immediately after the opening backticks.tests/functional/php/User/RepositoryTest.php (1)
20-26: ⚡ Quick winValidate that a published post exists before running tests.
If no published posts are available,
$this->postIdwill be0, causing all tests to operate with an invalid entity ID. This may lead to false positives or unclear test failures.✅ Proposed validation
$postIds = get_posts([ 'fields' => 'ids', 'numberposts' => 1, 'post_status' => 'publish', ]); $this->postId = (int) ($postIds[0] ?? 0); + + if ($this->postId === 0) { + throw new \RuntimeException('No published posts available for functional tests'); + } });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/functional/php/User/RepositoryTest.php` around lines 20 - 26, Ensure tests validate that a published post was found after calling get_posts before assigning $this->postId: after retrieving $postIds via get_posts(...) check if the array is empty (or $postIds[0] is not set / $this->postId === 0) and if so call $this->markTestSkipped('No published posts available for tests') or throw a clear exception so tests don't run with an invalid ID; update the setup where get_posts is called and $this->postId is set to perform this validation using the existing $this->postId and get_posts symbols.sources/Database/Module.php (1)
63-67: 💤 Low valueConsider the trade-offs of manual dependency reconstruction in uninstall hook.
The
onUninstall()method manually reconstructsSchemaManagerandInteractionsTablerather than using the container. While WordPress'sregister_uninstall_hookconstraints may necessitate this approach, it creates maintenance risk: if constructor signatures change, this code won't benefit from container-managed dependency updates.This is a known trade-off documented in the proposal, but consider adding a comment explaining why container access isn't feasible during uninstall.
📝 Optional documentation improvement
public static function onUninstall(): void { + // Manual reconstruction required: WordPress uninstall hooks execute + // in a limited context where the container may not be available. global $wpdb; SchemaManager::new(InteractionsTable::new($wpdb->prefix))->drop(); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sources/Database/Module.php` around lines 63 - 67, The onUninstall() method reconstructs SchemaManager and InteractionsTable manually instead of resolving them from the DI container; add a concise inline comment above the code in onUninstall() (referencing SchemaManager::new and InteractionsTable::new) explaining that WordPress's register_uninstall_hook execution context prevents access to the app container, so dependencies must be reconstructed here, and optionally reference the design proposal or ticket ID for future maintainers to understand the trade-off and where to update if constructor signatures change.openspec/changes/extract-shared-storage-module/design.md (1)
121-174: ⚡ Quick winConsider adding migration and testing strategy sections.
The design is comprehensive, but would benefit from explicit discussion of:
Rollback/Migration strategy: While line 66 states "No DB migration is required", there's no discussion of the rollback plan if issues are discovered post-deployment.
Testing strategy: The Files Changed table mentions test updates, but the document doesn't explicitly outline the testing approach for validating this large refactor (e.g., integration test coverage requirements, data migration validation).
Backward compatibility: No explicit statement about whether this is a breaking change for any external consumers or plugins.
These additions would make the design document more complete and reduce implementation risks.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openspec/changes/extract-shared-storage-module/design.md` around lines 121 - 174, Add three short sections to design.md: "Rollback/Migration strategy" explaining the plan if issues occur post-deploy even though no DB migration is required (include rollback steps, feature-flag or container-binding revert to legacy MetaStorage, and verification checks), "Testing strategy" listing required test coverage and validation steps (unit tests under tests/unit/php/Storage, integration tests like tests/integration/php/* to validate flattened fixtures, and end-to-end verification for TableStorage(Axis::*) behavior), and "Backward compatibility" stating whether public contracts (Storage, Record, StorageKey, Module wiring) are breaking and how external consumers/plugins can continue functioning or be migrated (e.g., wire old Post/User Storage to shared TableStorage or use container override). Include brief acceptance criteria and rollback triggers for each section.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@openspec/changes/archive/2026-05-08-custom-table-storage/design.md`:
- Line 49: Update the index description to use the correct column name
`entity_id` (as defined earlier) instead of `post_id`: change the UNIQUE
constraint text that currently reads "(post_id, user_id, group_key)" to
"(entity_id, user_id, group_key)" so the index description matches the schema
and avoids confusion.
In `@openspec/changes/extract-shared-storage-module/design.md`:
- Around line 111-117: Update the fenced code block containing the snippet with
$this->loadItems(...), $snapshot = clone $this->registry,
$this->prepareDataToStore(...), $this->storage->write(...) to include a language
specifier (e.g., change the fence from ``` to ```php if this is PHP or to
```text if it's intended as pseudo-code) so markdown renders with proper syntax
highlighting and satisfies the linter.
In `@sources/Database/SchemaManager.php`:
- Around line 28-31: SchemaManager currently accesses the global $wpdb and calls
dbDelta() directly; refactor to inject two dependencies via the constructor: a
WpDbGateway (with methods charsetCollate() and query()) and a DbDeltaRunner
(with method run()). Replace direct global usage around table
name/charsetCollate (where $wpdb->get_charset_collate() is used) with
$this->wpDbGateway->charsetCollate(), switch any raw SQL execution to
$this->wpDbGateway->query(...), and replace the direct dbDelta(...) call with
$this->dbDeltaRunner->run($sqlOrFile). Add private properties for these
interfaces, update the constructor signature to accept and assign them, and
update all calls in SchemaManager to use $this->wpDbGateway and
$this->dbDeltaRunner.
In `@tests/functional/php/Post/RepositoryTest.php`:
- Around line 21-27: The current test uses get_posts() to grab an arbitrary
published post and sets $this->postId from it, which is flaky; instead create
deterministic fixtures with wp_insert_post() during setup: call wp_insert_post()
to create a known published post and assign its return ID to $this->postId (and
create the second post used later rather than computing +999), ensure both posts
are cleaned up in teardown or created within the test isolation, and replace
references to get_posts() and the +999 guess with the explicit IDs returned by
wp_insert_post().
In `@tests/integration/php/UserRepositoryTest.php`:
- Around line 79-100: Replace the arrow fn closures used in the anonymous
callbacks with traditional function closures: change the array_filter callback
that builds $matching (fn (Storage\Record $record) => $record->entityId === 1)
to a function (function (Storage\Record $record) { return $record->entityId ===
1; }), and change both array_map callbacks that build $beforeIds and $afterIds
(each using fn (Storage\Record $record) => $record->entityId) to function
closures (function (Storage\Record $record) { return $record->entityId; }) so
the test file uses function-style closures consistent with the repository test
style.
---
Outside diff comments:
In `@tests/integration/php/PostRepositoryTest.php`:
- Around line 90-111: The test uses PHP arrow functions in the array_filter
callbacks (e.g. the closures filtering Storage\Record instances when building
$matching and when filtering $this->postStorage->get(...)), which violates the
test style rule; replace each arrow fn (...) => ... with an equivalent named
anonymous function closure using function (Storage\Record $record) { return
<same boolean expression>; } so the callbacks explicitly type-hint
Storage\Record and return the same comparison (record->userId ===
$this->wpUser->ID), keeping surrounding calls (array_values(array_filter(...))
and array_filter($this->postStorage->get(...), ...)) unchanged.
---
Nitpick comments:
In `@openspec/changes/archive/2026-05-08-extract-storage-interface/design.md`:
- Around line 9-17: The fenced code blocks showing Post\Storage (concrete),
User\Storage (concrete) and the later Post\Storage (interface) lack language
identifiers; update each triple-backtick fence to include a language tag (e.g.,
```php) so the blocks read like ```php ... ``` to enable proper syntax
highlighting and markdown linting for those code sections.
In `@openspec/changes/extract-shared-storage-module/design.md`:
- Around line 121-174: Add three short sections to design.md:
"Rollback/Migration strategy" explaining the plan if issues occur post-deploy
even though no DB migration is required (include rollback steps, feature-flag or
container-binding revert to legacy MetaStorage, and verification checks),
"Testing strategy" listing required test coverage and validation steps (unit
tests under tests/unit/php/Storage, integration tests like
tests/integration/php/* to validate flattened fixtures, and end-to-end
verification for TableStorage(Axis::*) behavior), and "Backward compatibility"
stating whether public contracts (Storage, Record, StorageKey, Module wiring)
are breaking and how external consumers/plugins can continue functioning or be
migrated (e.g., wire old Post/User Storage to shared TableStorage or use
container override). Include brief acceptance criteria and rollback triggers for
each section.
In `@openspec/changes/extract-shared-storage-module/proposal.md`:
- Line 93: Add explicit language specifiers to each fenced code block in the
proposal to enable proper syntax highlighting: update the fenced blocks labelled
"file structure", "component diagram", "UML class", "read flow", and "write
flow" to use appropriate identifiers (e.g., ```text for plain text blocks,
```mermaid for diagrams, or ```php where code is PHP) so each triple-backtick
fence includes the correct language token immediately after the opening
backticks.
In `@sources/Database/Module.php`:
- Around line 63-67: The onUninstall() method reconstructs SchemaManager and
InteractionsTable manually instead of resolving them from the DI container; add
a concise inline comment above the code in onUninstall() (referencing
SchemaManager::new and InteractionsTable::new) explaining that WordPress's
register_uninstall_hook execution context prevents access to the app container,
so dependencies must be reconstructed here, and optionally reference the design
proposal or ticket ID for future maintainers to understand the trade-off and
where to update if constructor signatures change.
In `@tests/functional/php/User/RepositoryTest.php`:
- Around line 20-26: Ensure tests validate that a published post was found after
calling get_posts before assigning $this->postId: after retrieving $postIds via
get_posts(...) check if the array is empty (or $postIds[0] is not set /
$this->postId === 0) and if so call $this->markTestSkipped('No published posts
available for tests') or throw a clear exception so tests don't run with an
invalid ID; update the setup where get_posts is called and $this->postId is set
to perform this validation using the existing $this->postId and get_posts
symbols.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 512594e2-3c53-4b3a-9f00-aaac0667bb08
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (65)
.editorconfigdocs/storage-drivers.mdkonomi.phpopenspec/changes/archive/2026-05-08-custom-table-storage/.openspec.yamlopenspec/changes/archive/2026-05-08-custom-table-storage/design.mdopenspec/changes/archive/2026-05-08-custom-table-storage/proposal.mdopenspec/changes/archive/2026-05-08-custom-table-storage/specs/table-schema/spec.mdopenspec/changes/archive/2026-05-08-custom-table-storage/specs/table-storage/spec.mdopenspec/changes/archive/2026-05-08-custom-table-storage/tasks.mdopenspec/changes/archive/2026-05-08-extract-storage-interface/design.mdopenspec/changes/archive/2026-05-08-extract-storage-interface/proposal.mdopenspec/changes/archive/2026-05-08-extract-storage-interface/tasks.mdopenspec/changes/archive/2026-05-08-simplify-storage-key/design.mdopenspec/changes/archive/2026-05-08-simplify-storage-key/proposal.mdopenspec/changes/archive/2026-05-08-simplify-storage-key/specs/table-storage/spec.mdopenspec/changes/archive/2026-05-08-simplify-storage-key/tasks.mdopenspec/changes/archive/2026-05-08-split-table-storage-query-build/.openspec.yamlopenspec/changes/archive/2026-05-08-split-table-storage-query-build/design.mdopenspec/changes/archive/2026-05-08-split-table-storage-query-build/proposal.mdopenspec/changes/archive/2026-05-08-split-table-storage-query-build/specs/table-storage/spec.mdopenspec/changes/archive/2026-05-08-split-table-storage-query-build/tasks.mdopenspec/changes/extract-shared-storage-module/design.mdopenspec/changes/extract-shared-storage-module/proposal.mdopenspec/changes/extract-shared-storage-module/specs/table-storage/spec.mdopenspec/changes/extract-shared-storage-module/tasks.mdopenspec/config.yamlopenspec/specs/table-schema/spec.mdopenspec/specs/table-storage/spec.mdsources/Database/InteractionsTable.phpsources/Database/Module.phpsources/Database/SchemaManager.phpsources/Post/Module.phpsources/Post/RawDataAssert.phpsources/Post/Repository.phpsources/Post/Storage.phpsources/Post/StorageKey.phpsources/Storage/Axis.phpsources/Storage/Record.phpsources/Storage/Storage.phpsources/Storage/StorageKey.phpsources/Storage/TableStorage.phpsources/User/Module.phpsources/User/RawDataAssert.phpsources/User/Repository.phpsources/User/Storage.phpsources/User/StorageKey.phptests/WpTestCase.phptests/functional/php/Post/RepositoryTest.phptests/functional/php/User/RepositoryTest.phptests/helpers/InMemoryStorage.phptests/helpers/integration.phptests/integration/php/PostRepositoryTest.phptests/integration/php/UserRepositoryTest.phptests/stubs/php/valid-post-user-reactions.phptests/stubs/php/valid-users-reactions.phptests/unit/php/Post/ItemRegistryTest.phptests/unit/php/Post/RawDataAssertTest.phptests/unit/php/Post/StorageKeyTest.phptests/unit/php/Post/StorageTest.phptests/unit/php/Storage/RecordTest.phptests/unit/php/Storage/StorageKeyTest.phptests/unit/php/Storage/TableStorageTest.phptests/unit/php/User/RawDataAssertTest.phptests/unit/php/User/StorageKeyTest.phptests/unit/php/User/StorageTest.php
💤 Files with no reviewable changes (14)
- tests/unit/php/User/RawDataAssertTest.php
- tests/stubs/php/valid-users-reactions.php
- tests/unit/php/Post/StorageTest.php
- sources/Post/StorageKey.php
- sources/Post/RawDataAssert.php
- tests/unit/php/Post/StorageKeyTest.php
- tests/stubs/php/valid-post-user-reactions.php
- tests/unit/php/User/StorageKeyTest.php
- sources/User/RawDataAssert.php
- tests/unit/php/Post/RawDataAssertTest.php
- tests/unit/php/User/StorageTest.php
- sources/Post/Storage.php
- sources/User/Storage.php
- sources/User/StorageKey.php
|
|
||
| **Naming — `entity_id` over `post_id`**: In the current code `Item::id()` always equals the post ID, but the domain concept is "entity being interacted with". Using `entity_id` keeps the schema neutral if non-post entities are added later. `Post\TableStorage` passes the post ID as `entity_id` since today they are the same value. | ||
|
|
||
| **Index strategy**: The UNIQUE constraint on `(post_id, user_id, group_key)` covers post-side queries. An additional index on `(user_id, group_key)` covers user-side queries efficiently. |
There was a problem hiding this comment.
Fix column name inconsistency in index description.
The schema defines the column as entity_id (line 39), but line 49 refers to it as post_id in the index description. This inconsistency may confuse readers.
✏️ Proposed fix
-**Index strategy**: The UNIQUE constraint on `(post_id, user_id, group_key)` covers post-side queries. An additional index on `(user_id, group_key)` covers user-side queries efficiently.
+**Index strategy**: The UNIQUE constraint on `(entity_id, user_id, group_key)` covers post-side queries. An additional index on `(user_id, group_key)` covers user-side queries efficiently.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| **Index strategy**: The UNIQUE constraint on `(post_id, user_id, group_key)` covers post-side queries. An additional index on `(user_id, group_key)` covers user-side queries efficiently. | |
| **Index strategy**: The UNIQUE constraint on `(entity_id, user_id, group_key)` covers post-side queries. An additional index on `(user_id, group_key)` covers user-side queries efficiently. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@openspec/changes/archive/2026-05-08-custom-table-storage/design.md` at line
49, Update the index description to use the correct column name `entity_id` (as
defined earlier) instead of `post_id`: change the UNIQUE constraint text that
currently reads "(post_id, user_id, group_key)" to "(entity_id, user_id,
group_key)" so the index description matches the schema and avoids confusion.
Comment on lines
+111
to
+117
| ``` | ||
| $this->loadItems(...); | ||
| $snapshot = clone $this->registry; | ||
| $this->prepareDataToStore(...); // mutates registry | ||
| $stored = $this->storage->write(...); | ||
| $stored or $this->registry->replace($snapshot); | ||
| ``` |
There was a problem hiding this comment.
Add language specifier to fenced code block.
The fenced code block should specify a language for proper syntax highlighting and linting compliance.
📝 Proposed fix
-```
+```php
$this->loadItems(...);
$snapshot = clone $this->registry;Or if this is pseudo-code:
-```
+```text
$this->loadItems(...);
$snapshot = clone $this->registry;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ``` | |
| $this->loadItems(...); | |
| $snapshot = clone $this->registry; | |
| $this->prepareDataToStore(...); // mutates registry | |
| $stored = $this->storage->write(...); | |
| $stored or $this->registry->replace($snapshot); | |
| ``` |
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 111-111: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@openspec/changes/extract-shared-storage-module/design.md` around lines 111 -
117, Update the fenced code block containing the snippet with
$this->loadItems(...), $snapshot = clone $this->registry,
$this->prepareDataToStore(...), $this->storage->write(...) to include a language
specifier (e.g., change the fence from ``` to ```php if this is PHP or to
```text if it's intended as pseudo-code) so markdown renders with proper syntax
highlighting and satisfies the linter.
Comment on lines
+28
to
+31
| global $wpdb; | ||
|
|
||
| $tableName = $this->table->name(); | ||
| $charsetCollate = $wpdb->get_charset_collate(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify direct global DB usage in schema lifecycle code.
rg -n -C2 --type=php '\bglobal\s+\$wpdb\b|dbDelta\(|->query\(' sources/Database/SchemaManager.phpRepository: Spaghetti-Dojo/konomi
Length of output: 878
🏁 Script executed:
cat -n sources/Database/SchemaManager.phpRepository: Spaghetti-Dojo/konomi
Length of output: 2063
Replace direct global $wpdb access with injected DB gateway and dbDelta runner.
The class currently bypasses constructor injection and accesses WordPress globals directly at lines 28 and 50, which violates the guideline "Do not use global state or Singletons; dependencies should be explicit." Additionally, line 45 directly calls dbDelta() without declaring that dependency.
Inject two interfaces—one for database operations (WpDbGateway providing charsetCollate() and query()) and one for the schema delta runner (DbDeltaRunner providing run())—into the constructor. This makes dependencies explicit, improves testability, and aligns with PSR-4 and constructor-injection requirements.
♻️ Proposed refactor direction
class SchemaManager
{
- final private function __construct(private readonly InteractionsTable $table)
+ final private function __construct(
+ private readonly InteractionsTable $table,
+ private readonly WpDbGateway $db,
+ private readonly DbDeltaRunner $dbDelta
+ )
{
}
public function create(): void
{
- global $wpdb;
-
$tableName = $this->table->name();
- $charsetCollate = $wpdb->get_charset_collate();
+ $charsetCollate = $this->db->charsetCollate();
$sql = "CREATE TABLE {$tableName} (... ) {$charsetCollate};";
- require_once ABSPATH . 'wp-admin/includes/upgrade.php';
- dbDelta($sql);
+ $this->dbDelta->run($sql);
}
public function drop(): void
{
- global $wpdb;
$tableName = $this->table->name();
- $wpdb->query("DROP TABLE IF EXISTS {$tableName}");
+ $this->db->query("DROP TABLE IF EXISTS {$tableName}");
}
}Also applies to: 50–56
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@sources/Database/SchemaManager.php` around lines 28 - 31, SchemaManager
currently accesses the global $wpdb and calls dbDelta() directly; refactor to
inject two dependencies via the constructor: a WpDbGateway (with methods
charsetCollate() and query()) and a DbDeltaRunner (with method run()). Replace
direct global usage around table name/charsetCollate (where
$wpdb->get_charset_collate() is used) with $this->wpDbGateway->charsetCollate(),
switch any raw SQL execution to $this->wpDbGateway->query(...), and replace the
direct dbDelta(...) call with $this->dbDeltaRunner->run($sqlOrFile). Add private
properties for these interfaces, update the constructor signature to accept and
assign them, and update all calls in SchemaManager to use $this->wpDbGateway and
$this->dbDeltaRunner.
Comment on lines
+21
to
+27
| $postIds = get_posts([ | ||
| 'fields' => 'ids', | ||
| 'numberposts' => 1, | ||
| 'post_status' => 'publish', | ||
| ]); | ||
| $this->postId = (int) ($postIds[0] ?? 0); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify this test currently relies on ambient posts instead of explicit fixture creation.
rg -n -C2 --type=php 'get_posts\(|wp_insert_post\(' tests/functional/php/Post/RepositoryTest.phpRepository: Spaghetti-Dojo/konomi
Length of output: 230
🏁 Script executed:
cat -n tests/functional/php/Post/RepositoryTest.php | head -130 | tail -120Repository: Spaghetti-Dojo/konomi
Length of output: 4437
Use explicit post fixtures to avoid flaky functional tests.
Lines 21–26 depend on whatever published post already exists in the database, which yields 0 or unstable IDs across runs. Line 111 compounds this by guessing another ID with + 999. Create both posts explicitly in the test setup and isolation case using wp_insert_post().
Suggested deterministic fixture setup
- $postIds = get_posts([
- 'fields' => 'ids',
- 'numberposts' => 1,
- 'post_status' => 'publish',
- ]);
- $this->postId = (int) ($postIds[0] ?? 0);
+ $this->postId = (int) wp_insert_post([
+ 'post_title' => 'Konomi Post Fixture A',
+ 'post_type' => 'post',
+ 'post_status' => 'publish',
+ ]);- $otherPostId = $this->postId + 999;
+ $otherPostId = (int) wp_insert_post([
+ 'post_title' => 'Konomi Post Fixture B',
+ 'post_type' => 'post',
+ 'post_status' => 'publish',
+ ]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| $postIds = get_posts([ | |
| 'fields' => 'ids', | |
| 'numberposts' => 1, | |
| 'post_status' => 'publish', | |
| ]); | |
| $this->postId = (int) ($postIds[0] ?? 0); | |
| }); | |
| $this->postId = (int) wp_insert_post([ | |
| 'post_title' => 'Konomi Post Fixture A', | |
| 'post_type' => 'post', | |
| 'post_status' => 'publish', | |
| ]); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/functional/php/Post/RepositoryTest.php` around lines 21 - 27, The
current test uses get_posts() to grab an arbitrary published post and sets
$this->postId from it, which is flaky; instead create deterministic fixtures
with wp_insert_post() during setup: call wp_insert_post() to create a known
published post and assign its return ID to $this->postId (and create the second
post used later rather than computing +999), ensure both posts are cleaned up in
teardown or created within the test isolation, and replace references to
get_posts() and the +999 guess with the explicit IDs returned by
wp_insert_post().
Comment on lines
+79
to
+100
| $matching = array_values(array_filter( | ||
| $stored, | ||
| fn (Storage\Record $record) => $record->entityId === 1 | ||
| )); | ||
|
|
||
| expect($result)->toBeTrue(); | ||
| expect(($this->stubsCounter)()['update_user_meta'])->toBe(1); | ||
| expect($this->userMetaStorage[1]['_konomi_items.bookmark'][1])->toBe([1, 'product']); | ||
| expect($this->storage->writes)->toBe(1); | ||
| expect($matching)->toHaveCount(1)->and($matching[0]->entityType)->toBe('product'); | ||
| expect(did_action('konomi.user.repository.save-successfully'))->toBe(1); | ||
| }); | ||
|
|
||
| it('do not save inactive items', function (): void { | ||
| $user = User\CurrentUser::new($this->repository); | ||
| $inactiveItem = User\Item::new(1, 'product', false); | ||
|
|
||
| expect($this->userMetaStorage[1]['_konomi_items.reaction'])->toEqual([ | ||
| 1 => [1, 'product'], | ||
| 2 => [2, 'page'], | ||
| ]); | ||
| $beforeIds = array_map(fn (Storage\Record $record) => $record->entityId, $this->storage->get(1, 'reaction')); | ||
| expect($beforeIds)->toBe([1, 2]); | ||
|
|
||
| $this->repository->save($user, $inactiveItem); | ||
|
|
||
| expect($this->userMetaStorage[1]['_konomi_items.reaction'])->toEqual([ | ||
| 2 => [2, 'page'], | ||
| ]); | ||
| $afterIds = array_map(fn (Storage\Record $record) => $record->entityId, $this->storage->get(1, 'reaction')); | ||
| expect($afterIds)->toBe([2]); |
There was a problem hiding this comment.
Replace arrow functions with function closures in test callbacks.
Lines 81, 94, and 99 use arrow fn (...) => ... syntax, which conflicts with the repository's test style requirements.
♻️ Proposed fix
- $stored = $this->storage->get(1, 'bookmark');
- $matching = array_values(array_filter(
- $stored,
- fn (Storage\Record $record) => $record->entityId === 1
- ));
+ $stored = $this->storage->get(1, 'bookmark');
+ $matching = array_values(array_filter(
+ $stored,
+ function (Storage\Record $record): bool {
+ return $record->entityId === 1;
+ }
+ ));- $beforeIds = array_map(fn (Storage\Record $record) => $record->entityId, $this->storage->get(1, 'reaction'));
+ $extractEntityId = function (Storage\Record $record): int {
+ return $record->entityId;
+ };
+ $beforeIds = array_map($extractEntityId, $this->storage->get(1, 'reaction'));
expect($beforeIds)->toBe([1, 2]);
$this->repository->save($user, $inactiveItem);
- $afterIds = array_map(fn (Storage\Record $record) => $record->entityId, $this->storage->get(1, 'reaction'));
+ $afterIds = array_map($extractEntityId, $this->storage->get(1, 'reaction'));
expect($afterIds)->toBe([2]);As per coding guidelines: "Use function instead of arrow functions in PHP tests".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/integration/php/UserRepositoryTest.php` around lines 79 - 100, Replace
the arrow fn closures used in the anonymous callbacks with traditional function
closures: change the array_filter callback that builds $matching (fn
(Storage\Record $record) => $record->entityId === 1) to a function (function
(Storage\Record $record) { return $record->entityId === 1; }), and change both
array_map callbacks that build $beforeIds and $afterIds (each using fn
(Storage\Record $record) => $record->entityId) to function closures (function
(Storage\Record $record) { return $record->entityId; }) so the test file uses
function-style closures consistent with the repository test style.
Introduce the `Activable` contract for modules that need lifecycle setup or teardown, and the `ActivationTasks` fluent registry that collects the activation, deactivation, and uninstall callables. `ActivationTasks` is `final readonly` but backs each task list with a mutable `ArrayObject`, so modules receiving the shared registry through `Activable::activate()` accumulate their tasks into the same instance. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Wire the collected `ActivationTasks` to the WordPress activation, deactivation, and uninstall hooks. `prepare()` iterates modules in registration order, calling `Activable::activate()` so tasks accumulate in module order. Activation and deactivation use closures (registered in-request, not persisted). Uninstall is wired through a serializable static callback bridged to the collected registry, because `register_uninstall_hook()` persists its callback to the `uninstall_plugins` option and a closure cannot be serialized. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add the `Activation\Module` ServiceModule registering `ActivationTasks` and `ActivationExecute`. `ActivationExecute` resolves the container's `Package::PROPERTIES` service for its `PluginProperties` dependency. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Implement the `Activable` contract instead of registering the WordPress activation and uninstall hooks directly in the module. The schema create and drop operations are now declared as activation and uninstall tasks, which the `Activation` module wires to WordPress at the correct time (before `plugins_loaded`). This fixes the first-activation bug where `register_activation_hook`, registered during `plugins_loaded`, was too late for WordPress to catch. The unused `PluginProperties` dependency is dropped; the main plugin file is now resolved by `ActivationExecute`. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Restructure konomi.php to run at top level scope: collect the modules, add them to the package, build the container, then resolve `ActivationExecute` to prepare the modules' tasks and register the activation, deactivation, and uninstall logic. The package now boots on `plugins_loaded`. Running the registration before `plugins_loaded` is what lets WordPress catch the activation hook on first activation. Also rename the `ActivationTasks` accessors to the project's property-style convention (activationTasks/deactivationTasks/uninstallTasks), drop `final` from the `ActivationExecute` service to match the codebase's non-final services and allow test doubles, and update PackageTest to assert the new bootstrap contract. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
sources/Database/Module.php (1)
33-36: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAvoid global state — inject
$wpdbexplicitly.
global $wpdb;inside the service factory reaches into global state, which the project guidelines forsources/**/*.phpexplicitly disallow in favor of explicit dependencies.As per coding guidelines, "Do not use global state or Singletons; dependencies should be explicit."
♻️ Proposed fix: register `wpdb` as an explicit service
public function services(): array { return [ + 'wpdb' => static function (): \wpdb { + global $wpdb; + return $wpdb; + }, InteractionsTable::class => static function (): InteractionsTable { - global $wpdb; - return InteractionsTable::new($wpdb->prefix); + return InteractionsTable::new($wpdb->prefix); },(the
InteractionsTableclosure would need aContainerInterface $containerparameter to call$container->get('wpdb')->prefixinstead of relying on the enclosing global.)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sources/Database/Module.php` around lines 33 - 36, The service factory for InteractionsTable in Module should not read from global state. Update the InteractionsTable registration closure to accept the container and resolve the wpdb dependency explicitly from it, then pass the prefix into InteractionsTable::new without using global $wpdb. Also ensure wpdb is registered as a service so the factory can fetch it through the container rather than relying on implicit globals.Source: Coding guidelines
🧹 Nitpick comments (1)
sources/Activation/ActivationExecute.php (1)
25-107: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting an interface for
ActivationExecute.This orchestration class has no interface, unlike
Activable. Per the guideline to "Use interfaces to define contracts between components" and to prefer testing/injecting against interfaces, extracting an interface (e.g.ActivationExecutor) withprepare()/registerActivationLogic()/registerDeactivationLogic()/registerUninstallLogic()would letActivation\Module.phpandkonomi.phpdepend on a contract, and letPackageTest.phpmock that interface instead of the concrete class.As per coding guidelines, "Use interfaces to define contracts between components in PHP" and (for tests) "Test against interfaces rather than implementations when possible in PHP tests."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sources/Activation/ActivationExecute.php` around lines 25 - 107, Extract a contract for the orchestration class: add an interface for ActivationExecute that declares prepare(), registerActivationLogic(), registerDeactivationLogic(), and registerUninstallLogic(), then have ActivationExecute implement it. Update the consumers that currently depend on the concrete class, especially Activation\Module.php and konomi.php, to type-hint the new interface, and adjust PackageTest.php to mock the interface instead of the implementation.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@sources/Database/Module.php`:
- Around line 33-36: The service factory for InteractionsTable in Module should
not read from global state. Update the InteractionsTable registration closure to
accept the container and resolve the wpdb dependency explicitly from it, then
pass the prefix into InteractionsTable::new without using global $wpdb. Also
ensure wpdb is registered as a service so the factory can fetch it through the
container rather than relying on implicit globals.
---
Nitpick comments:
In `@sources/Activation/ActivationExecute.php`:
- Around line 25-107: Extract a contract for the orchestration class: add an
interface for ActivationExecute that declares prepare(),
registerActivationLogic(), registerDeactivationLogic(), and
registerUninstallLogic(), then have ActivationExecute implement it. Update the
consumers that currently depend on the concrete class, especially
Activation\Module.php and konomi.php, to type-hint the new interface, and adjust
PackageTest.php to mock the interface instead of the implementation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2d342353-e23e-4bf4-b8bc-4225fb6564d1
📒 Files selected for processing (8)
docs/superpowers/specs/2026-07-04-activation-tasks-design.mdkonomi.phpsources/Activation/Activable.phpsources/Activation/ActivationExecute.phpsources/Activation/ActivationTasks.phpsources/Activation/Module.phpsources/Database/Module.phptests/unit/php/PackageTest.php
✅ Files skipped from review due to trivial changes (1)
- docs/superpowers/specs/2026-07-04-activation-tasks-design.md
Signed-off-by: Guido Scialfa <dev@guidoscialfa.com>
Convert the Axis from being a dependency of the Storage implementation to be a component of the storage methods, this allow us to put the storage service into the container and third parties to extend it. --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
## Problem
`konomi/konomi` declares `blockHooks: { "core/post-title": "after" }`.
On a
fresh install the block is auto-inserted after the post title on the
single
view but renders **empty** on the front-end (only the editor showed it).
## Root cause
The block's content lives entirely in inner blocks (editor
`useInnerBlocksProps` template + `save` → `InnerBlocks.Content`). Block
Hooks
inserts a **bare** `konomi/konomi` (WordPress does not apply the editor
template to hooked blocks), so `render.php` echoes an empty `$content`.
## Fix
Add a `hooked_block_konomi/konomi` filter
(`Blocks\Konomi\HookedContent`) that
populates the auto-inserted block with the same `core/group → reaction +
bookmark` inner blocks the editor template defines. Guards against
non-array
input and never overrides an instance that already has inner blocks, so
hand-placed blocks and native per-child editing are untouched.
## Verification
- **Live wp-env, front-end single view:** `konomi-reaction` /
`konomi-bookmark`
markup went `0 → 3` — the buttons now render directly under the post
title.
- `phpcs` clean on the changed files; `phpstan` reports no errors.
## Notes
- `tests/functional/php/Blocks/BlockHooksTest.php` covers hook
registration,
inner-block injection, front-end render, and the no-override guard. It
does
not run yet: `composer test:functional` is red on the parent branch due
to a
Modularity double-`build()` in the test bootstrap (separate pre-existing
issue).
🤖 Generated with [Claude Code](https://claude.com/claude-code)
---------
Signed-off-by: Guido Scialfa <dev@guidoscialfa.com>
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
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 by CodeRabbit
New Features
Bug Fixes
Documentation