fix(query-strategy): compound-key DELETE and no-op UPDATE#11
Merged
alexstandiford merged 2 commits intoApr 21, 2026
Merged
Conversation
The `tests/` harness imported `Phoenix\Core\Tests\TestCase` — a class that does not exist in the package — so no test file could ever load. `phpnomad/tests` wasn't declared as a dev dependency either, which meant phpunit and mockery weren't installed at all. Add `phpnomad/tests` as a dev requirement, update the namespaces on the pre-existing files to match the package's PSR-4 map, and leave the `ValidateCITest` smoke test in place so CI has something to exercise.
Two bugs in `QueryStrategy`, both surfaced when downstream packages started leaning on compound primary keys and upsert flows: DELETE — `delete()` called `$clauseBuilder->where()` inside the per-id loop. `where()` appends clauses without a logical operator between them, which emits invalid SQL (`a = ?s b = ?s`) as soon as there is more than one id column. Switched to `andWhere()` so the conditions are AND'd together; the first `andWhere` call on an empty clause list still omits the leading AND, so single-key deletes emit the same SQL as before. UPDATE — `update()` threw `RecordNotFoundException` whenever MySQL reported zero affected rows. MySQL also reports zero affected rows for a legitimate no-op update: the row exists, the supplied values match what's already stored. That is not a "record not found" condition. The guard made idempotent writes and find-or-create flows impossible, and callers that do need existence checks are better served by performing them before calling `update()` where intent is explicit. Dropped the throw. Adds a `QueryStrategyTest` covering both behaviors, and a per-layer comment on each method explaining why the code looks the way it does so the next contributor doesn't re-introduce the original shape.
alexstandiford
added a commit
that referenced
this pull request
May 26, 2026
CI on main has been failing since #11 because the SSH_PRIVATE_KEY secret used by the workflows can't be loaded ("Error loading key: error in libcrypto"). The SSH dance was originally for pulling private git dependencies via Composer — all phpnomad packages are public on Packagist now, so the dance isn't needed. Replace the SSH key/known-hosts/agent steps with shivammathur/setup-php plus a vanilla `composer install`. Same outcome, no secrets required, runs on forks. Also: - Add `pull_request` triggers so PRs run CI - Bump actions/checkout to v4 - Add `fail-fast: false` so a failure on one PHP version doesn't hide results from the others - Extend the PHPUnit matrix to include 8.3 - Move PHPStan to a sensible PHP runtime (8.2) while keeping `php_version: 7.4` for the analysis target - Bump the spellcheck checkout from `@master` to `@v4`
alexstandiford
added a commit
that referenced
this pull request
May 26, 2026
CI on main has been failing since #11 because the SSH_PRIVATE_KEY secret used by the workflows can't be loaded ("Error loading key: error in libcrypto"). The SSH dance was originally for pulling private git dependencies via Composer — all phpnomad packages are public on Packagist now, so the dance isn't needed. Replace the SSH key/known-hosts/agent steps with shivammathur/setup-php plus a vanilla `composer install`. Same outcome, no secrets required, runs on forks. Also: - Add `pull_request` triggers so PRs run CI - Bump actions/checkout to v4 - Add `fail-fast: false` so a failure on one PHP version doesn't hide results from the others - Extend the PHPUnit matrix to include 8.3 - Move PHPStan to a sensible PHP runtime (8.2) while keeping `php_version: 7.4` for the analysis target - Bump the spellcheck checkout from `@master` to `@v4`
alexstandiford
added a commit
that referenced
this pull request
May 26, 2026
CI on main has been failing since #11 because the SSH_PRIVATE_KEY secret used by the workflows can't be loaded ("Error loading key: error in libcrypto"). The SSH dance was originally for pulling private git dependencies via Composer — all phpnomad packages are public on Packagist now, so the dance isn't needed. Replace the SSH key/known-hosts/agent steps with shivammathur/setup-php plus a vanilla `composer install`. Same outcome, no secrets required, runs on forks. Also: - Add `pull_request` triggers so PRs run CI - Bump actions/checkout to v4 - Add `fail-fast: false` so a failure on one PHP version doesn't hide results from the others - Extend the PHPUnit matrix to include 8.3 - Move PHPStan to a sensible PHP runtime (8.2) while keeping `php_version: 7.4` for the analysis target - Bump the spellcheck checkout from `@master` to `@v4`
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
QueryStrategy::delete()calledClauseBuilder::where()inside the per-id loop, so compound-key deletes emitted invalid SQL (a = ?s b = ?s) with no AND between clauses. Switch toandWhere().QueryStrategy::update()threwRecordNotFoundExceptionwhenever MySQL reported zero affected rows — but MySQL also reports zero for a legitimate no-op (row exists, values unchanged), so idempotent writes blew up. Drop the throw.phpnomad/testsas a dev dep and repair the pre-existingtests/TestCase.php/tests/Unit/ValidateCITest.php, which were pointing at a non-existentPhoenix\Core\Tests\TestCaseclass and couldn't load under the package's PSR-4 map.Downstream packages (PHPNomad/Siren) have been shipping local
QueryStrategyoverrides to work around both bugs. This PR removes the need for those overrides.Test plan
./vendor/bin/phpunitpasses the newQueryStrategyTest— 2 cases covering both fixes, plus the existing smoke test. Verified both tests fail on the currentmainbefore the fix is applied, and pass after.where()loop would produce).update()with values equal to what's already stored no longer throws.