fix: an account edit that matched no row is not saved - #900
Merged
Conversation
Account::update() and Account::editPassword() called the repository and threw the result away. The other four writes in the same file — restoreModified(), updatePasswordMasterPass(), delete() and deleteByIdBatch() — all read getAffectedNumRows() and refuse when it is zero. So an edit that matched no row came back as saved, and for a password change the user is told their new password is stored when nothing was written. How narrow it is: addHistory() runs first and calls getById(), which throws for an account that is already gone, so the ordinary case was caught. What was not is the window between that read and the write — both inside the same transaction, and the UPDATE takes the latest committed row, so a delete landing in between matches nothing here. It is defence in depth more than a live hole, and it is the check four siblings in the same file already make. The connection sets ATTR_FOUND_ROWS, so affected means matched, not changed: a save that alters no field still returns 1 and is not mistaken for a missing row. The eleven existing tests over these paths now stub a matched row, where they relied on PHPUnit's default QueryResult, which reports zero affected.
blaipr
deleted the
fix/an-account-edit-that-matched-no-row-is-not-saved
branch
September 2, 2026 23:05
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
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.
Account::update()andAccount::editPassword()called the repository and threw the result away.The other four writes in the same file —
restoreModified(),updatePasswordMasterPass(),delete()anddeleteByIdBatch()— all readgetAffectedNumRows()and refuse when it is zero. Therepositories were already counting; only these two ignored it.
So an edit that matched no row came back as saved, and for a password change that is the worse half:
the user is told their new password is stored when nothing was written.
How narrow it is, honestly
addHistory()runs first and callsgetById(), which throws for an account that is already gone —so the ordinary case of editing something deleted a minute ago was already caught. What was not is
the window between that read and the write. Both sit inside the same
transactionAware()closure,and MariaDB's UPDATE takes the latest committed row version, so a delete committed in between
leaves the update matching nothing, and the service reported success for it.
It is defence in depth more than a live hole. It is also the check four siblings in the same file
already make, which is the reason to close it rather than describe it.
Not an unchanged save
The connection sets
Pdo\Mysql::ATTR_FOUND_ROWS, so affected means matched, not changed: a savethat alters no field still returns 1. Without that attribute this check would turn every no-op save
into "Account not found".
Tests
Two, one per method, each with the repository reporting zero matched rows.
update()'s also assertsthat
updateItems()andaddPresetPermissions()never run — nothing downstream of the write shouldhappen for an account that is not there.
Mutation-verified: reverting the service fails both, and only those two.
The eleven existing tests that exercise these paths now stub a matched row, where they previously
relied on PHPUnit's default return value for
QueryResult— which reports zero affected.