Repository navigation
fix: issues found in a full package audit - #58
Merged
Merged
Conversation
kbond
marked this pull request as ready for review
August 20, 2026 17:34
`Pages::count()` loaded the first page's items to check for emptiness, and `getIterator()` re-counted on every iteration - expensive for collections that count by iterating. `Page::totalCount()` is now memoized.
`ArrayCollection::first()` couldn't distinguish an empty collection from one whose first item is `null`, unlike every other implementation.
`first()` compiled the query to SQL just to check for a delete, then invalidated that compilation with `setMaxResults()` - parsing the query twice on every call.
Passing it as a parameter made Doctrine compare the discriminator column to the FQCN, so the specification never matched anything.
Adds coverage for `Util::stringify()`, `modifyQuery()`, `cacheResult()`, the `cache` specification and `ChainObjectRepositoryFactory::reset()`, and fixes the namespace of two test classes.
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.
Findings from a full audit of the package (grid excluded), one commit each.
Pagescounted more than it needed to.count()loaded the first page's items just to test for emptiness, andgetIterator()re-counted on every iteration - for a collection that counts by iterating, that made paging over it O(n × pages).Page::totalCount()is now memoized too.ArrayCollection::first()returned the default for anullfirst item, unable to distinguish it from an empty collection - unlike every other implementation.first()compiled the query twice. It calledgetSQL()to check for a delete, then invalidated that compilation withsetMaxResults(). It now checks the DQL, which is already built.DoctrineSpec::instanceOf()never matched anything. The class name was bound as a parameter, so Doctrine compared the discriminator column to the FQCN (s0_.type IN ('App\Entity\Circle')). DQL needs it inlined. Single-table-inheritance fixtures were added to cover it.Util::stringify(),EntityResultQueryBuilder::modifyQuery()/cacheResult(), thecachespecification, andChainObjectRepositoryFactory::reset(). Also corrects the namespace of two test classes, which weren't PSR-4 autoloadable.Batch::process()uses a single transaction for the whole loop rather than one per chunk.