Extract Neo4j node-label read/remove into a shared helper#1098
Merged
JeroenDeDauw merged 1 commit intoJul 20, 2026
Merged
Conversation
Follows-up to #1080 The stub reduction added in #1080 gave Neo4jProjectionStore a getSubjectLabels() and removeNonStubLabels() that near-verbatim duplicated Neo4jSubjectUpdater's getNodeLabels() and the remove branch of updateNodeLabels(): the same read-labels query, the same empty guard, and the same array_diff + Cypher::buildLabelList + MATCH ... REMOVE orchestration. Both operations now live in a small stateless Neo4jNodeLabels helper (read() and remove()), which the Store's removeNonStubLabels() and the Updater's updateNodeLabels() both call. The Updater still reads its labels once and keeps its add branch inline, since that branch is not duplicated anywhere. Why this clears the original "grows beyond a safe move" objection: the two classes hold their transaction differently -- the Updater receives it in its constructor, the Store threads it through its methods from writeTransaction() -- but neither needs to own the transaction to run these queries. Neo4jNodeLabels takes the TransactionInterface as a parameter, so it is agnostic to ownership, and as a static utility (like the sibling Cypher class) it is not a DI collaborator: nothing to inject or wire. No behavior change and no test-expectation edits. Neo4jProjectionStoreTest gains a @Covers for the new helper -- it exercises it through both the stub path and, via savePage, the Updater path; the Updater unit test mocks the transaction and never runs the label queries, so it does not claim coverage. The now-dead getNodeLabels phpstan-baseline entries are dropped. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
JeroenDeDauw
marked this pull request as ready for review
July 20, 2026 17:50
JeroenDeDauw
merged commit Jul 20, 2026
b6bb7e7
into
fix/544-referenced-subject-stubs
6 checks passed
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.
Follows-up to #1080
The stub reduction added in #1080 gave
Neo4jProjectionStoreagetSubjectLabels()andremoveNonStubLabels()that near-verbatim duplicatedNeo4jSubjectUpdater'sgetNodeLabels()and the remove branch ofupdateNodeLabels(): the same read-labels query, the same empty guard, and the samearray_diff+Cypher::buildLabelList+MATCH … REMOVEorchestration.Both operations now live in a small stateless
Neo4jNodeLabelshelper (read()andremove()), which the Store'sremoveNonStubLabels()and the Updater'supdateNodeLabels()both call. The Updater still reads its labels once and keeps its add branch inline, since that branch is not duplicated anywhere.Why this clears the original "grows beyond a safe move" objection: the two classes hold their transaction differently — the Updater receives it in its constructor, the Store threads it through its methods from
writeTransaction()— but neither needs to own the transaction to run these queries.Neo4jNodeLabelstakes theTransactionInterfaceas a parameter, so it is agnostic to ownership, and as a static utility (like the siblingCypherclass) it is not a DI collaborator: nothing to inject or wire.No behavior change and no test-expectation edits.
Neo4jProjectionStoreTestgains a@coversfor the new helper — it exercises it through both the stub path and, viasavePage, the Updater path; the Updater unit test mocks the transaction and never runs the label queries, so it does not claim coverage. The now-deadgetNodeLabelsphpstan-baseline entries are dropped.