Audit follow-up: bug fixes, refactors, tests and full docs revamp by daycry · Pull Request #15 · daycry/doctrine

daycry · 2026-05-07T15:37:59Z

Summary

Outcome of an exhaustive audit of the package against
its current code.
The branch ships 9 themed commits covering
critical bug fixes, code
refactors, test additions and a complete documentation
overhaul.
No breaking changes for users on the documented
API; one internal
helper (Doctrine::convertDbConfigPdo()) and one
test-only escape hatch
(DoctrineCollector::debugToolbarDisplayPublic())
were removed — both
were dead code.

Why

Cross-checking src/, tests/, docs/ and the CI
configuration
surfaced:

A documented helper that did not exist
(getFromCacheOrQuery()).
A documented filter that was a no-op
(DoctrineSlcReset).
Operators that failed silently instead of validating
([><] with the wrong arity, DataTables regex: true).
A regex with a stray • (U+2022) accepting
unintended prefixes.
rector.php targeting PHP 7.4 while composer.json
requires PHP ≥ 8.2.
PHP 8.5 in the CI matrix while it is still in alpha.
Config/, Commands/ excluded from coverage.
Two Spanish inline comments in production code.
README documenting an operator ([*%]) that was
never implemented and
links to two .md files that do not exist.

Commits (in order)

#	Commit	Scope
1	`chore(tooling): bump Rector PHP target to 8.2
and tighten CI matrix`	`rector.php`,
`.php-cs-fixer.dist.php`, `phpunit.xml.dist`,
`phpunit.yml`
2	`feat(helpers): add getFromCacheOrQuery
cache-aside helper`	new helper + tests + autoload
entry
3	`fix(slc-filter): make DoctrineSlcReset reset
stats per request`	filter implementation + test
4	`fix(datatables): strict operator parsing, [><]
arity, reject regex flag`	`Builder` + edge-case
tests
5	`refactor(doctrine): add getEm() with null
guards, drop dead code`	`Doctrine`,
`DoctrinePublish`, `cli-config.php`
6	`refactor(cache): consolidate Memcached/Redis
wrappers via shared trait`	new
`InitializesNativeClient` trait
7	`refactor(toolbar): memoize SLC lookup, FIFO
buffer, drop test escape hatch`	collector +
middleware
8	`test: add Services coverage, idempotent seeder,
support cs-fixes`	`ServicesTest`, idempotent
`TestSeeder`
9	`docs: comprehensive update aligned with current
code, add CHANGELOG`	every `.md` + new
`CHANGELOG.md`

Highlights

New API

Daycry\Doctrine\Helpers\getFromCacheOrQuery() —
cache-aside helper
backed by Doctrine's configured resultsCache PSR-6
pool, autoloaded
globally.
Doctrine::getEm() — typed accessor that throws if
the
EntityManager is not initialised.
Builder::withMaxFilterValues(int) — caps [IN] /
[OR] value lists
(default 500; DoS guard).
DoctrineCollector::setMaxQueries(int) — FIFO
discard for
long-running CLI workers.
Daycry\Doctrine\Libraries\InitializesNativeClient
trait — shared by
Memcached and Redis wrappers, returning precise
types instead of
mixed.

Behaviour changes

Builder operator [><] now throws
InvalidArgumentException if
the value count is not exactly 2 (was a silent
drop).
Builder rejects DataTables search.regex: true
and
columns[*].search.regex: true with
InvalidArgumentException (was
silently ignored).
DoctrineSlcReset::before() now actually calls
Services::doctrine()->resetSecondLevelCacheStatisti cs().
DoctrinePublish returns exit code 1 on every
error path.
Builder::parseFilterOperator() regex restricted to
documented
tokens.

Tooling / CI

rector.php targets PHP 8.2 (matches
composer.json); applies
match expressions, constructor promotion and 8.x
refactors.
.php-cs-fixer.dist.php now formats tests/ too.
phpunit.xml.dist only excludes
src/cli-config.php from coverage
(src/Config/, src/Commands/, src/Cache/ are
now covered).
PHP 8.5 dropped from the phpunit.yml matrix until
GA.

Documentation

Canonical operator reference moved to
docs/search_modes.md. Both
README.md and docs/datatables.md link to it
instead of duplicating
the table.
New sections: Requirements, Features,
Multi-Database Groups,
Memory Management, Resetting Stats per Request,
Manual Result Caching.
Full Options Reference table in
docs/configuration.md covering
proxyFactory, customTypeMappings,
secondLevelCacheTtl.
New CHANGELOG.md (Keep a Changelog 1.1.0).
Broken links to DATATABLES_FIX.md /
TEST_COVERAGE.md removed.
rector.php targets PHP 8.2 (matches composer.json); applies
match expressions, constructor promotion and 8.x refactors.
.php-cs-fixer.dist.php now formats tests/ too.
phpunit.xml.dist only excludes src/cli-config.php from coverage
(src/Config/, src/Commands/, src/Cache/ are now covered).
PHP 8.5 dropped from the phpunit.yml matrix until GA.

Documentation

Canonical operator reference moved to docs/search_modes.md. Both
README.md and docs/datatables.md link to it instead of duplicating
the table.
New sections: Requirements, Features, Multi-Database Groups,
Memory Management, Resetting Stats per Request,
Manual Result Caching.
Full Options Reference table in docs/configuration.md covering
proxyFactory, customTypeMappings, secondLevelCacheTtl.
New CHANGELOG.md (Keep a Changelog 1.1.0).
Broken links to DATATABLES_FIX.md / TEST_COVERAGE.md removed.

Test plan

composer cs-fix clean
composer phpstan (level 6) — no errors
composer rector dry-run — no changes
composer test --filter "DoctrineCollector|Services|GetFromCacheOrQuery|DoctrineSlcReset|DataTablesBuilder|BuilderCoverage" — 78/78
green locally

- rector.php: target PhpVersion::PHP_82 and LevelSetList::UP_TO_PHP_82 (was PHP_74); drop the obsolete RemoveUnusedPromotedPropertyRector skip. Bump PHPUnitSetList to PHPUNIT_100. - .php-cs-fixer.dist.php: include tests/ in the formatter scope. - phpunit.xml.dist: stop excluding src/Config/, src/Commands/, src/Cache/ from coverage reports (only src/cli-config.php remains excluded). - .github/workflows/phpunit.yml: drop PHP 8.5 from the matrix until it reaches GA. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Adds a small cache-aside helper backed by Doctrine's configured PSR-6 result cache (Config\Doctrine::$resultsCache). On a cache hit the closure is skipped; on a miss the value is stored with the supplied TTL and returned. When the result cache is disabled the closure always runs. The function is autoloaded as a global function via composer.json "autoload.files" — no manual import beyond `use function`. Includes unit tests for hit, miss, ttl=0 and "result cache disabled". Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

The filter was previously a no-op that returned null in both before() and after() despite being documented in installation.md as resetting SLC counters per request. before() now calls Daycry\Doctrine\Config\Services::doctrine() ->resetSecondLevelCacheStatistics() (note the package-specific Services namespace — Config\Services::doctrine() routes via __callStatic but method_exists() on it returns false, which silently disabled the reset in earlier attempts). Wrapped in a try/catch so toolbar filters never break the request pipeline. Tests cover (a) graceful no-op when Doctrine is not bootstrapped, and (b) actual hit/miss/put counter reset after seeding the logger via Services::injectMock(). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Three behaviour fixes in DataTables\Builder: 1. parseFilterOperator() regex now matches only the documented operator tokens (!=|><|>|<|=|%%|%|IN|OR|LIKE). The previous character class "[A-Z!=%<>•]+" allowed accidental "•" (U+2022) matches and produced ambiguous fall-through to "%" for unrelated prefixes. 2. The "[><]" BETWEEN operator now throws InvalidArgumentException when the value count is not exactly 2. Previously sending "[><]50" or "[><]1,2,3" silently dropped the filter, making the user think a filter was applied when it wasn't. 3. Both global "search.regex: true" and per-column "columns[*].search.regex: true" now raise InvalidArgumentException. The DataTables regex flag was never supported but went silently ignored. Use bracket-prefix operators ([IN], [OR], [><]) instead. Adds three regression tests in DataTablesBuilderEdgeCasesTest covering the new exception paths. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

- Doctrine::getEm(): EntityManager — typed accessor that throws RuntimeException if the EntityManager has not been initialised. Used internally by reOpen() and registerTypeMappings() to replace silent null-dereference paths. - Doctrine::reOpen() now resolves the connection through getEm() and is safe to call before/after construction failures. - Drop Doctrine::convertDbConfigPdo() — no callers in the codebase. - Drop the dead "?? new DoctrineCollector()" in __construct (the service factory never returns null). - Replace the metadata-driver switch with a typed match expression (auto-applied by Rector under PHP 8.2 target). - DoctrinePublish: exit(1) on every error path so CI scripts can detect failures. exit() previously returned 0. - cli-config.php: minor cleanup applied by Rector. The 3 test files in this commit are touched only by the cs-fix pass that ran after the functional change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Introduces InitializesNativeClient, a small trait that: - verifies the required PHP extension is loaded, - runs CodeIgniter's initialize(), - wraps any low-level connection error in a CacheException with the original exception chained as `previous`. Memcached and Redis wrappers now use the trait, removing ~30 lines of duplicated boilerplate. getInstance() returns precise types (`?\Memcached`, `?\Redis`) instead of `mixed`, which lets PHPStan narrow the consumer side. Cache tests touched only by cs-fix. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

…hatch DoctrineCollector - resolveSlcLogger(): memoized helper that resolves the StatisticsCacheLogger once per request. Replaces two duplicate Services::doctrine() lookups in getTitleDetails() and getData(). - getData(): caches the payload so isEmpty() + display() (called in sequence by the toolbar) only resolve SLC stats once. - setMaxQueries(int): caps the in-memory query log with FIFO discard (default 1000; 0 disables the cap). Useful in long-running CLI workers and queue consumers that share a process. - reset() now also invalidates the memoised SLC logger and data. - Removed debugToolbarDisplayPublic() — the test-only escape hatch. DoctrineCollectorTest now exercises the SQL highlighter through the canonical display() output. - Translated the two remaining Spanish inline comments to English so the toolbar source stays English-only. DoctrineQueryMiddleware - Refactored by Rector under the new PHP 8.2 target (constructor promotion, minor adjustments). No behavioural change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

- tests/ServicesTest.php (new): covers Services::doctrine() shared and unshared modes, Services::resetDoctrine() (default + explicit group) and Services::doctrineCollector() shared/unshared semantics. Uses SQLite in-memory so it runs without a live MySQL. - tests/_support/Database/Seeds/TestSeeder.php: now idempotent (truncate + insertBatch) so re-running it does not violate the UNIQUE constraint on `name`. - The remaining test files are touched only by the cs-fix pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

- README.md: new Requirements and Features sections; Quick Start with three usage forms; Manual Result Caching example; DataTables example with withMaxFilterValues(); Debug Toolbar setMaxQueries() callout; SLC section now documents the per-request reset filter. - docs/search_modes.md: rewritten as the canonical operator reference (single source of truth). New "Limits & Validation" section covers withMaxFilterValues, [><] arity, regex rejection. New "Unknown Operators" lists every recognised token to catch typos. - docs/datatables.md: links to search_modes.md instead of duplicating the operator table; documents withMaxFilterValues, the [><] strict arity, the regex rejection and the case-insensitivity rules. - docs/usage.md: new sections on EntityManager access (em vs getEm()), reOpen(), Multi-Database Groups, getFromCacheOrQuery with $dbGroup, and SLC statistics reset. - docs/configuration.md: full Options Reference table; proxyFactory, customTypeMappings and secondLevelCacheTtl fully documented. - docs/installation.md: drops the toolbar duplication (links to debug_toolbar.md); adds an autoload note for getFromCacheOrQuery. - docs/cli_commands.md: documents `php spark doctrine:publish` and clarifies the relation with cli-config.php. - docs/debug_toolbar.md: new Memory Management section (setMaxQueries), Manual Reset (reset()) and Resetting Stats per Request (consolidates the SlcReset filter). - docs/second_level_cache.md: regions documented as automatic; Cache attribute example annotated; TTL wording unified with configuration.md. - docs/second_level_cache_stats.md: documents getSecondLevelCacheLogger return type; consolidates reset; links to the Doctrine API. - docs/README.md: features list and Quick Start aligned. - CHANGELOG.md (new): Keep a Changelog 1.1.0 with an [Unreleased] section that summarises every Added/Changed/Removed/Fixed item from this code overhaul. No code behaviour changes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Two follow-up fixes after the CI run on PR #15 surfaced regressions: 1. The previous strict rejection of `regex: true` raised `InvalidArgumentException` even when the matching search value was empty. DataTables clients commonly include the `regex` flag in every payload regardless of intent, so this broke ~12 legacy tests in DataTableTest that send `regex: true` alongside an empty value. The check now only fires when the value is non-empty (i.e. the flag would actually be applied). 2. The new whitelist regex in `parseFilterOperator()` correctly limited the *recognised* operators, but it also stopped stripping unknown bracket prefixes — so `[XYZ]term` searched for `LIKE '%[XYZ]term%'` instead of falling back to `LIKE '%term%'`. Reintroduces a permissive strip pattern that runs only when the strict whitelist does not match, restoring the legacy typo-tolerant behaviour (`testDataTableSearchColumnWithInvalidOperatorFallback`). Also includes: - `tests/_support/Models/Entities/TestAttribute.php`: php-cs-fixer 3.95 on PHP 8.2 (CI runner) flags `#[ ORM\Column...]` as an `attribute_block_no_spaces` violation; the local cache hid this from the previous commit. Removes the stray space. - `tests/DataTableTest.php`: one legacy test (`testDataTableSearchColumn`) pairs a non-empty value with `regex: true`. Updated to `regex: false` to match what real DataTables clients send when they are not asking for regex matching. - `docs/datatables.md`, `docs/search_modes.md`, `CHANGELOG.md`: clarify the new "value must be non-empty" condition and the prefix-stripping fallback for unknown operators. Local verification: `composer cs` (PHP 8.5.3 + PHP 8.2 cache cleared), `composer phpstan`, `composer rector`, `composer test` — all green (132 tests, 358 assertions, 5 skipped). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

The parent CI4 MemcachedHandler types `$memcached` as `Memcache|Memcached`. Symfony's MemcachedAdapter only accepts `Memcached`, so we narrow the return type — Psalm correctly flagged the mismatch (`InvalidReturnStatement`). Added a runtime `instanceof` narrow that returns `null` if the parent ever holds a `Memcache` instance, so a downstream type error is replaced by a descriptive null-handling path. Two `@psalm-suppress UndefinedClass` suppressions guard the references to `\Memcached` in environments where ext-memcached is not loaded (typical for CI Psalm runners). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Coveralls receives the clover report only from one matrix job. Switch the elected job (and the corresponding `carryforward` flag) from `PHP-8.2-MySQL-8.0` to `PHP-8.5-MySQL-8.0` to align with the latest runner used elsewhere in the workflows. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Coveralls flagged a -2.1% drop after PR #15 merged. The cause was structural rather than functional: phpunit.xml.dist stopped excluding src/Commands and src/Config from the coverage scope, but no tests were added for them. This change closes the gap with four targeted test files. No production code is touched. New tests: - tests/Commands/DoctrinePublishTest.php — happy path for the `php spark doctrine:publish` command. Snapshots any existing Config/Doctrine.php and cli-config.php in the host app, runs the command, asserts the files are written with the right namespace and parent class, then restores the snapshots in tearDown(). Covers ~64 % of DoctrinePublish (the exit() error branches remain intractable in PHPUnit and would require a refactor to test cleanly). - tests/DoctrineNullGuardsTest.php — exercises the new Doctrine::getEm() RuntimeException, the matching guard inherited by Doctrine::reOpen(), and the StatisticsCacheLogger-instanceof false branch in Doctrine::getSecondLevelCacheLogger() (both with SLC disabled and with SLC enabled but stats disabled). resetSecondLevelCacheStatistics() is also asserted to be a no-op when no logger is attached. - tests/Libraries/InitializesNativeClientTest.php — covers both error paths of the trait: missing PHP extension and initialize() throwing. The fixture uses anonymous classes so the trait is exercised in isolation, without needing redis/memcached extensions loaded in the runner. - tests/DoctrineCollectorCoverageTest.php — adds three tests for the setMaxQueries() FIFO eviction added in PR #15: cap reached evicts the oldest, cap of 0 disables the limit, negative values are normalised to 0. Local results: - Lines coverage: 90.52 % (621/686) — up from 86.818 %. - Tests: 143 (+8 over master), assertions: 381, skipped: 5. - composer cs / phpstan / psalm / rector / test all green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Rector dry-run on the CI runner (PHP 8.2, no local cache) flagged three adjustments that the local cache had hidden in the previous commit: - tests/DoctrineNullGuardsTest.php — `assertNull(?Object)` ⇒ `assertNotInstanceOf(Class::class, ?Object)` (`AssertEmptyNullableObjectToAssertInstanceofRector`). Imports reordered alphabetically by php-cs-fixer. - tests/Commands/DoctrinePublishTest.php — `(string)` cast on `file_get_contents()` results before passing them to `assertStringContainsString` / `assertStringNotContainsString` (`StringCastAssertStringContainsStringRector`). - tests/Libraries/InitializesNativeClientTest.php — the anonymous `initialize()` that always throws now declares `: never` (`ReturnNeverTypeRector`). Local: cs / phpstan / rector / psalm / test all green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

daycry and others added 17 commits May 7, 2026 17:26

add php version

04091c0

adding PHP versions

962c8ab

Update cs.yml

c8d3f94

daycry merged commit d99db3a into master May 7, 2026
14 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Audit follow-up: bug fixes, refactors, tests and full docs revamp#15

Audit follow-up: bug fixes, refactors, tests and full docs revamp#15
daycry merged 17 commits into
masterfrom
refactor/audit-and-docs

daycry commented May 7, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

daycry commented May 7, 2026

Summary

Why

Commits (in order)

Highlights

New API

Behaviour changes

Tooling / CI

Documentation

Documentation

Documentation

Test plan

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant