v3.0: Refactor exception architecture and simplify API with generator-based returns #7
Conversation
WalkthroughA v3.0 redesign replaces result-wrapper objects with generator-based flows, adds a typed pagination exception hierarchy, extends OffsetAdapter/OffsetResult with generator-based APIs and factories, removes SourceResultInterface and its adapter, updates tests and CI, and adds migration and contributor documentation. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant OffsetAdapter
participant SourceInterface
participant OffsetResult
participant Exception
Client->>OffsetAdapter: generator(offset, limit, nowCount) / execute(...)
OffsetAdapter->>OffsetAdapter: assertArgumentsAreValid(...)
alt invalid args
OffsetAdapter->>Exception: throw InvalidPaginationArgumentException
Exception-->>Client: error
else valid args
loop pages
OffsetAdapter->>SourceInterface: execute(page, pageSize)
SourceInterface-->>OffsetAdapter: Generator<T>
OffsetAdapter->>OffsetAdapter: createLimitedGenerator(from page generator)
alt items
OffsetAdapter->>OffsetResult: yield items
OffsetResult-->>Client: items (via generator or fetchAll)
else empty page
Note over OffsetAdapter: stop on exhaustion or limit reached
end
end
OffsetAdapter-->>Client: OffsetResult (with generator() / getFetchedCount())
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120+ minutes Areas to focus review on:
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
- Add yoda_style rule to PHP-CS-Fixer configuration - Convert all comparison operators to Yoda style (0 === $value) - Applied to src/, tests/ directories - Prevents accidental assignment bugs in conditionals - Maintains consistent coding style throughout project
Complete exception architecture refactoring with enterprise-grade exception system: - NEW: PaginationExceptionInterface extending Throwable for type safety - NEW: PaginationException, InvalidPaginationArgumentException, InvalidPaginationResultException - IMPROVED: Rich contextual error messages with parameter values and actionable guidance - UPDATED: All core classes and interfaces to use new exception hierarchy - TESTED: 102 tests with comprehensive exception scenario coverage - QUALITY: Zero PHPStan errors, full PSR-12 compliance, backward compatible This transforms the pagination package into a production-ready library.
Updated all core classes and tests to use new exception architecture: - OffsetAdapter: Enhanced validation with detailed InvalidPaginationArgumentException - OffsetResult: Result validation with InvalidPaginationResultException - Source adapters: Callback validation with contextual error messages - Interfaces: Added exception documentation to PHPDoc - Test suite: Updated 102 tests with new exception expectations Quality: Zero PHPStan errors, 10,498 assertions, backward compatible
- Enhance SourceInterface with positive-int types for better static analysis - Reorganize OffsetAdapterTest methods for logical test flow - Restructure OffsetResultTest with improved method grouping - Optimize PropertyBasedTest method ordering for clarity - Maintain full backward compatibility and test coverage Quality: 96 tests passing, zero static analysis errors
- Move PHP 8.2 migration changes to version 2.0.0 - Create version 3.0.0 for pagination analysis and architecture improvements - Properly categorize breaking changes, new features, and improvements - Follow semantic versioning principles for major version bumps
- Remove SourceResultInterface references and examples - Update basic usage examples to show direct Generator returns - Add advanced features section (exceptions, static factories, generators) - Include migration guide for v2.x to v3.0 breaking changes - Document new exception hierarchy and error handling - Update custom source implementation examples - Add information about enhanced type safety and validation
🔧 API Improvements: - Add OffsetAdapter::fromCallback() static factory for convenient callback-based setup - Add OffsetAdapter::fetchAll() convenience method for direct array results - Enhance SourceCallbackAdapter documentation and discoverability - Improve inline documentation across all public APIs 📚 Documentation Overhaul: - Completely rewrite README.md with compelling hero section and 30-second quickstart - Extract migration guide to dedicated UPGRADE.md file for better organization - Create comprehensive CONTRIBUTING.md with development setup and guidelines - Structure documentation for different audiences (users, contributors, upgraders) 📦 Packagist Optimization: - Optimize composer.json description for SEO with key search terms - Add 15+ relevant keywords for better discoverability - Enhance package metadata with homepage and support links 🛠️ Quality & CI Enhancements: - Configure CI artifact uploads for test results and coverage reports - Add PHPUnit JUnit logging for better test result visualization - Update .gitignore to exclude build artifacts - Maintain strict code quality standards (PHPStan level 9, PSR-12) 🎯 Developer Experience: - Transform package from functional library to discoverable, easy-to-use solution - Reduce onboarding time from complex setup to 30-second copy-paste - Position as clear alternative to manual pagination implementations - Framework-agnostic design with modern PHP 8.2+ features
Co-authored-by: Igor Pinchuk <somework@users.noreply.github.com>
- Enable Xdebug in composer-install action (coverage: xdebug) - Make coverage artifact upload conditional on success - Ensure coverage reports are properly generated in CI pipeline Resolves PHPUnit warning: 'No code coverage driver available'
somework
changed the title
somework
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (14)
.github/actions/composer-install/action.yml (1)
4-32: Composite Composer action is correct; consider making coverage configurableThe steps (PHP setup, Composer cache, dependency install) are all wired correctly and should work fine across matrix builds. Since
coverage: xdebugis always enabled, you may see some overhead on jobs that don’t need coverage; if that ever becomes an issue, you could promote coverage to an optional input with a default ofnoneand override it only in coverage-producing workflows.tests/ArraySource.php (1)
31-39: Generator-based ArraySource matches the paging contract; ternary can be simplified for clarityThe normalization of
$pageand theyield fromwitharray_slicecorrectly implement “page < 1 → page 1” and “pageSize <= 0 → empty generator” semantics in line with the interface docs. If you’d like to make the control flow a bit clearer for future readers, you could optionally expand the ternary, e.g.:- yield from 0 < $pageSize ? - array_slice($this->data, ($page - 1) * $pageSize, $pageSize) : - []; + if (0 < $pageSize) { + yield from array_slice($this->data, ($page - 1) * $pageSize, $pageSize); + }Functionally identical, but a bit easier to scan in tests.
src/SourceCallbackAdapter.php (1)
18-56: Runtime validation is good; consider simplifying callback invocationThe generator-type validation and dedicated
InvalidPaginationResultException::forInvalidCallbackResult()use look good and will make debugging misbehaving callbacks much easier.If you want to tighten things a bit, you could:
- Normalize the callback to a
\Closurein the constructor (via\Closure::fromCallable) and- Invoke it directly instead of via
call_user_func, which tends to be slower and less friendly to static analysis.For example:
public function __construct(callable $callback) { $this->callback = $callback instanceof \Closure ? $callback : \Closure::fromCallable($callback); } public function execute(int $page, int $pageSize): \Generator { $result = ($this->callback)($page, $pageSize); if (!$result instanceof \Generator) { throw InvalidPaginationResultException::forInvalidCallbackResult( $result, \Generator::class, 'should return Generator', ); } return $result; }Not required for correctness, but would make the adapter slightly more robust and analyzable.
UPGRADE.md (1)
9-75: Upgrade guide is clear; minor wording tweak around removed typesThe upgrade steps and code samples accurately reflect the new generator-based API and
getFetchedCount()rename.One small clarity tweak: in the “Interface Removed” bullet,
SourceResultInterfaceis an interface butSourceResultCallbackAdapteris (was) a class. Renaming that bullet to something like “Types Removed” or splitting interface vs class would avoid confusion for readers skimming the guide.tests/IntegrationTest.php (3)
193-207: Potential infinite loop if source never returns empty.The
while (true)loop relies on the source returning an empty batch or a batch smaller than the limit to break. If the source has a bug and always returns exactly$limititems, this would loop forever. Consider adding a safety guard.$allResults = []; $offset = 0; $limit = 10; + $maxIterations = 100; // Safety guard + $iterations = 0; while (true) { + if (++$iterations > $maxIterations) { + $this->fail('Exceeded maximum iterations - potential infinite loop'); + } $result = $adapter->execute($offset, $limit);
304-312: Clarify the expected behavior ofnowCountparameter.The test shows that
nowCount=2reduces the fetched count from 5 to 3 when requesting 5 items. A brief comment explaining why this is expected behavior would improve test clarity, as the relationship betweennowCountandgetFetchedCount()may not be immediately obvious.// Test with different nowCount values - $result1 = $adapter->execute(0, 5); + // Without nowCount, fetches full limit (5 items) + $result1 = $adapter->execute(0, 5); + // With nowCount=2, only fetches remaining items up to limit (5-2=3 items) $result2 = $adapter->execute(0, 5, 2);
354-357: Empty generator return without explicit yield.The callback uses bare
return;for empty pages. While this works in PHP (a function becomes a generator if it containsyield), it may be clearer to useyield from []for consistency with other empty-generator patterns in this codebase.if ($startIndex >= $totalItems) { - // Return empty generator - return; + // Return empty generator explicitly + yield from []; + return; }tests/OffsetResultTest.php (2)
131-145: Test name "testError" is misleading.The test validates that
execute(0, 0)returns an empty result (not an error). The docblock says "Infinite fetch" but the test actually validates the special zero-limit case. Consider renaming for clarity.- /** - * Infinite fetch. - */ - public function testError(): void + /** + * Zero limit returns empty result when offset and nowCount are also zero. + */ + public function testZeroLimitReturnsEmptyResult(): void {
189-205: Exception type assertion could be more specific.The test expects
\Exception::classwhen iterating a consumed generator. PHP throws a specificExceptionwith message "Cannot traverse an already closed generator". Consider asserting the specific exception type or message for better test precision.// Should throw exception when trying to iterate consumed generator - $this->expectException(\Exception::class); + $this->expectException(\Exception::class); + $this->expectExceptionMessage('Cannot traverse an already closed generator'); iterator_to_array($generator);src/Exception/InvalidPaginationArgumentException.php (1)
105-108: Consider usingarray_key_existsfor nullable parameter values.Using
??operator returnsnullfor both missing keys and keys withnullvalues. If a parameter could legitimately benull, this conflates the two cases.public function getParameter(string $name): mixed { - return $this->parameters[$name] ?? null; + return array_key_exists($name, $this->parameters) + ? $this->parameters[$name] + : null; }Alternatively, document that
nullis returned for both missing and null-valued parameters if that's the intended behavior.CHANGELOG.md (1)
68-69: Missing blank line before "### Changed" in v2.0.0 section.The Added section under v2.0.0 has an extra
~marker on line 58, and the Changed section lacks visual separation compared to the v3.0.0 formatting.Minor formatting inconsistency - consider adding a blank line before
### Changedin the v2.0.0 section for consistency with v3.0.0 formatting.src/OffsetResult.php (1)
85-91: Exposing internal generator has consumption caveats.The
generator()method returns the internal generator, which can only be consumed once. After callingfetchAll(),fetch(), or iterating the returned generator, subsequent calls will return an exhausted generator. This is documented in the class docblock, but a method-level warning could help prevent misuse./** + * Returns the internal generator for advanced use cases. + * + * Warning: The generator can only be consumed once. After calling + * fetch(), fetchAll(), or iterating this generator, it will be exhausted. + * * @return \Generator<T> */ public function generator(): \Generatortests/SourceCallbackAdapterTest.php (1)
130-159: Consider simplifying the conditional logic in the parameterized test.The callback has branching logic based on specific parameter values, which makes the test harder to read and maintain. A simpler approach would be to just yield based on the actual parameters received.
public function testExecuteWithParameters(int $page, int $size, array $expectedResult, bool $assertParameters): void { $source = new SourceCallbackAdapter( function (int $callbackPage, int $callbackSize) use ($page, $size, $assertParameters) { if ($assertParameters) { $this->assertEquals($page, $callbackPage); $this->assertEquals($size, $callbackSize); } - - if (5 === $page && 20 === $size) { - yield "page{$callbackPage}_size$callbackSize"; - } elseif (0 === $page && 0 === $size) { - yield 'zero_params'; - } elseif (1000 === $page && 5000 === $size) { - yield 'large_params'; - } + + yield from $expectedResult; }, );tests/OffsetAdapterTest.php (1)
24-33: Consider verifying the expected fetched data, not just the count.The test verifies
getFetchedCount()returns 1, but it would be more robust to also assert the actual value returned (e.g.,[3]since offset=0 with nowCount=2 would skip 2 items).// Should work with positive nowCount - $this->assertIsArray($result->fetchAll()); - $this->assertSame(1, $result->getFetchedCount()); // Only 1 item should be returned due to nowCount=2 + $items = $result->fetchAll(); + $this->assertSame([3], $items); // With limit=3 and nowCount=2, only 1 more item needed + $this->assertSame(1, $result->getFetchedCount());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (29)
.github/actions/composer-install/action.yml(1 hunks).github/dependabot.yml(1 hunks).github/workflows/ci.yml(1 hunks).gitignore(1 hunks).php-cs-fixer.php(2 hunks)CHANGELOG.md(2 hunks)CONTRIBUTING.md(1 hunks)README.md(2 hunks)UPGRADE.md(1 hunks)composer.json(2 hunks)phpunit.xml.dist(1 hunks)src/Exception/InvalidPaginationArgumentException.php(1 hunks)src/Exception/InvalidPaginationResultException.php(1 hunks)src/Exception/PaginationException.php(1 hunks)src/Exception/PaginationExceptionInterface.php(1 hunks)src/OffsetAdapter.php(2 hunks)src/OffsetResult.php(3 hunks)src/SourceCallbackAdapter.php(1 hunks)src/SourceInterface.php(1 hunks)src/SourceResultCallbackAdapter.php(0 hunks)src/SourceResultInterface.php(0 hunks)tests/ArraySource.php(1 hunks)tests/ArraySourceResult.php(0 hunks)tests/IntegrationTest.php(6 hunks)tests/OffsetAdapterTest.php(1 hunks)tests/OffsetResultTest.php(6 hunks)tests/PropertyBasedTest.php(4 hunks)tests/SourceCallbackAdapterTest.php(4 hunks)tests/SourceResultCallbackAdapterTest.php(0 hunks)
💤 Files with no reviewable changes (4)
- tests/ArraySourceResult.php
- src/SourceResultCallbackAdapter.php
- src/SourceResultInterface.php
- tests/SourceResultCallbackAdapterTest.php
🧰 Additional context used
🧬 Code graph analysis (10)
src/SourceCallbackAdapter.php (2)
src/Exception/InvalidPaginationResultException.php (2)
InvalidPaginationResultException(22-67)forInvalidCallbackResult(33-45)src/SourceInterface.php (1)
execute(50-50)
src/Exception/InvalidPaginationArgumentException.php (4)
src/OffsetAdapter.php (1)
__construct(35-37)src/OffsetResult.php (1)
__construct(37-40)src/SourceCallbackAdapter.php (1)
__construct(35-37)tests/ArraySource.php (1)
__construct(26-28)
tests/IntegrationTest.php (5)
src/SourceCallbackAdapter.php (2)
SourceCallbackAdapter(30-57)execute(44-56)src/OffsetAdapter.php (3)
OffsetAdapter(28-199)execute(65-77)fetchAll(110-113)src/OffsetResult.php (5)
execute(103-111)fetchAll(73-83)fetch(56-66)empty(45-51)getFetchedCount(93-96)src/SourceInterface.php (1)
execute(50-50)tests/ArraySource.php (2)
execute(33-40)ArraySource(21-41)
src/OffsetResult.php (5)
src/Exception/InvalidPaginationResultException.php (1)
InvalidPaginationResultException(22-67)src/OffsetAdapter.php (3)
generator(92-95)__construct(35-37)execute(65-77)src/SourceCallbackAdapter.php (2)
__construct(35-37)execute(44-56)tests/ArraySource.php (2)
__construct(26-28)execute(33-40)src/SourceInterface.php (1)
execute(50-50)
tests/ArraySource.php (3)
src/OffsetResult.php (1)
execute(103-111)src/SourceCallbackAdapter.php (1)
execute(44-56)src/SourceInterface.php (1)
execute(50-50)
tests/PropertyBasedTest.php (5)
src/SourceCallbackAdapter.php (2)
SourceCallbackAdapter(30-57)execute(44-56)src/OffsetAdapter.php (4)
OffsetAdapter(28-199)execute(65-77)generator(92-95)fetchAll(110-113)src/OffsetResult.php (6)
execute(103-111)fetch(56-66)generator(88-91)OffsetResult(29-112)fetchAll(73-83)getFetchedCount(93-96)src/SourceInterface.php (1)
execute(50-50)tests/ArraySource.php (1)
execute(33-40)
tests/OffsetAdapterTest.php (4)
src/OffsetAdapter.php (3)
OffsetAdapter(28-199)execute(65-77)generator(92-95)tests/ArraySource.php (2)
ArraySource(21-41)execute(33-40)src/OffsetResult.php (2)
execute(103-111)generator(88-91)src/SourceInterface.php (1)
execute(50-50)
tests/SourceCallbackAdapterTest.php (3)
src/Exception/InvalidPaginationResultException.php (1)
InvalidPaginationResultException(22-67)src/SourceCallbackAdapter.php (2)
SourceCallbackAdapter(30-57)execute(44-56)src/SourceInterface.php (1)
execute(50-50)
tests/OffsetResultTest.php (4)
src/OffsetResult.php (7)
OffsetResult(29-112)fetchAll(73-83)getFetchedCount(93-96)empty(45-51)fetch(56-66)generator(88-91)execute(103-111)src/OffsetAdapter.php (4)
fetchAll(110-113)generator(92-95)OffsetAdapter(28-199)execute(65-77)src/SourceCallbackAdapter.php (2)
SourceCallbackAdapter(30-57)execute(44-56)src/SourceInterface.php (1)
execute(50-50)
src/OffsetAdapter.php (4)
src/Exception/InvalidPaginationArgumentException.php (4)
InvalidPaginationArgumentException(22-119)__construct(33-41)forInvalidParameter(52-67)forInvalidZeroLimit(78-96)src/OffsetResult.php (6)
__construct(37-40)execute(103-111)OffsetResult(29-112)empty(45-51)generator(88-91)fetchAll(73-83)src/SourceCallbackAdapter.php (3)
__construct(35-37)SourceCallbackAdapter(30-57)execute(44-56)src/SourceInterface.php (1)
execute(50-50)
🪛 actionlint (1.7.9)
.github/workflows/ci.yml
48-48: "strategy" section is alias node but mapping node is expected
(syntax-check)
57-57: property "php-version" is not defined in object type {}
(expression)
🪛 PHPMD (2.15.0)
tests/OffsetAdapterTest.php
162-162: Avoid unused parameters such as '$page'. (undefined)
(UnusedFormalParameter)
281-281: Avoid unused parameters such as '$page'. (undefined)
(UnusedFormalParameter)
281-281: Avoid unused parameters such as '$size'. (undefined)
(UnusedFormalParameter)
🔇 Additional comments (53)
.gitignore (1)
17-18: Well-aligned addition to ignore build artifacts.The
build/entry follows the existing section-based organization and aligns well with the CI/CD enhancements in this PR (test result artifacts, coverage reports)..github/dependabot.yml (1)
3-16: Dependabot config formatting looks goodIndentation and list structure under
updatesare valid YAML and keep the existing behavior for bothgithub-actionsandcomposerecosystems. No changes requested..php-cs-fixer.php (1)
15-16: Php-CS-Fixer config is consistent; Yoda style will be enforced globallyFinder paths and the added
yoda_stylerule are valid and consistent with the rest of the config. Just ensure the team is comfortable with enforcing Yoda comparisons (0 < $var, etc.) across the codebase.Also applies to: 69-69
CONTRIBUTING.md (1)
1-112: Contributing guide is accurate and aligned with tooling/scriptsThe prerequisites, composer scripts, and workflow instructions all match the current
composer.jsonand CI setup. This is a solid DX improvement; just remember to update this doc if PHP version requirements or script names change in future.src/Exception/PaginationExceptionInterface.php (1)
1-28: Exception marker interface is well-structuredDefining
PaginationExceptionInterfaceas a thin extension of\Throwableis a clean way to type-hint all pagination-related errors and keeps future exception classes flexible. No issues from a design or implementation standpoint.composer.json (1)
3-7: composer.json metadata and constraints look consistentThe updated description, keywords, homepage/support links, PHP
^8.2requirement, logic dependency, and dev tooling (PHPUnit, PHPStan, PHP-CS-Fixer) all line up with the rest of the PR and CONTRIBUTING.md. The newtest-coveragescript fits nicely into the documented workflow. No changes requested.Also applies to: 22-33, 44-58
phpunit.xml.dist (1)
16-25: Coverage and logging config align well with CI artifactsThe
<coverage>and<logging>sections write Clover, HTML, text coverage, and JUnit output to the same paths used in the CI workflow, which should make artifact collection straightforward. No issues from a config/structure standpoint.src/Exception/PaginationException.php (1)
16-24: Solid base exception typeUsing a dedicated
PaginationExceptionthat extends\RuntimeExceptionand implementsPaginationExceptionInterfaceis a clean way to provide a common pagination error base without complicating behavior.src/Exception/InvalidPaginationResultException.php (1)
16-66: Exception factories are well‑structured and informativeBoth
forInvalidCallbackResult()andforInvalidSourceResult()produce clear, type-rich messages usingget_debug_type(), and extending\UnexpectedValueExceptionwhile implementingPaginationExceptionInterfacefits the intended hierarchy and existing tests that catchUnexpectedValueException. No changes needed.tests/PropertyBasedTest.php (4)
24-45: Randomized datasets plus explicit edge cases look goodThe
randomDataSetsProvider()combines a few random numeric datasets with explicitempty,single, andmultiplecases, which gives nice coverage forOffsetResultbehavior. Given that the properties under test are pure equivalences, the use of randomness here shouldn’t introduce flakiness.
62-82: OffsetResult property tests match implementation semanticsThe generator-of-generators setup,
fetchAll()roundtrip,getFetchedCount()checks (including idempotence), andfetch()returningnullafter exhaustion are all consistent with theOffsetResultimplementation. The test nicely documents the intended contract.
113-140: Streaming vs batch equivalence test is well‑designedUsing two independent
OffsetResultinstances to comparefetchAll()vs repeatedfetch()(with an explicit!== nullcheck) is a good way to guard against subtle streaming vs batch discrepancies. Assertions against both$batchResultand$streamingResultmatching the original$datafurther tighten the check.
142-161: Type safety test aligns with mixed-data supportAsserting that
fetchAll()returns the mixed dataset unchanged and thatgetFetchedCount()equalscount($mixedData)is a good validation that the pipeline is type-agnostic and that the internalfetchedCountbookkeeping works with arbitrary payloads..github/workflows/ci.yml (1)
9-15: No changes needed—workflow YAML is valid and both concerns are unfoundedThe matrix anchor pattern works correctly and causes no issues:
strategy: &php-matrixwithstrategy: *php-matrixis valid YAML that properly expands to the same strategy block in both jobs- The matrix property is correctly defined and accessible via
${{ matrix.php-version }}- No actionlint configuration exists in the repository, and the YAML is syntactically valid (no
alias node but mapping nodeerrors occur)
actions/checkout@v6is also valid and officially released. It runs onubuntu-latestwithout issues and requires only the standard runner version.Inlining the matrix duplication is unnecessary.
tests/IntegrationTest.php (6)
23-90: Well-structured data provider with comprehensive edge cases.The provider covers important pagination scenarios including empty datasets, single items, exact page sizes, partial pages, offsets beyond data, and mixed data types. Good coverage for boundary conditions.
92-114: API failure simulation test looks correct.The test properly validates that:
- First call succeeds
- Second call throws the expected exception
- Exception is triggered on
fetch()call (lazy evaluation)Note that
expectExceptionmust be called before the throwing code executes.
116-145: Concurrent access simulation is well-designed.The test validates sequential pagination requests with access logging. Minor note: this tests sequential requests (not true concurrency), but the naming is acceptable as it simulates concurrent-like access patterns.
257-294: Memory usage test is valuable but has timing sensitivity.The memory check inside the loop is good for detecting leaks. However,
memory_get_usage()can be affected by garbage collection timing. The 5MB threshold seems reasonable for this test size.
398-422: Streaming processing test validates lazy evaluation correctly.The test demonstrates partial consumption of the generator with early break, confirming that items are processed one at a time without loading the entire dataset.
424-440: Data-driven test with proper DataProvider usage.The test correctly uses PHPUnit 10's attribute-based
#[DataProvider]syntax and validates both results and fetched count.tests/OffsetResultTest.php (5)
22-50: Data provider with inline generator creation is clean.The use of immediately-invoked arrow functions to create generators inline keeps test data self-contained and readable.
67-74: Test correctly validates complex fetch scenarios.The data-driven approach with expected results and counts is well-structured.
87-98: Test validates empty source handling in middle of stream.This is an important edge case ensuring empty generators don't break the iteration flow.
100-107: Good coverage for theempty()static factory.Tests all three access patterns:
fetchAll(),getFetchedCount(), andfetch().
377-380: Helper method is clean and reusable.The
getGenerator()helper properly usesyield fromto create generators from arrays, enabling consistent test data creation.src/Exception/InvalidPaginationArgumentException.php (3)
22-41: Well-designed exception with rich context.The constructor properly chains to parent and stores parameters for debugging. The optional
$codeand$previousparameters follow exception best practices.
52-67: Factory method produces clear, actionable error messages.The
forInvalidParametermethod generates descriptive messages with the parameter name, invalid value, and guidance for correction. Theis_scalarcheck handles non-scalar types gracefully.
78-96: Zero-limit validation message is comprehensive.The message explains the constraint (zero limit only at start), the current values, and provides guidance for alternative approaches. This improves developer experience significantly.
CHANGELOG.md (1)
8-54: Changelog is comprehensive and well-organized.The v3.0.0 entry clearly documents:
- Breaking changes with BREAKING prefix
- New additions (exception hierarchy, factory methods)
- Removed components
- Bug fixes and developer experience improvements
This follows Keep a Changelog conventions and provides excellent migration guidance.
src/OffsetResult.php (2)
42-51: Well-implemented empty result factory.The
empty()method correctly creates an OffsetResult with an empty generator usingyield from []. The PHPDoc return annotationnever-returnaccurately describes that the generator yields nothing.
103-111: Generator flattening logic is correct and efficient.The
execute()method properly iterates through page generators, yields each item, and increments the fetched count. This lazy evaluation ensures memory efficiency for large datasets.tests/SourceCallbackAdapterTest.php (6)
20-36: Well-structured data providers for comprehensive test coverage.The data providers cover important edge cases: null returns, array returns, and object returns that aren't generators. The
parameterTestProvidertests various parameter combinations including edge cases (zero parameters, large parameters).
48-61: Good coverage of empty generator behavior.The test correctly validates that an empty generator (
yield from []) produces an empty result array. This aligns with the new generator-based API.
63-76: Solid parameterized test for invalid callback returns.The test properly uses the data provider and validates both the exception type and the expected message format matching the
InvalidPaginationResultException::forInvalidCallbackResultimplementation.
78-88: Good exception propagation test.This correctly verifies that exceptions thrown inside callbacks are propagated without being caught or wrapped by the adapter.
90-110: Thorough test for complex callback logic.The test validates:
- Callback receives correct parameters (page=2, size=3)
- Generator yields expected items
- Call count tracking confirms single invocation
161-174: LGTM!The test correctly validates the basic generator flow with
yield fromsyntax and iteration.src/OffsetAdapter.php (8)
28-37: Readonly class with constructor property promotion is correct.The
readonlyclass modifier combined withprotected SourceInterface $sourceis valid. The property is immutable after construction.
39-52: Clean factory method for callback-based instantiation.The
fromCallbackfactory provides a convenient way to create an adapter without manually wrapping the callback inSourceCallbackAdapter.
65-77: Good early-return optimization for zero-parameter sentinel case.The check for
offset=0, limit=0, nowCount=0correctly returns an empty result without invoking the pagination logic, which is efficient.
92-113: Useful convenience methods that delegate correctly.Both
generator()andfetchAll()properly delegate toexecute()and then call the appropriate method onOffsetResult. The docblocks are clear about the intended usage.
120-151: Pagination loop logic is well-structured.The logic correctly:
- Tracks
totalDeliveredandcurrentNowCountfor pagination state- Uses
shouldContinuePaginationto control the loop- Validates page/pageSize before fetching
- Handles empty source gracefully with
valid()check- Catches
AlreadyGetNeededCountExceptionfor early termination
153-170: Comprehensive argument validation with descriptive error messages.The validation loop with
matchexpression provides clear, contextual error messages for each parameter. The zero-limit edge case is correctly handled.
172-190: Limited generator correctly enforces bounds and updates counters.The
createLimitedGeneratormethod properly:
- Checks limit before yielding each item
- Updates both
totalDeliveredandcurrentNowCountvia references- Breaks early when limit is reached
Note: Using references (
&$totalDelivered,&$currentNowCount) is correct here since generator execution is lazy and state must persist across iterations.
192-198: Simple and clear continuation predicate.The
shouldContinuePaginationmethod correctly handles the "unlimited" case (limit=0) and the bounded case.tests/OffsetAdapterTest.php (8)
59-72: Good exception introspection test.The test properly validates that the exception:
- Contains the parameter values via
getParameters()- Supports individual parameter access via
getParameter()- Returns null for non-existent parameters
- Includes descriptive message with the actual value
74-95: Solid interface compliance verification.Testing that exceptions implement
PaginationExceptionInterfaceensures consumers can catch all pagination-related exceptions with a single catch clause.
97-111: Good test for generator/execute equivalence.This validates that
generator()produces the same data asexecute()->fetchAll(), ensuring API consistency.
159-173: Unused$pageparameter is intentional for this test.The static analysis warning about unused
$pageis a false positive here. The callback intentionally ignores pagination parameters to return a fixed sequence, testing that the adapter correctly limits results.
279-291: Unused parameters are intentional for testing empty source behavior.The static analysis warnings about unused
$pageand$sizeare false positives. The callback intentionally returns an empty generator regardless of parameters to test adapter behavior with empty sources.
208-236: Comprehensive validation of zero-limit edge cases.These tests thoroughly cover the zero-limit scenarios with various combinations of offset and nowCount, validating both the exception type and the full error message.
238-277: Complete coverage of negative argument validation.All three parameters (offset, limit, nowCount) are tested for negative value rejection with proper exception messages. This ensures the validation logic in
assertArgumentsAreValidis working correctly.
293-316: Good edge case tests for exception parameters and zero-limit sentinel.The tests validate:
- Exception provides all parameter values for zero-limit errors
- Zero-limit sentinel (offset=0, limit=0, nowCount=0) returns empty result correctly
somework
changed the title
- README: Fix streaming example to use strict null check instead of truthy check - OffsetResult: Remove unused InvalidPaginationResultException import - OffsetResult: Remove incorrect @throws annotation from fetchAll() method - SourceInterface: Update @param types from positive-int to int to match actual contract - PropertyBasedTest: Fix callback signatures to accept required (int $page, int $pageSize) parameters - Style: Auto-fix code formatting with CS fixer All tests pass (96/96) and quality checks pass.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tests/PropertyBasedTest.php (2)
47-60: Callback tests correctly match adapter contract; consider silencing “unused parameter” warningsThe callbacks in
testExceptionPropagation()andtestSourceCallbackAdapterRobustness()now correctly accept(int $page, int $pageSize), so they exerciseSourceCallbackAdapter::execute()instead of failing withArgumentCountError. That fixes the earlier behavioral issue.If PHPMD’s
UnusedFormalParameterwarnings are bothersome in tests, you can prefix the parameters with underscores to make the intent explicit:- $source = new SourceCallbackAdapter(function (int $page, int $pageSize) { + $source = new SourceCallbackAdapter(function (int $_page, int $_pageSize) { throw new \DomainException('Domain error'); });- $source = new SourceCallbackAdapter( - fn (int $page, int $pageSize) => $invalidReturn, - ); + $source = new SourceCallbackAdapter( + fn (int $_page, int $_pageSize) => $invalidReturn, + );Purely optional, but it keeps static analysis quiet while preserving clarity.
Also applies to: 84-112
24-45: Property-based tests give solid coverage of OffsetResult semanticsThe new provider-driven tests (
randomDataSetsProvider,testOffsetResultProperties,testStreamingVsBatchEquivalence, and the extragetFetchedCountassertion intestTypeSafety) nicely exercise the newOffsetResultAPI: generator-of-generators shape,fetchAll()vs streamingfetch(), andgetFetchedCount()consistency across different data sets (including empty/single/multiple).If you ever want more reproducibility for failing runs, you could consider seeding or logging the generated datasets, but as-is this is a good, practical balance for property-style testing.
Also applies to: 62-82, 115-142, 144-163
README.md (1)
8-13: Minor markdownlint MD036: emphasis used as headingIf you’re running
markdownlint, the bold “Transform page-based APIs…” line will trigger MD036 (“emphasis used instead of a heading”). Easiest fix is to turn it into an actual heading:-**Transform page-based APIs into offset-based pagination with zero hassle** +## Transform page-based APIs into offset-based pagination with zero hassleOr, if you like the current look, you can instead relax/disable MD036 in your markdownlint config.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
README.md(2 hunks)src/OffsetResult.php(2 hunks)src/SourceInterface.php(1 hunks)tests/PropertyBasedTest.php(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/SourceInterface.php (4)
src/OffsetResult.php (1)
execute(99-107)src/SourceCallbackAdapter.php (1)
execute(44-56)tests/ArraySource.php (1)
execute(33-40)src/OffsetAdapter.php (1)
execute(65-77)
src/OffsetResult.php (2)
src/OffsetAdapter.php (3)
generator(92-95)__construct(35-37)execute(65-77)tests/ArraySource.php (2)
__construct(26-28)execute(33-40)
tests/PropertyBasedTest.php (5)
src/SourceCallbackAdapter.php (2)
SourceCallbackAdapter(30-57)execute(44-56)src/OffsetAdapter.php (4)
OffsetAdapter(28-199)execute(65-77)generator(92-95)fetchAll(110-113)src/OffsetResult.php (6)
execute(99-107)fetch(54-64)generator(84-87)OffsetResult(27-108)fetchAll(69-79)getFetchedCount(89-92)tests/OffsetResultTest.php (2)
DataProvider(67-74)DataProvider(182-187)tests/IntegrationTest.php (1)
DataProvider(424-440)
🪛 markdownlint-cli2 (0.18.1)
README.md
8-8: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🪛 PHPMD (2.15.0)
tests/PropertyBasedTest.php
50-50: Avoid unused parameters such as '$page'. (undefined)
(UnusedFormalParameter)
50-50: Avoid unused parameters such as '$pageSize'. (undefined)
(UnusedFormalParameter)
100-100: Avoid unused parameters such as '$page'. (undefined)
(UnusedFormalParameter)
100-100: Avoid unused parameters such as '$pageSize'. (undefined)
(UnusedFormalParameter)
🔇 Additional comments (2)
src/SourceInterface.php (1)
27-50: SourceInterface contract now matches real usage and testsThe execute() signature and docblock (plain
intparams with documented handling for< 1andpageSize = 0, generator return) are now consistent with ArraySource, SourceCallbackAdapter, and OffsetAdapter’s expectations. Looks good and aligns with the v3.0 generator-based design.src/OffsetResult.php (1)
29-49: OffsetResult API looks consistent with adapter logic and testsThe new
OffsetResultsurface (fetch(),fetchAll(),generator(),getFetchedCount(), andempty()) is internally coherent:
execute()flattens\Generator<\Generator<T>>and increments$fetchedCountonce per yielded item.- Both
fetchAll()and streamingfetch()just advance the same generator, so counts stay accurate regardless of access pattern.empty()cleanly models the zero-result case and lines up with the special branch inOffsetAdapter::execute().This matches how the tests use it (batch vs streaming equivalence and fetched-count checks), so the implementation looks solid.
Also applies to: 54-79, 81-107
Code Quality & Clarity: - Simplify ArraySource ternary to if-statement for better readability - Add safety guard to prevent infinite loops in IntegrationTest - Clarify nowCount parameter behavior with detailed comments - Use yield from [] consistently for empty generators Test Improvements: - Rename misleading testError() to testZeroLimitReturnsEmptyResult() - Add specific exception message assertions for better test precision - Verify expected fetched data values instead of just counts Documentation & Safety: - Update UPGRADE.md to clarify removed types (interface vs class) - Add consumption warning to OffsetResult::generator() method - Use array_key_exists for more precise parameter handling All tests pass (96/96, 488 assertions) and quality checks clean.
- Add optional coverage input to composer-install action (default: none) - Enable xdebug only for tests job, disable for quality job - Reduces CI overhead by avoiding unnecessary coverage instrumentation - Maintains coverage generation where needed while optimizing performance Addresses CodeRabbit suggestion for configurable coverage.
- README: Convert bold text to proper heading to avoid markdownlint MD036 - Tests: Parameter names already prefixed with underscores (previously fixed) - Quality: All static analysis and linting checks pass cleanly Completes the comprehensive CodeRabbit review response.
Co-authored-by: Igor Pinchuk <somework@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
.github/workflows/ci.yml (1)
26-28: Remove redundantcontinue-on-error: false.This is the default behavior for workflow steps. Removing it reduces noise.
- name: Run tests with coverage run: composer test -- --coverage-clover=build/logs/clover.xml - continue-on-error: falseUPGRADE.md (1)
50-67: Consider adding a note about generator consumption semantics.The migration example shows
yield $item, but developers migrating existing code should also understand that generators can only be iterated once. A brief note about this behavior change would help prevent confusion.Consider adding after line 66:
> **Note**: Unlike the previous interface, generators can only be consumed once. > If you need to iterate multiple times, collect results with `iterator_to_array()` first.tests/IntegrationTest.php (1)
361-366: Explicit empty generator handling is correct but could be simplified.The
yield from []; return;pattern works but is verbose. An alternative is simply returning without yielding when there's no data.if ($startIndex >= $totalItems) { - // Return empty generator explicitly - yield from []; - - return; + return; // Empty generator - no items to yield }Note: This suggestion maintains the same behavior since a generator function that returns without yielding is still a valid empty generator.
tests/OffsetResultTest.php (1)
378-381: Consider adding type hint to getGenerator helper.The
@return \GeneratorPHPDoc is present but adding explicit types would improve IDE support and static analysis.- protected function getGenerator(array $value): \Generator + /** + * @param array<mixed> $value + * @return \Generator<mixed> + */ + protected function getGenerator(array $value): \Generator { yield from $value; }tests/PropertyBasedTest.php (1)
65-70: Nested generator pattern may not matchOffsetResultexpectations.The generator wraps data in a nested generator using
yield (static fn () => yield from $data)();. However,OffsetResultexpects aGenerator<Generator<T>>where each inner generator represents a page. This pattern yields a single generator containing all data, which works but doesn't truly test the multi-page scenario.For a more representative test of the pagination behavior, consider yielding items directly or testing with multiple page generators:
- $generator = static function () use ($data) { - yield (static fn () => yield from $data)(); - }; + $generator = static function () use ($data) { + // Single page containing all data + yield (static function () use ($data) { + yield from $data; + })(); + };This is functionally equivalent but more readable. The current implementation works correctly for validating
fetchAll()andgetFetchedCount()behavior.src/OffsetResult.php (1)
43-49: Minor docblock improvement forempty()factory.The
@return OffsetResult<T>annotation is technically incorrect since the empty generator usesnever-returntype. Consider using a more precise return type:/** - * @return OffsetResult<T> + * @return self<never> */ public static function empty(): selfThis is a minor typing nit; the implementation is correct.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
.github/actions/composer-install/action.yml(1 hunks).github/workflows/ci.yml(1 hunks)README.md(2 hunks)UPGRADE.md(1 hunks)src/Exception/InvalidPaginationArgumentException.php(1 hunks)src/OffsetResult.php(2 hunks)tests/ArraySource.php(1 hunks)tests/IntegrationTest.php(6 hunks)tests/OffsetAdapterTest.php(1 hunks)tests/OffsetResultTest.php(6 hunks)tests/PropertyBasedTest.php(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- README.md
🧰 Additional context used
🧬 Code graph analysis (4)
tests/ArraySource.php (4)
src/OffsetResult.php (1)
execute(104-112)src/SourceInterface.php (1)
execute(50-50)src/SourceCallbackAdapter.php (1)
execute(44-56)src/OffsetAdapter.php (1)
execute(65-77)
src/OffsetResult.php (2)
src/OffsetAdapter.php (1)
execute(65-77)src/SourceInterface.php (1)
execute(50-50)
tests/PropertyBasedTest.php (2)
src/SourceCallbackAdapter.php (2)
SourceCallbackAdapter(30-57)execute(44-56)src/OffsetResult.php (6)
execute(104-112)fetch(54-64)generator(89-92)OffsetResult(27-113)fetchAll(69-79)getFetchedCount(94-97)
tests/OffsetAdapterTest.php (4)
src/Exception/InvalidPaginationArgumentException.php (3)
InvalidPaginationArgumentException(22-121)getParameters(117-120)getParameter(105-110)src/OffsetAdapter.php (4)
OffsetAdapter(28-199)execute(65-77)fetchAll(110-113)generator(92-95)src/SourceCallbackAdapter.php (2)
SourceCallbackAdapter(30-57)execute(44-56)src/SourceInterface.php (1)
execute(50-50)
🪛 actionlint (1.7.9)
.github/workflows/ci.yml
49-49: "strategy" section is alias node but mapping node is expected
(syntax-check)
58-58: property "php-version" is not defined in object type {}
(expression)
🪛 PHPMD (2.15.0)
tests/PropertyBasedTest.php
50-50: Avoid unused parameters such as '$_page'. (undefined)
(UnusedFormalParameter)
50-50: Avoid unused parameters such as '$_pageSize'. (undefined)
(UnusedFormalParameter)
100-100: Avoid unused parameters such as '$_page'. (undefined)
(UnusedFormalParameter)
100-100: Avoid unused parameters such as '$_pageSize'. (undefined)
(UnusedFormalParameter)
tests/OffsetAdapterTest.php
163-163: Avoid unused parameters such as '$page'. (undefined)
(UnusedFormalParameter)
282-282: Avoid unused parameters such as '$page'. (undefined)
(UnusedFormalParameter)
282-282: Avoid unused parameters such as '$size'. (undefined)
(UnusedFormalParameter)
🔇 Additional comments (28)
.github/actions/composer-install/action.yml (1)
7-10: Well-structured coverage parameterization.The new
coverageinput with a sensiblenonedefault enables flexible coverage driver selection while maintaining backward compatibility for jobs that don't need coverage. This aligns well with the CI workflow's matrix-based coverage generation..github/workflows/ci.yml (2)
11-14: YAML anchor/alias for matrix reuse is valid but may trigger linter warnings.The
&php-matrixanchor and*php-matrixalias (line 49) are valid YAML syntax that GitHub Actions supports. The static analysis warning about "alias node" at line 49 is a known actionlint limitation with YAML aliases. This pattern works correctly in practice.If you encounter issues with the YAML alias in CI, you can inline the matrix definition in the quality job as a fallback.
46-49: Quality job configuration looks correct.The
needs: testsdependency ensures quality checks run only after tests pass. The static analysis hint aboutphp-versionnot being defined (line 58) is a false positive — the property is correctly inherited from the matrix alias.tests/ArraySource.php (1)
30-40: Clean generator-based implementation.The migration to generator-based returns is correctly implemented. The guard
if (0 < $pageSize)properly handles the zero pageSize edge case by yielding nothing, andyield from array_slice()efficiently delegates iteration.UPGRADE.md (1)
20-27: Clear migration guidance for the interface removal.The before/after code examples accurately demonstrate the shift from
SourceResultInterfaceto\Generator. This is the key breaking change developers need to understand.tests/IntegrationTest.php (3)
92-114: Good API failure simulation test.The test correctly verifies lazy evaluation by calling
fetch()(line 113) to trigger the generator execution that throws the exception. The first result usingfetchAll()works because it's a separate generator instance.
192-212: Excellent infinite loop guard pattern.The
maxIterationssafety guard (lines 192-198) prevents tests from hanging if pagination logic has bugs. This defensive pattern should be adopted in similar loop-based tests.
262-299: Memory efficiency test validates streaming behavior.The test verifies that batch processing 10k items keeps memory growth under 5MB. The periodic memory checks every 1000 items provide reasonable coverage without excessive overhead.
tests/OffsetResultTest.php (2)
22-50: Creative use of inline generator IIFEs in data providers.The pattern
(static fn () => yield from ['item'])()is compact but may be harder to parse for developers unfamiliar with generator expressions. This is acceptable for test code where brevity aids readability of the test scenarios.
189-206: Test correctly validates consumed generator behavior.Testing that
iterator_to_array()throws on a consumed generator (line 203-205) ensures thegenerator()method doesn't hide this PHP limitation. Using\Exception::classis correct since PHP throws a genericExceptionfor closed generators.src/Exception/InvalidPaginationArgumentException.php (4)
22-41: Well-designed exception class with rich context.The constructor pattern storing parameters separately from the message enables programmatic access to error context, which is valuable for logging and error handling. Implementing
PaginationExceptionInterfaceenables typed catch blocks.
78-96: Clear and actionable error message for zero limit validation.The multi-line message in
forInvalidZeroLimitexplains:
- What the constraint is
- Why it exists (semantic meaning of zero limit)
- Alternative approach (use large limit value)
This is excellent user-facing error design.
105-110: Usingarray_key_existsis correct for nullable values.This correctly handles the case where a parameter is explicitly set to
nullvs not being set at all.
52-67: The hardcoded message "greater than or equal to zero" is appropriate for this factory method's single use case.The
forInvalidParameterfactory is called only once inOffsetAdapter.php(line 163) to validate three parameters (offset,limit,nowCount) with the constraintif (0 > $value), which explicitly checks for non-negative integers. The message accurately reflects this constraint, and dedicated factory methods likeforInvalidZeroLimitexist for handling different constraint types.Likely an incorrect or invalid review comment.
tests/PropertyBasedTest.php (3)
84-112: LGTM!The callback signatures now correctly accept
(int $_page, int $_pageSize)parameters, fixing the previous issue. The test properly validates thatSourceCallbackAdapterthrowsUnexpectedValueExceptionfor invalid return types. The underscore-prefixed parameter names clearly indicate intentionally unused parameters.
115-142: Good equivalence test for streaming vs batch consumption.The test correctly validates that
fetch()andfetchAll()produce identical results when consuming the same data. Using separateOffsetResultinstances for each consumption method is the right approach since generators can only be consumed once.
144-163: LGTM!The type safety test correctly validates that mixed data types are handled properly through the pagination system. The use of
ArraySourceprovides a clean, deterministic test fixture.src/OffsetResult.php (4)
51-64: LGTM!The
fetch()method correctly advances the generator and returns items one at a time, returningnullwhen exhausted. The implementation properly handles the generator state.
66-79: LGTM!The
fetchAll()method correctly drains the remaining generator contents into an array. The implementation is clean and efficient.
81-92: Good warning in docblock about generator consumption.The documentation clearly warns users about the single-consumption nature of the generator. This is important for preventing misuse.
104-112: LGTM!The
execute()method correctly flattens the nested page generators and increments the fetched count for each yielded item. The double-foreach pattern properly handles theGenerator<Generator<T>>input structure.tests/OffsetAdapterTest.php (7)
98-112: Test creates two separate adapter calls but compares results as if they should match.The test calls
execute(2, 3)andgenerator(2, 3)on the same adapter with the same parameters. This is valid for testing equivalence, but note that$result->fetchAll()is called after$generatoris created, so the order of consumption doesn't affect the comparison. The test logic is sound.
160-174: Good pagination termination test.The test correctly validates that the adapter doesn't loop endlessly when fetching paginated data. The counter check (
assertLessThanOrEqual(2, $counter)) ensures the source isn't called excessively. This is an important safeguard against infinite loops.
176-184: LGTM!The test correctly validates that when
nowCount >= limit, no additional items are fetched. This is the expected behavior when the caller has already retrieved enough items.
209-237: Comprehensive zero-limit rejection tests.The three tests thoroughly validate the zero-limit sentinel logic:
- Rejects
limit=0whennowCount > 0- Rejects
limit=0whenoffset > 0- Rejects
limit=0when both are non-zeroThe exception message assertions verify the helpful error messages. Good coverage of edge cases.
280-292: Good empty source handling test.The test validates that the adapter gracefully handles sources that return empty generators immediately, returning an empty result with zero fetched count.
294-308: LGTM!The test properly validates that
InvalidPaginationArgumentExceptionexposes all parameter values throughgetParameters()andgetParameter()methods. Using try-catch with explicit assertions is appropriate here to verify exception properties.
24-34: Verify expected result fornowCountbehavior is correct.The test correctly verifies nowCount semantics. With execute(0, 3, 2), where offset=0, limit=3, and nowCount=2, the adapter calculates that 1 more item is needed (3-2). The Offset::logic implementation recursively adjusts the request to Offset::logic(2, 1), which returns page=3, pageSize=1. ArraySource then yields the item at index 2 (the third element: value 3), matching the expected result [3] and fetchedCount of 1. The comment's explanation is accurate: nowCount represents already-fetched items, and the adapter correctly starts from that position and returns only the remaining items needed to reach the limit.
- CI: Remove redundant continue-on-error: false (default behavior) - UPGRADE.md: Add note about generator consumption semantics for migration clarity All automated code review suggestions now fully addressed.
|
@coderabbitai generate docstrings |
✅ Actions performedInitiated docstring generation; will generate only if new commits exist. |
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @somework. * #7 (comment) The following files were modified: * `src/Exception/InvalidPaginationArgumentException.php` * `src/Exception/InvalidPaginationResultException.php` * `src/OffsetAdapter.php` * `src/OffsetResult.php` * `src/SourceCallbackAdapter.php` * `tests/ArraySource.php`
…n` (#11) * 📝 Add docstrings to `codex/analyze-pagination-behavior-and-test-design` Docstrings generation was requested by @somework. * #7 (comment) The following files were modified: * `src/Exception/InvalidPaginationArgumentException.php` * `src/Exception/InvalidPaginationResultException.php` * `src/OffsetAdapter.php` * `src/OffsetResult.php` * `src/SourceCallbackAdapter.php` * `tests/ArraySource.php` * Apply fixes from StyleCI (#12) Co-authored-by: Igor Pinchuk <somework@users.noreply.github.com> --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: Igor Pinchuk <i.pinchuk.work@gmail.com> Co-authored-by: Igor Pinchuk <somework@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/SourceCallbackAdapter.php (1)
53-59: Redundant context string produces awkward error message.The
$contextparameter value'should return Generator'combined with the exception's format string produces:"Callback (should return Generator) must return Generator, got ...". Consider a more meaningful context like'SourceCallbackAdapter'or omit it entirely.if (!$result instanceof \Generator) { throw InvalidPaginationResultException::forInvalidCallbackResult( $result, \Generator::class, - 'should return Generator', + 'SourceCallbackAdapter', ); }src/Exception/InvalidPaginationArgumentException.php (1)
54-69: Minor: Error message wording could be clearer.The message states the parameter "must be greater than or equal to zero" which is correct, but when a negative value is passed, it might be clearer to say "must be non-negative" or "cannot be negative" to directly state what's wrong.
$message = sprintf( - '%s must be greater than or equal to zero, got %s. Use a non-negative integer to specify the %s.', + '%s cannot be negative, got %s. Use a non-negative integer to specify the %s.', $parameterName, is_scalar($value) ? $value : gettype($value), $description, );src/Exception/InvalidPaginationResultException.php (1)
55-66:forInvalidSourceResult()is unused within the codebase.This factory method is defined but has no usages anywhere. If this is a public library, it may be intentionally exposed for external consumers. If not, consider removing it to reduce maintenance burden.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.github/workflows/ci.yml(1 hunks)UPGRADE.md(1 hunks)src/Exception/InvalidPaginationArgumentException.php(1 hunks)src/Exception/InvalidPaginationResultException.php(1 hunks)src/OffsetAdapter.php(1 hunks)src/OffsetResult.php(3 hunks)src/SourceCallbackAdapter.php(1 hunks)tests/ArraySource.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
src/Exception/InvalidPaginationArgumentException.php (4)
src/OffsetAdapter.php (1)
__construct(37-39)src/OffsetResult.php (1)
__construct(39-42)src/SourceCallbackAdapter.php (1)
__construct(37-39)tests/ArraySource.php (1)
__construct(28-30)
src/SourceCallbackAdapter.php (2)
src/Exception/InvalidPaginationResultException.php (2)
InvalidPaginationResultException(22-67)forInvalidCallbackResult(33-45)src/SourceInterface.php (1)
execute(50-50)
tests/ArraySource.php (1)
src/SourceInterface.php (1)
execute(50-50)
src/OffsetResult.php (2)
src/OffsetAdapter.php (2)
generator(92-95)execute(67-79)src/SourceInterface.php (1)
execute(50-50)
src/OffsetAdapter.php (4)
src/Exception/InvalidPaginationArgumentException.php (4)
InvalidPaginationArgumentException(22-123)__construct(35-43)forInvalidParameter(54-69)forInvalidZeroLimit(80-98)src/OffsetResult.php (6)
__construct(39-42)execute(126-134)OffsetResult(27-135)empty(49-55)generator(102-105)fetchAll(83-93)src/SourceCallbackAdapter.php (3)
__construct(37-39)SourceCallbackAdapter(30-63)execute(50-62)src/SourceInterface.php (1)
execute(50-50)
🪛 actionlint (1.7.9)
.github/workflows/ci.yml
48-48: "strategy" section is alias node but mapping node is expected
(syntax-check)
57-57: property "php-version" is not defined in object type {}
(expression)
🔇 Additional comments (13)
.github/workflows/ci.yml (2)
48-48: YAML anchor/alias for strategy may not work in GitHub Actions workflows.Using
strategy: *php-matrixassumes the workflow engine resolves YAML anchors correctly. However, GitHub Actions may not support this syntax reliably, potentially causing the quality job to fail or use an unexpected environment.To ensure compatibility, consider duplicating the matrix explicitly instead of using YAML aliases:
- quality: - needs: tests - runs-on: ubuntu-latest - strategy: *php-matrix - - steps: + quality: + needs: tests + runs-on: ubuntu-latest + strategy: + fail-fast: false + matrix: + php-version: [ '8.2', '8.3' ] + + steps:Alternatively, verify with your GitHub Actions version that YAML anchors are supported in the strategy context.
26-27: PHPUnit configuration is correctly set up.The
phpunit.xml.distproperly configures all required artifacts:build/logs/clover.xml,build/logs/junit.xml, andbuild/coverage/directory. The workflow will successfully generate these outputs when running tests with coverage.src/OffsetResult.php (3)
49-55: Theempty()factory method correctly creates a no-op generator.The implementation uses
yield from []to create a generator that yields nothing, which is the correct approach for an empty result. The docblock accurately describes the behavior.
64-74: Thefetch()method correctly advances the generator.The implementation properly checks validity, retrieves the current value, advances the generator, and returns
nullwhen exhausted. The return typemixedwith docblockT|nullis appropriate for this pattern.
126-134: Generator flattening logic is correct.The
execute()method properly iterates over page generators and yields individual items while tracking the fetched count. This aligns with the new architecture where sources returnGenerator<Generator<T>>.UPGRADE.md (1)
1-101: Comprehensive and well-structured upgrade guide.The documentation clearly explains breaking changes, provides before/after code examples, and includes helpful notes about generator single-use behavior. The migration steps are actionable and accurate.
tests/ArraySource.php (1)
40-47: Clean implementation of generator-based source.The
execute()method correctly implements the updatedSourceInterfacecontract, handling edge cases for invalid page numbers and page sizes gracefully.src/Exception/InvalidPaginationResultException.php (1)
22-45: Well-designed exception class with informative factory methods.The static factory pattern provides clear, context-rich error messages using
get_debug_type()for accurate type reporting. This aligns with the PR's goal of improving developer experience.src/Exception/InvalidPaginationArgumentException.php (1)
22-43: Good pattern: Parameter-rich exceptions for debugging.Storing the invalid parameters on the exception allows callers to programmatically access the values that caused the error, which is useful for logging and debugging.
src/OffsetAdapter.php (4)
71-76: Clarify semantics:limit=0means "empty result" when all params are zero.When
offset=0,limit=0, andnowCount=0, the method returns an empty result. However, the docblock stateslimit=0means "no limit". This edge case appears intentional (fetching nothing from position 0), but the semantics might confuse users expecting "no limit" behavior.Consider adding a note to the docblock clarifying this edge case, or verify this is the intended behavior for your use case.
196-211: Pass-by-reference pattern works but adds cognitive overhead.The
createLimitedGenerator()method modifies$totalDeliveredand$currentNowCountvia reference. While functional, this pattern can be harder to reason about. The current implementation is acceptable given the tight coupling withinlogic().
167-184: Comprehensive argument validation.The
assertArgumentsAreValid()method properly validates all parameters and provides descriptive error messages via the new exception hierarchy. The match expression for descriptions is clean and maintainable.
50-53: Convenient factory method for callback-based sources.The
fromCallback()static factory simplifies the common use case of wrapping a callback, reducing boilerplate for users.
- Add testFromCallbackCreatesAdapterWithCallbackSource() to verify fromCallback works - Add testFromCallbackWithEmptyData() for edge case testing - Tests cover pagination behavior and callback invocation - Improves method coverage from 55.56% to higher percentage Resolves coverage gap for the fromCallback static factory method.
- Add comprehensive fromCallback method tests - Add edge case tests for empty generators - Add comprehensive method execution test - Improve method coverage from 66.67% to 77.78% (7/9 methods) - Improve line coverage from 92.86% to 95.24% (40/42 lines) Remaining 2 uncovered lines are in AlreadyGetNeededCountException catch block, which is a pagination optimization exception that may be difficult to trigger reliably.
- Improve method coverage from 77.78% to 88.89% (8/9 methods) - Improve line coverage from 95.24% to 97.62% (41/42 lines) - Add testLimitReachedInGeneratorProcessing() to cover break condition in createLimitedGenerator - Add testPaginationWithExtremeParameters() for edge case handling - Successfully covered limit enforcement logic (line 204) Remaining 1 uncovered line (line 138) is safety check for invalid pagination parameters, which is an edge case unlikely to occur in normal operation with the pagination library.
- Add testPaginationExceptionBaseClass() to verify inheritance and interface implementation - PaginationException is an empty base class, so 0% coverage is expected (no executable lines) - Confirms proper inheritance: RuntimeException -> PaginationException -> PaginationExceptionInterface Functional classes maintain excellent coverage: - OffsetAdapter: 88.89% methods, 97.62% lines - OffsetResult: 100% methods, 100% lines - SourceCallbackAdapter: 100% methods, 100% lines - Exception classes: 100% coverage each
Transformed offset-page from functional library to Packagist-ready, DX-first solution: - Enhanced API with fromCallback() and fetchAll() convenience methods - Enterprise exception architecture with comprehensive error handling - Complete documentation overhaul (README, CONTRIBUTING, UPGRADE) - Packagist optimization with SEO keywords and metadata - 99.07% line coverage, 104 tests, enterprise-grade quality - Removed unused PaginationException dead code 29 files changed, 1831 additions, 1356 deletions 104 tests passing, 509 assertions Ready for production release!
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/OffsetAdapterTest.php (2)
371-386: Simplify the empty generator pattern.The
return; yield;pattern creates a generator that yields nothing, but the unreachableyieldstatement on line 377 is confusing. While the comment explains the intent, this can be simplified for better readability.Apply this diff to use a clearer pattern:
- $source = new SourceCallbackAdapter(function (int $page, int $pageSize) { - // Return an empty generator (never yields anything) - return; - yield; // This line is never reached - }); + $source = new SourceCallbackAdapter(function (int $page, int $pageSize) { + // Return an empty generator (never yields anything) + yield from []; + });Alternatively, if testing the specific
returnbehavior is intentional, remove the unreachableyieldand adjust the callback signature or implementation to make the generator nature explicit.
418-437: Use conventional comparison order.The comparison
5 >= $ion line 423 is backwards from the conventional$i <= 5style, making it slightly harder to read. While functionally correct, following conventional ordering improves readability.Apply this diff:
- for ($i = 1; 5 >= $i; $i++) { + for ($i = 1; $i <= 5; $i++) { yield "item{$i}"; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Exception/InvalidPaginationResultException.php(1 hunks)tests/OffsetAdapterTest.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Exception/InvalidPaginationResultException.php
🧰 Additional context used
🪛 PHPMD (2.15.0)
tests/OffsetAdapterTest.php
163-163: Avoid unused parameters such as '$page'. (undefined)
(UnusedFormalParameter)
282-282: Avoid unused parameters such as '$page'. (undefined)
(UnusedFormalParameter)
282-282: Avoid unused parameters such as '$size'. (undefined)
(UnusedFormalParameter)
360-360: Avoid unused parameters such as '$page'. (undefined)
(UnusedFormalParameter)
360-360: Avoid unused parameters such as '$pageSize'. (undefined)
(UnusedFormalParameter)
374-374: Avoid unused parameters such as '$page'. (undefined)
(UnusedFormalParameter)
374-374: Avoid unused parameters such as '$pageSize'. (undefined)
(UnusedFormalParameter)
421-421: Avoid unused parameters such as '$page'. (undefined)
(UnusedFormalParameter)
421-421: Avoid unused parameters such as '$pageSize'. (undefined)
(UnusedFormalParameter)
Co-authored-by: Igor Pinchuk <somework@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
tests/OffsetAdapterTest.php (4)
60-96: Exception behavior coverage is strong but tightly coupled to message wordingThe exception‑oriented tests (
testExceptionProvidesAccessToParameterValues,testExceptionsImplementPaginationExceptionInterface, the zero‑limit rejection tests, and the negative argument tests) do a good job of validating:
- Parameter capture via
getParameters()/getParameter()- That pagination exceptions implement
PaginationExceptionInterface- Exact messages from
InvalidPaginationArgumentException::forInvalidParameter()and::forInvalidZeroLimit()However, using
expectExceptionMessage()with the full multi‑sentence string makes the tests brittle to any future copy or UX tweaks to these messages. You already useassertStringContainsString()intestExceptionProvidesAccessToParameterValues; consider using similar “contains” checks (or structured error codes) for the longer zero‑limit and negative‑argument messages so you can evolve wording without breaking tests, while still asserting key substrings (e.g., parameter name and non‑negative requirement). Based on relevant_code_snippets, this aligns with howInvalidPaginationArgumentExceptioncurrently formats its messages.Also applies to: 209-237, 239-278, 294-308, 310-317
98-112: Generator API tests are comprehensive; consider a couple of small enhancementsThe generator‑focused tests (
testGeneratorMethodReturnsGeneratorWithSameData,testGeneratorMethodWithLargeDataset,testGeneratorMethodWithNowCountParameter,testGeneratorMethodWithZeroLimitSentinel,testGeneratorMethodWithEdgeCaseParameters,testAllMethodsExecutedThroughDifferentPaths, andtestLimitReachedInGeneratorProcessing) nicely verify:
- Parity between
execute()->fetchAll(),generator(), andfetchAll()convenience methods.- Correct handling of
nowCountandlimitin streaming scenarios.- Enforcement of the limit when the underlying source yields more items than requested.
Two optional tweaks you might consider:
- Add explicit
assertInstanceOf(\Generator::class, $generator);in one or two places where you call$adapter->generator(...), to lock in the return type independently of howiterator_to_array()behaves.- In the generator tests where you don’t currently assert
getFetchedCount()(e.g.,testGeneratorMethodWithLargeDataset), you could also assert the count to catch any discrepancies between delivered items and the internal counter.These are minor; the current tests already give good coverage.
Also applies to: 122-148, 150-158, 389-399, 401-417, 419-438
150-158: Zero‑limit sentinel semantics vs. exception message—worth a quick double‑checkThe tests treat
limit=0withoffset=nowCount=0as a special “zero sentinel” that yields an empty result (testGeneratorMethodWithZeroLimitSentinel,testZeroLimitSentinelReturnsEmptyResult), while other combinations withlimit=0are rejected withInvalidPaginationArgumentException::forInvalidZeroLimit()and a message explaining that zero limit “indicates 'fetch all remaining items' and can only be used at the start of pagination.”Given these tests, it would be good to confirm that:
- The library’s documented semantics for the “zero limit” sentinel (in code comments and README/UPGRADE notes) align with the observed behavior in both the valid (
offset=0, limit=0, nowCount=0) and invalid cases, and- The explanatory message in
forInvalidZeroLimit()still reflects how callers are expected to uselimit=0.If everything is intentional, no code change is needed; otherwise, you may want to tweak either the message text or add a short comment near these tests clarifying the intended meaning of the sentinel. Based on relevant_code_snippets, the current text in
InvalidPaginationArgumentExceptionis slightly more expansive than what these tests directly assert.Also applies to: 209-218, 228-237, 294-308, 310-317
160-174: Callback / empty‑source tests are good; watch for PHPMD noise and fragile generator trickThe callback‑ and empty‑source‑centric tests (
testLoopTerminatesAfterRequestedLimit,testStopsWhenSourceReturnsEmptyImmediately,testFromCallbackCreatesAdapterWithCallbackSource,testFromCallbackWithEmptyData,testExecuteHandlesSourceReturningEmptyGenerator,testLimitReachedInGeneratorProcessing) effectively probe:
- That pagination stops once the requested limit is met and does not loop indefinitely.
- Behavior when sources immediately yield no items (both via
yield from []and a generator that never yields).- The
fromCallbackadapter path and multi‑page behavior.Two minor points:
PHPMD’s
UnusedFormalParameterwarnings for$page/$size/$pageSizein these closures are expected, since the signature is constrained bySourceInterface. If these warnings are noisy, you might suppress them for this test class or excludetests/from that particular PHPMD rule.In
testExecuteHandlesSourceReturningEmptyGenerator, thereturn; yield;pattern relies on the presence of an unreachableyieldpurely to make the function a generator. This is subtle and easy to “clean up” accidentally. If you want to keep the intent but reduce fragility, consider switching to a more explicit empty generator pattern, e.g.:$source = new SourceCallbackAdapter(function (int $page, int $pageSize) { // Empty generator that never yields if (false) { yield; } });or simply
yield from [];if you don’t specifically need the “never yields even once advanced” behavior.These are refinements only; the current tests are functionally fine.
Also applies to: 280-292, 319-370, 372-387, 419-438
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/OffsetAdapterTest.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/OffsetAdapterTest.php (3)
src/SourceCallbackAdapter.php (2)
SourceCallbackAdapter(30-63)execute(50-62)src/OffsetResult.php (4)
execute(126-134)fetchAll(83-93)getFetchedCount(112-115)generator(102-105)src/SourceInterface.php (1)
execute(50-50)
🪛 PHPMD (2.15.0)
tests/OffsetAdapterTest.php
163-163: Avoid unused parameters such as '$page'. (undefined)
(UnusedFormalParameter)
282-282: Avoid unused parameters such as '$page'. (undefined)
(UnusedFormalParameter)
282-282: Avoid unused parameters such as '$size'. (undefined)
(UnusedFormalParameter)
361-361: Avoid unused parameters such as '$page'. (undefined)
(UnusedFormalParameter)
361-361: Avoid unused parameters such as '$pageSize'. (undefined)
(UnusedFormalParameter)
375-375: Avoid unused parameters such as '$page'. (undefined)
(UnusedFormalParameter)
375-375: Avoid unused parameters such as '$pageSize'. (undefined)
(UnusedFormalParameter)
422-422: Avoid unused parameters such as '$page'. (undefined)
(UnusedFormalParameter)
422-422: Avoid unused parameters such as '$pageSize'. (undefined)
(UnusedFormalParameter)
🔇 Additional comments (1)
tests/OffsetAdapterTest.php (1)
24-34: Happy‑path pagination and basic semantics tests look solidThe tests for valid combinations of
offset,limit, andnowCount(includingtestAcceptsValidNowCountParameter,testAcceptsValidPositiveValues,testAcceptsZeroValuesForAllParameters,testNowCountStopsWhenAlreadyEnough,testOffsetGreaterThanLimitNonDivisibleUsesDivisorMapping,testOffsetLessThanLimitUsesLogicPaginationAndStopsAtLimit,testPaginationLogicWithLargeLimits, andtestPaginationWithExtremeParameters) consistently match the intended offset/limit behavior and assert both data and fetched counts appropriately. No changes needed here.Also applies to: 36-48, 50-58, 176-184, 186-207, 440-451, 453-466
| use PHPUnit\Framework\TestCase; | ||
| use SomeWork\OffsetPage\Exception\InvalidPaginationArgumentException; | ||
| use SomeWork\OffsetPage\Exception\PaginationExceptionInterface; | ||
| use SomeWork\OffsetPage\OffsetAdapter; | ||
| use SomeWork\OffsetPage\SourceCallbackAdapter; | ||
| use SomeWork\OffsetPage\SourceInterface; | ||
|
|
There was a problem hiding this comment.
Import ArraySource to avoid undefined class errors
ArraySource is referenced multiple times without an import, so in the SomeWork\OffsetPage\Tests namespace it resolves to SomeWork\OffsetPage\Tests\ArraySource, which will fail at runtime. Import the correct class (or fully qualify it) so the tests run.
use PHPUnit\Framework\TestCase;
+use SomeWork\OffsetPage\ArraySource;
use SomeWork\OffsetPage\Exception\InvalidPaginationArgumentException;
use SomeWork\OffsetPage\Exception\PaginationExceptionInterface;
use SomeWork\OffsetPage\OffsetAdapter;Also applies to: 26-27
🤖 Prompt for AI Agents
In tests/OffsetAdapterTest.php around lines 16-21 (and also occurrences at
26-27), ArraySource is referenced without an import which causes PHP to resolve
it to SomeWork\OffsetPage\Tests\ArraySource; add a proper use statement for the
real ArraySource class (or fully-qualify each usage) so the class resolves
correctly at runtime—update the top imports to include the correct namespace for
ArraySource and adjust any usages if necessary.
🚀 Complete Package Transformation: From Basic Library to Packagist-Ready, DX-First Solution
🎯 Executive Summary
This PR represents a comprehensive transformation of the offset-page library from a functional pagination adapter into a Packagist-ready, developer-experience-first solution that developers will discover, adopt, and love to use.
✨ Transformation Scope: 29 Files, +1,828 Additions, -1,355 Deletions
🔧 API & Core Functionality Enhancement
OffsetAdapter::fromCallback(): 30-second setup vs complex multi-step processOffsetAdapter::fetchAll(): Direct array results convenience method🛡️ Enterprise-Grade Exception Architecture
📚 Documentation Revolution
📦 Packagist Optimization
pagination,offset,api,database,framework-agnostic🛠️ Quality & CI/CD Excellence
🎯 Business Impact
Discoverability & Adoption
Developer Experience
Maintenance & Support
✅ Quality Assurance Verified
📋 Files Changed (29 total)
🏷️ Labels
enhancement,documentation,ci,dx,breaking-change,ready-for-release🎉 This is a Complete Package Renaissance
Before: Basic functional library with minimal documentation
After: Production-ready, discoverable, developer-delighting solution
The offset-page library is now positioned for success in the PHP ecosystem! 🚀
Ready for merge, tag, and Packagist release! ✨
Summary by CodeRabbit
New Features
Breaking Changes
Bug Fixes
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.