do some refactoring and optimizations and fix issues #17
Conversation
WalkthroughThis update introduces comprehensive improvements to CSV reading and writing functionality, including enhanced error handling, explicit state management, header and record caching, and expanded configuration options. The codebase now features complete PHPDoc documentation, improved test coverage for all core classes, and compatibility tests ensuring consistent behavior between FastCSV and SplFileObject implementations. Configuration, control flow, and exception handling are now more robust and explicit. Changes
Sequence Diagram(s)CSV Reading (High-Level, Unified for Spl and FastCSV)sequenceDiagram
participant Client
participant CsvReader/SplCsvReader
participant CsvConfig
participant File (SplFileObject/FastCSVReader)
Client->>CsvReader/SplCsvReader: __construct(source, config)
CsvReader/SplCsvReader->>CsvConfig: (optional) set/get config
CsvReader/SplCsvReader->>File: open source file
CsvReader/SplCsvReader->>File: cache headers (if enabled)
loop Reading records
Client->>CsvReader/SplCsvReader: nextRecord() / getRecord()
CsvReader/SplCsvReader->>File: read record at position
CsvReader/SplCsvReader->>Client: return record or false
end
Client->>CsvReader/SplCsvReader: seek(position)
CsvReader/SplCsvReader->>File: seek to position
CsvReader/SplCsvReader->>Client: return record or false
CSV Writing (High-Level, Unified for Spl and FastCSV)sequenceDiagram
participant Client
participant CsvWriter/SplCsvWriter
participant CsvConfig
participant File (SplFileObject/FastCSVWriter)
Client->>CsvWriter/SplCsvWriter: __construct(target, config, headers)
CsvWriter/SplCsvWriter->>CsvConfig: (optional) set/get config
CsvWriter/SplCsvWriter->>File: open/create target file
loop Writing records
Client->>CsvWriter/SplCsvWriter: write(record) / writeAll(records)
CsvWriter/SplCsvWriter->>File: write record(s)
end
Client->>CsvWriter/SplCsvWriter: close()
CsvWriter/SplCsvWriter->>File: close file
Possibly related PRs
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (10)
src/Contracts/CsvWriterInterface.php (1)
5-7: Fully-qualify or aliasFastCSVWriterto avoid autoload surprises
FastCSVWriteris imported from the global namespace.
Unless the extension truly registers the class as\FastCSVWriter, this will break under PSR-4 autoloaders.
Either use the fully-qualified path that matches the package (\Phpcsv\CsvHelper\FastCSV\FastCSVWriteror similar) or add an alias so the intent is crystal-clear.tests/Configs/CsvConfigTest.php (2)
268-274: Costly tight loop in a unit test – turn it into a data providerThe ten-iteration loop does not add extra assurance; it just prolongs the test run.
Consider replacing it with a single assertion or moving the different states into a data-provider, which will also isolate failures.Not blocking, purely a perf/readability nit.
148-158: Great use of an extensive data-provider – consider addingpathandoffsetcases
CsvConfigexposesgetPath()/getOffset(), yet they aren’t exercised.
Adding a few rows covering those will push coverage to 💯 and spot accidental regressions.tests/Readers/SplCsvReaderTest.php (1)
228-243: Consider explicitly closing the writer before reading.In the Unicode test, the writer is unset but not explicitly closed. While PHP's garbage collector will handle this, it's better to be explicit to ensure the file is fully written and flushed before reading.
$unicodeFile = self::TEST_DATA_DIR . '/unicode.csv'; $writer = new SplCsvWriter($unicodeFile); foreach ($unicodeData as $row) { $writer->write($row); } -unset($writer); +unset($writer); // Ensures writer is closedConsider adding a comment to clarify that unsetting the writer ensures the file is closed and flushed.
src/Writers/SplCsvWriter.php (1)
52-56: Consider creating the directory if it doesn't exist.Instead of throwing an exception when the directory doesn't exist, consider attempting to create it with appropriate permissions. This would make the writer more user-friendly.
// Check if directory exists if (! is_dir($directory)) { - throw new FileNotFoundException("Directory does not exist: $directory"); + // Attempt to create the directory + if (!mkdir($directory, 0777, true) && !is_dir($directory)) { + throw new FileNotFoundException("Failed to create directory: $directory"); + } }tests/Writers/CsvWriterTest.php (1)
63-65: Consider moving extension check to setUp() methodThe fastcsv extension check is repeated in every test method. This creates unnecessary code duplication.
Move the extension check to the
setUp()method:protected function setUp(): void { parent::setUp(); + if (! extension_loaded('fastcsv')) { + $this->markTestSkipped('FastCSV extension not loaded'); + } $this->setupTestDirectory(); $this->testFile = self::TEST_DATA_DIR . '/test_output.csv'; }Then remove the extension checks from individual test methods.
Also applies to: 77-79, 90-92, 107-109, 120-122, 134-136
src/Readers/CsvReader.php (2)
74-76: Simplify redundant configuration checkThe instanceof check with fully qualified class name is redundant. The property is already type-hinted and the constructor ensures it's never null.
-if (! $this->config instanceof \Phpcsv\CsvHelper\Contracts\CsvConfigInterface) { - throw new Exception("Configuration is required"); -} +// Config is guaranteed to be set by constructor
128-131: Remove redundant configuration checkThis check is unnecessary as the config property is initialized in the constructor and type-hinted.
public function getConfig(): CsvConfigInterface { - if (! $this->config instanceof \Phpcsv\CsvHelper\Contracts\CsvConfigInterface) { - throw new Exception("Configuration not set"); - } - return $this->config; }src/Readers/SplCsvReader.php (2)
430-432: Remove redundant configuration checkSame as in CsvReader, this instanceof check is unnecessary since the property is type-hinted and initialized in the constructor.
-if (! $this->config instanceof \Phpcsv\CsvHelper\Contracts\CsvConfigInterface) { - throw new Exception("Configuration not set"); -}
164-203: Record counting implementation matches FastCSV behaviorThe implementation correctly counts only valid records and handles headers properly. While iterating the entire file has performance implications for large files, this matches the FastCSV extension behavior for consistency.
For very large files, consider implementing lazy counting or caching the count more aggressively if performance becomes an issue.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
.gitignore(1 hunks)phpstan.neon.dist(1 hunks)phpunit.xml(1 hunks)src/Configs/AbstractCsvConfig.php(1 hunks)src/Configs/CsvConfig.php(1 hunks)src/Contracts/CsvConfigInterface.php(1 hunks)src/Contracts/CsvReaderInterface.php(1 hunks)src/Contracts/CsvWriterInterface.php(1 hunks)src/Exceptions/InvalidConfigurationException.php(1 hunks)src/Readers/AbstractCsvReader.php(1 hunks)src/Readers/CsvReader.php(8 hunks)src/Readers/SplCsvReader.php(6 hunks)src/Writers/AbstractCsvWriter.php(1 hunks)src/Writers/CsvWriter.php(14 hunks)src/Writers/SplCsvWriter.php(9 hunks)tests/CompatibilityTest.php(1 hunks)tests/Configs/CsvConfigTest.php(1 hunks)tests/Readers/CsvReaderTest.php(2 hunks)tests/Readers/SplCsvReaderTest.php(3 hunks)tests/Writers/CsvWriterTest.php(4 hunks)tests/Writers/SplCsvWriterTest.php(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (9)
src/Contracts/CsvWriterInterface.php (3)
src/Writers/AbstractCsvWriter.php (4)
getWriter(50-50)getConfig(57-57)write(64-64)setTarget(71-71)src/Writers/SplCsvWriter.php (4)
getWriter(43-72)getConfig(79-86)write(94-108)setTarget(216-219)src/Writers/CsvWriter.php (4)
getWriter(52-59)getConfig(100-103)write(111-119)setTarget(194-204)
src/Configs/AbstractCsvConfig.php (2)
src/Contracts/CsvConfigInterface.php (11)
hasHeader(93-93)getDelimiter(18-18)setDelimiter(26-26)getEnclosure(33-33)setEnclosure(41-41)getEscape(48-48)setEscape(56-56)getPath(63-63)setPath(71-71)getOffset(78-78)setOffset(86-86)src/Configs/CsvConfig.php (11)
hasHeader(150-153)getDelimiter(34-37)setDelimiter(45-50)getEnclosure(57-60)setEnclosure(68-73)getEscape(80-83)setEscape(91-96)getPath(103-107)setPath(115-120)getOffset(127-130)setOffset(138-143)
src/Contracts/CsvReaderInterface.php (3)
src/Readers/AbstractCsvReader.php (11)
getReader(65-65)getConfig(72-72)getRecordCount(79-79)rewind(86-90)getCurrentPosition(97-97)getRecord(104-104)nextRecord(111-111)getHeader(118-118)hasRecords(125-125)setSource(132-132)getSource(139-139)src/Readers/CsvReader.php (11)
getReader(55-62)getConfig(126-133)getRecordCount(140-152)rewind(159-172)getCurrentPosition(179-182)getRecord(189-202)nextRecord(209-235)getHeader(242-261)hasRecords(300-314)setSource(334-342)getSource(349-352)src/Readers/SplCsvReader.php (11)
getReader(50-57)getConfig(428-435)getRecordCount(164-203)rewind(210-223)getCurrentPosition(269-272)getRecord(279-313)nextRecord(230-262)getHeader(320-332)hasRecords(377-380)setSource(397-404)getSource(442-445)
src/Contracts/CsvConfigInterface.php (2)
src/Configs/AbstractCsvConfig.php (11)
getDelimiter(50-50)setDelimiter(58-58)getEnclosure(65-65)setEnclosure(73-73)getEscape(80-80)setEscape(88-88)getPath(95-95)setPath(103-103)getOffset(110-110)setOffset(118-118)hasHeader(125-125)src/Configs/CsvConfig.php (11)
getDelimiter(34-37)setDelimiter(45-50)getEnclosure(57-60)setEnclosure(68-73)getEscape(80-83)setEscape(91-96)getPath(103-107)setPath(115-120)getOffset(127-130)setOffset(138-143)hasHeader(150-153)
tests/Configs/CsvConfigTest.php (3)
src/Configs/CsvConfig.php (9)
CsvConfig(13-167)getDelimiter(34-37)getEnclosure(57-60)getEscape(80-83)hasHeader(150-153)setDelimiter(45-50)setEnclosure(68-73)setEscape(91-96)setHasHeader(161-166)src/Configs/AbstractCsvConfig.php (8)
getDelimiter(50-50)getEnclosure(65-65)getEscape(80-80)hasHeader(125-125)setDelimiter(58-58)setEnclosure(73-73)setEscape(88-88)setHasHeader(133-133)src/Contracts/CsvConfigInterface.php (8)
getDelimiter(18-18)getEnclosure(33-33)getEscape(48-48)hasHeader(93-93)setDelimiter(26-26)setEnclosure(41-41)setEscape(56-56)setHasHeader(101-101)
tests/Readers/SplCsvReaderTest.php (5)
src/Exceptions/FileNotFoundException.php (1)
FileNotFoundException(11-17)src/Readers/SplCsvReader.php (12)
SplCsvReader(23-470)getConfig(428-435)getSource(442-445)getReader(50-57)getRecordCount(164-203)nextRecord(230-262)getRecord(279-313)seek(340-370)rewind(210-223)hasRecords(377-380)hasNext(387-390)getHeader(320-332)src/Writers/SplCsvWriter.php (4)
SplCsvWriter(17-278)write(94-108)getConfig(79-86)getSource(236-239)tests/Writers/SplCsvWriterTest.php (16)
Test(60-68)Test(70-77)Test(79-90)Test(92-99)Test(101-109)Test(111-123)Test(125-146)Test(148-171)Test(173-186)Test(188-201)Test(203-216)Test(239-258)Test(260-281)Test(283-305)Test(307-332)Test(334-347)src/Configs/CsvConfig.php (2)
CsvConfig(13-167)hasHeader(150-153)
tests/CompatibilityTest.php (7)
src/Configs/CsvConfig.php (2)
CsvConfig(13-167)hasHeader(150-153)src/Readers/CsvReader.php (11)
CsvReader(22-419)getSource(349-352)getCurrentPosition(179-182)getRecordCount(140-152)getHeader(242-261)nextRecord(209-235)rewind(159-172)seek(269-293)hasRecords(300-314)hasNext(321-327)getConfig(126-133)src/Readers/SplCsvReader.php (11)
SplCsvReader(23-470)getSource(442-445)getCurrentPosition(269-272)getRecordCount(164-203)getHeader(320-332)nextRecord(230-262)rewind(210-223)seek(340-370)hasRecords(377-380)hasNext(387-390)getConfig(428-435)src/Writers/CsvWriter.php (5)
CsvWriter(20-269)getSource(221-224)write(111-119)getConfig(100-103)writeAll(143-151)src/Writers/SplCsvWriter.php (5)
SplCsvWriter(17-278)getSource(236-239)write(94-108)getConfig(79-86)writeAll(269-277)tests/Readers/SplCsvReaderTest.php (12)
Test(88-97)Test(99-106)Test(108-119)Test(121-128)Test(130-137)Test(139-147)Test(149-167)Test(169-182)Test(184-202)Test(204-209)Test(211-226)Test(228-258)tests/Configs/CsvConfigTest.php (4)
Test(15-25)Test(27-35)Test(37-44)Test(46-53)
tests/Writers/CsvWriterTest.php (7)
src/Configs/CsvConfig.php (7)
CsvConfig(13-167)setDelimiter(45-50)setEnclosure(68-73)setHasHeader(161-166)getDelimiter(34-37)getEnclosure(57-60)hasHeader(150-153)src/Exceptions/FileNotFoundException.php (1)
FileNotFoundException(11-17)tests/Readers/SplCsvReaderTest.php (12)
Test(88-97)Test(99-106)Test(108-119)Test(121-128)Test(130-137)Test(139-147)Test(149-167)Test(169-182)Test(184-202)Test(204-209)Test(211-226)Test(228-258)tests/Configs/CsvConfigTest.php (4)
Test(15-25)Test(27-35)Test(37-44)Test(46-53)src/Writers/CsvWriter.php (8)
CsvWriter(20-269)getConfig(100-103)getSource(221-224)getWriter(52-59)write(111-119)writeAll(143-151)setSource(231-234)setConfig(241-246)src/Writers/SplCsvWriter.php (7)
getConfig(79-86)getSource(236-239)getWriter(43-72)write(94-108)writeAll(269-277)setSource(246-249)setConfig(256-261)src/Contracts/CsvConfigInterface.php (6)
setDelimiter(26-26)setEnclosure(41-41)setHasHeader(101-101)getDelimiter(18-18)getEnclosure(33-33)hasHeader(93-93)
src/Readers/SplCsvReader.php (5)
src/Readers/AbstractCsvReader.php (13)
AbstractCsvReader(17-155)rewind(86-90)getConfig(72-72)getReader(65-65)seek(154-154)getRecordCount(79-79)nextRecord(111-111)getCurrentPosition(97-97)getRecord(104-104)getHeader(118-118)hasRecords(125-125)setSource(132-132)getSource(139-139)src/Contracts/CsvReaderInterface.php (11)
rewind(40-40)getConfig(28-28)getReader(21-21)getRecordCount(35-35)nextRecord(61-61)getCurrentPosition(47-47)getRecord(54-54)getHeader(68-68)hasRecords(75-75)setSource(82-82)getSource(89-89)src/Readers/CsvReader.php (16)
rewind(159-172)getConfig(126-133)cacheHeaders(412-418)getReader(55-62)seek(269-293)getRecordCount(140-152)nextRecord(209-235)getCurrentPosition(179-182)getRecord(189-202)getHeader(242-261)hasRecords(300-314)hasNext(321-327)setSource(334-342)setReader(72-118)getSource(349-352)reset(399-405)src/Contracts/CsvConfigInterface.php (3)
hasHeader(93-93)setPath(71-71)getPath(63-63)src/Configs/CsvConfig.php (3)
hasHeader(150-153)setPath(115-120)getPath(103-107)
🔇 Additional comments (36)
.gitignore (1)
6-6: Ignore JUnit XML output file
Addingjunit.xmlto.gitignoreprevents accidental commits of PHPUnit test reports and aligns with your CI logging.src/Exceptions/InvalidConfigurationException.php (1)
7-12: Document custom exception with PHPDoc
The added PHPDoc block clearly explains whenInvalidConfigurationExceptionis thrown, improving code readability and maintainability.src/Configs/CsvConfig.php (2)
7-12: Add class-level documentation
The new PHPDoc onCsvConfigoffers a concise overview of its purpose and fluent interface, enhancing discoverability.
21-27: Initialize configuration via constructor
The optional parameters for$pathand$hasHeaderstreamline setup. Consider validating the file path format if early error detection is desired.src/Writers/AbstractCsvWriter.php (8)
10-16: Add class-level PHPDoc
The abstract class doc now succinctly describes its role and responsibilities for CSV writers.
19-32: Document protected properties
Each property is now clearly annotated, which aids in understanding the writer’s internal state.
34-43: Document constructor signature
The PHPDoc for__constructcorrectly details parameters for target and config injection.
45-51: DocumentgetWriter()contract
The return type annotation aligns with the method signature, clarifying supported writer implementations.
52-57: DocumentgetConfig()contract
Explicitly stating the return type improves interface clarity for consumers.
59-65: Documentwrite()method
The param annotation provides clear guidance on expected record format.
66-72: DocumentsetTarget()method
The PHPDoc clearly defines its purpose for setting the output path.
73-79: DocumentgetTarget()method
The return annotation here completes the contract for retrieving the output path.src/Contracts/CsvConfigInterface.php (7)
5-10: Add interface-level PHPDoc
The new doc block effectively summarizes the role ofCsvConfigInterfacein the CSV helper.
13-26: Document delimiter accessors
The getter and setter PHPDocs for the delimiter now specify default and chaining behavior, making the API self-descriptive.
28-41: Document enclosure accessors
The enclosure getter/setter docs improve clarity on quoting behavior.
43-56: Document escape character accessors
The escape getter/setter docs clearly outline how to handle escape sequences.
58-71: Document file path accessors
These docs detail the path management API, ensuring correct usage.
73-86: Document offset accessors
The offset getter/setter docs make the row-skipping mechanism explicit.
88-102: Document header presence accessors
ThehasHeaderandsetHasHeaderdocs clearly convey header row handling.src/Contracts/CsvWriterInterface.php (1)
42-49: Interface now requiresgetTarget()– double-check that every implementation honors it
getTarget()is new in the interface, butAbstractCsvWriter(and thereforeCsvWriter/SplCsvWriter) did not expose this method in the snippet we received.
If any concrete writer misses it, the class will be non-instantiable (Fatal error: Class must implement…).
Please verify and add the method where missing.src/Configs/AbstractCsvConfig.php (1)
15-29: Docblocks look good – no functional impact detectedsrc/Contracts/CsvReaderInterface.php (1)
91-97:setConfig()added – ensure concrete readers implement itNeither
CsvReadernorSplCsvReadershowed asetConfig()method in the provided snippets.
Compilation will fail if they still rely on constructor injection only.
Please confirm and add an implementation or drop the method from the interface.phpunit.xml (1)
29-35: Duplicate<coverage>section after moving filterAfter the change above you’ll have two
<coverage>elements.
Keep only one; otherwise PHPUnit will bail out with a duplicate-node error.src/Readers/AbstractCsvReader.php (1)
10-155: Excellent documentation improvements!The comprehensive PHPDoc comments for the class, properties, and methods significantly improve code clarity and maintainability. The explicit type declarations and nullability indicators are also valuable additions.
src/Writers/SplCsvWriter.php (2)
47-72: Good error handling improvements!The enhanced error handling with specific directory checks and proper exception wrapping improves the user experience by providing clearer error messages.
269-277: Well-implemented batch writing method!The
writeAllmethod includes proper validation to ensure each record is an array, preventing potential errors during CSV writing.tests/CompatibilityTest.php (1)
14-561: Excellent comprehensive compatibility test suite!This test class provides thorough coverage of compatibility between the two CSV implementations. The use of data providers, proper handling of the optional fastcsv extension, and testing of edge cases like Unicode content demonstrate high-quality test design.
tests/Readers/CsvReaderTest.php (1)
17-554: Well-structured and comprehensive test suite!This test class provides excellent coverage of the CsvReader functionality with:
- Consistent structure matching SplCsvReaderTest for easy comparison
- Proper handling of the optional fastcsv extension
- Comprehensive edge case testing including malformed CSV and Unicode content
- Performance testing with a 1000-record file
src/Writers/CsvWriter.php (2)
143-151: Good implementation of batch writing with validationThe
writeAll()method properly validates input and provides a convenient way to write multiple records. The use of\InvalidArgumentExceptionfor parameter validation is appropriate.
216-234: Good API consistency with alias methodsThe
getSource()andsetSource()methods provide API consistency with reader classes, making the interface more intuitive for users switching between readers and writers.tests/Writers/SplCsvWriterTest.php (2)
15-58: Well-organized test structure with proper resource managementThe test setup properly creates and cleans up test directories and files. Good use of PHPUnit attributes and constants for maintainability.
240-281: Excellent test coverage for edge casesThe tests comprehensively cover special characters, Unicode content, and various data types. The assertions correctly account for SplFileObject's quoting behavior.
tests/Writers/CsvWriterTest.php (1)
354-355: Good cross-platform line ending handlingThe use of
preg_split('/\r\n|\r|\n/', trim($contents))properly handles different line ending styles across platforms.Also applies to: 383-384, 433-434
src/Readers/CsvReader.php (2)
87-118: Excellent error handling with specific exceptionsThe error handling properly distinguishes between different failure scenarios and throws appropriate exceptions. The preemptive empty file check is a good defensive programming practice.
174-235: Well-implemented position tracking with cachingThe position tracking correctly starts at -1 and updates only after successful reads. The record caching prevents unnecessary re-reads and improves performance.
src/Readers/SplCsvReader.php (1)
104-123: Clean initialization logicThe
initializeReader()method properly separates initialization concerns and ensures the reader starts in a consistent state matching FastCSV behavior.
| @@ -5,6 +5,7 @@ parameters: | |||
| excludePaths: | |||
| - tests/* | |||
| reportUnmatchedIgnoredErrors: true | |||
| treatPhpDocTypesAsCertain: false | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Disabling PHPDoc certainty – version and type-hint coverage checks.
The new treatPhpDocTypesAsCertain: false flag requires PHPStan ≥ 1.0.0 and will expose any methods or properties lacking native type declarations.
- Confirm
phpstan/phpstanin your composer.json/lock is updated to a compatible version. - Audit public APIs to ensure all parameters, return types, and properties have explicit native type hints.
Run:
🏁 Script executed:
#!/bin/bash
# Verify phpstan version constraint in composer files
grep -R '"phpstan/phpstan"' -n composer.json composer.lockLength of output: 1326
PHPStan ≥1.0.0 requirement satisfied
Your composer.json pins "phpstan/phpstan": "^2.1", so treatPhpDocTypesAsCertain: false is fully supported.
Next, please audit your public API surface to ensure no type information is lost when PHPStan ignores PHPDoc types:
- Review all public
functionsignatures for missing parameter and return type declarations. - Check class properties to verify they declare native types.
🤖 Prompt for AI Agents
In phpstan.neon.dist at line 8, the setting treatPhpDocTypesAsCertain is set to
false, which requires PHPStan version 1.0.0 or higher and enforces native type
declarations. Confirm your composer.json and composer.lock files specify
phpstan/phpstan version 1.0.0 or above (which is already done). Next, audit all
public APIs in your codebase to ensure every public function has explicit native
parameter and return type declarations, and all class properties have native
type hints, removing reliance on PHPDoc types.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
phpunit.xml (1)
19-33: Consider enablingprocessUncoveredFilesfor more accurate coverage %The
<coverage>block omitsprocessUncoveredFiles="true".
With PHPUnit 10/11 this flag defaults to false, so lines that are never executed are ignored in coverage metrics, giving an optimistic result.- <coverage> + <coverage processUncoveredFiles="true">Purely optional, but helps keep the reported coverage honest.
src/Writers/CsvWriter.php (2)
5-13: Unused import and impossible return-type
SplFileObjectis imported and included in the return union of
getWriter(), yet the FastCSV implementation never produces an
SplFileObject. Keeping dead types hurts static analysis.-use SplFileObject; … - public function getWriter(): SplFileObject|FastCSVWriter|null + public function getWriter(): FastCSVWriter|null
145-149:writeAll()silently accepts non-scalar fieldsUnlike
SplCsvWriter, this class forwards data straight to FastCSV without
type-sanitising. If an element is an array/object the extension may choke at
runtime.Consider re-using
convertToString()from the SPL implementation (or extracting
it into a shared trait) to keep behaviour consistent between the two writers.tests/Writers/CsvWriterTest.php (1)
482-490: Micro-benchmarks are brittle in CI
assertLessThan(5.0, $elapsed)can fail on slower CI runners, leading to
flaky builds. Either raise the threshold generously or mark the test as
@small/@mediumand skip the timing assertion.tests/Writers/SplCsvWriterTest.php (1)
492-500: Same timing-sensitive assertion as in FastCSV testsSee comment above – hard wall-clock limits are unreliable on shared runners.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
phpunit.xml(1 hunks)src/Contracts/CsvReaderInterface.php(1 hunks)src/Exceptions/DirectoryNotFoundException.php(1 hunks)src/Readers/AbstractCsvReader.php(1 hunks)src/Writers/CsvWriter.php(14 hunks)src/Writers/SplCsvWriter.php(9 hunks)tests/CompatibilityTest.php(1 hunks)tests/Writers/CsvWriterTest.php(4 hunks)tests/Writers/SplCsvWriterTest.php(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/Exceptions/DirectoryNotFoundException.php
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Readers/AbstractCsvReader.php
- tests/CompatibilityTest.php
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/Contracts/CsvReaderInterface.php (3)
src/Readers/AbstractCsvReader.php (11)
getReader(65-65)getConfig(72-72)getRecordCount(79-79)rewind(89-93)getCurrentPosition(100-100)getRecord(107-107)nextRecord(114-114)getHeader(121-121)hasRecords(128-128)setSource(135-135)getSource(142-142)src/Readers/CsvReader.php (11)
getReader(55-62)getConfig(126-133)getRecordCount(140-152)rewind(159-172)getCurrentPosition(179-182)getRecord(189-202)nextRecord(209-235)getHeader(242-261)hasRecords(300-314)setSource(334-342)getSource(349-352)src/Readers/SplCsvReader.php (11)
getReader(50-57)getConfig(428-435)getRecordCount(164-203)rewind(210-223)getCurrentPosition(269-272)getRecord(279-313)nextRecord(230-262)getHeader(320-332)hasRecords(377-380)setSource(397-404)getSource(442-445)
🔇 Additional comments (3)
src/Writers/SplCsvWriter.php (1)
118-123:array_map($this->convertToString(...), …)relies on PHP 8.1+ first-class callablesThe syntax is perfectly legal from 8.1 onwards, but if PHP 8.0 support is still
a goal for this library, replace with[$this, 'convertToString'].src/Contracts/CsvReaderInterface.php (2)
49-55: Clean return-type union—good fix
getRecord()now advertisesarray|false, eliminating the spuriousstringvariant noted in the previous review.
This tightens static analysis and aligns with all concrete reader implementations. 👍
56-62: NewnextRecord()contract—verify full coverageThe interface adds
nextRecord(): array|false, which is a welcome explicit iterator helper.
Please double-check that every existing reader and the abstract base class implement this method; otherwise,fatal error: Class ... contains 1 abstract method and must therefore be declared abstractmay surface.If all implementations are already updated (as it appears from the diff snippets), feel free to ignore.
| use FastCSVReader; | ||
| use SplFileObject; |
There was a problem hiding this comment.
Fatal compile-time risk when FastCSV extension is not installed
use FastCSVReader; introduces a hard dependency: PHP will throw an undeclared class fatal error as soon as this interface is autoloaded on hosts where the FastCSV extension is missing.
Because the name also appears in the union return types (e.g. getReader(): SplFileObject|FastCSVReader|null), the same crash occurs even before any concrete reader is constructed.
Mitigate by providing a lightweight polyfill or by removing the extension type from the public contract and letting concrete readers widen the return type if the class exists.
- use FastCSVReader;
+ // Keep in sync with the optional PECL FastCSV extension.
+ if (! class_exists(\FastCSVReader::class)) {
+ /** @internal Polyfill to avoid fatal errors when the extension is absent */
+ class FastCSVReader {}
+ }
+ use FastCSVReader;Alternatively, drop FastCSVReader from the interface and restrict it to the concrete implementation classes.
Failing to address this will break composer installs on any environment lacking the extension.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/Contracts/CsvReaderInterface.php around lines 5 to 6, the direct use
statement and return type hint for FastCSVReader cause fatal errors if the
FastCSV extension is not installed. To fix this, remove the use statement and
any references to FastCSVReader from the interface's public method signatures.
Instead, restrict FastCSVReader usage and return types to concrete
implementation classes that can check for the extension's existence, thereby
avoiding hard dependencies in the interface and preventing autoload failures.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
stubs/fastcsv.php (2)
1-2: Adddeclare(strict_types=1);to keep project-wide strictness consistent
Every source file insrc/andtests/declares strict types; stubs should do the same so IDE/static-analysis behaviour is uniform.<?php + +declare(strict_types=1);
27-37: Stub methods use empty bodies—prefer;prototypes to silence static-analysis “missing return” noise
For stubs you usually want signatures only. Using{}without areturnconflicts with the declared return types and trips PHPStan/Psalm. Replace bodies with semicolons. Example fix:- public function getDelimiter(): string {} + public function getDelimiter(): string; - public function setDelimiter(string $delimiter): FastCSVConfig { - return $this; - } + public function setDelimiter(string $delimiter): FastCSVConfig; - public function getEnclosure(): string {} + public function getEnclosure(): string; - public function setEnclosure(string $enclosure): FastCSVConfig { - return $this; - } + public function setEnclosure(string $enclosure): FastCSVConfig;Apply the same pattern to all other methods. This keeps the stub lightweight and analyzer-friendly.
Also applies to: 52-54, 69-71
tests/Writers/SplCsvWriterTest.php (2)
39-41: Use portable permission literal
0o777is a Python-style octal literal. Although PHP 8.1+ accepts it, older runtimes (≤ 8.0) will fatally error.
Unless the package explicitly targets 8.1+, prefer the traditional0777(or omit the mode and rely on the default umask).- mkdir(self::TEST_DATA_DIR, 0o777, true); + mkdir(self::TEST_DATA_DIR, 0777, true);
503-507: Performance threshold is fragileHard-coding
assertLessThan(5.0 …)makes the test susceptible to random CI slow-downs (shared runners, Windows, etc.).
Either raise the ceiling significantly or convert it to a warning (e.g. output elapsed time without asserting).- $this->assertLessThan(5.0, $elapsed, 'Writing 5000 records took too long'); + $this->assertLessThan(15.0, $elapsed, 'Performance test too strict for some CI environments');Or drop the assertion altogether and log
$elapsed.tests/Writers/CsvWriterTest.php (2)
39-41: Use traditional octal literal for mkdir permissionFor broader PHP version compatibility, switch
0o777to0777.- mkdir(self::TEST_DATA_DIR, 0o777, true); + mkdir(self::TEST_DATA_DIR, 0777, true);
128-130: Consider extracting common FS helpers to a base TestCase
setupTestDirectory(),cleanupTestFiles(), andcleanupTestDirectory()are duplicated in both writer test classes.
Moving them to a shared trait or abstract base class would DRY the code and make future maintenance easier.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
rector.php(1 hunks)src/Writers/SplCsvWriter.php(9 hunks)stubs(0 hunks)stubs/fastcsv.php(1 hunks)tests/Writers/CsvWriterTest.php(4 hunks)tests/Writers/SplCsvWriterTest.php(2 hunks)
💤 Files with no reviewable changes (1)
- stubs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Writers/SplCsvWriter.php
🧰 Additional context used
🧬 Code Graph Analysis (1)
stubs/fastcsv.php (1)
src/Writers/SplCsvWriter.php (2)
__construct(26-35)setConfig(263-268)
🔇 Additional comments (2)
rector.php (1)
13-16:stubsexclusion looks correct; consider mirroring in other tooling configs too
Adding the__DIR__.'/stubs'path to Rector’s skip list is appropriate, but remember that PHPStan/Psalm + ECS (if used) will still parse these files unless they are likewise excluded. Aligning the skip lists across tools avoids redundant scans and keeps analysis noise low.stubs/fastcsv.php (1)
214-246: Consider marking writer/reader stubs asfinalorinterfaceto match runtime behaviour
If the underlying FastCSV extension exposes final classes (common for C extensions), flagging them here prevents userland inheritance that will fatally error when the extension is actually loaded. Verify the real API and, if appropriate, addfinalor change to interfaces accordingly.
| class SplCsvWriterTest extends TestCase | ||
| { | ||
| private const string TEST_DATA_DIR = __DIR__.'/data'; | ||
| private const string TEST_DATA_DIR = __DIR__ . '/data'; | ||
|
|
||
| private const string TEST_CSV = self::TEST_DATA_DIR.'/test.csv'; | ||
|
|
||
| private const int PERFORMANCE_RECORDS = 1000; | ||
|
|
||
| private const float MAX_EXECUTION_TIME = 1.0; | ||
|
|
||
| private const int MAX_MEMORY_USAGE = 10 * 1024 * 1024; // 10MB | ||
|
|
||
| private CsvConfig $defaultConfig; | ||
| private string $testFile; |
There was a problem hiding this comment.
Invalid typed class-constant syntax will break the test suite
private const string TEST_DATA_DIR = … is not valid PHP (typed class constants are not supported as of PHP 8.3).
The file will fail to parse and the whole test run will abort before any assertions execute.
- private const string TEST_DATA_DIR = __DIR__ . '/data';
+ private const TEST_DATA_DIR = __DIR__ . '/data';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| class SplCsvWriterTest extends TestCase | |
| { | |
| private const string TEST_DATA_DIR = __DIR__.'/data'; | |
| private const string TEST_DATA_DIR = __DIR__ . '/data'; | |
| private const string TEST_CSV = self::TEST_DATA_DIR.'/test.csv'; | |
| private const int PERFORMANCE_RECORDS = 1000; | |
| private const float MAX_EXECUTION_TIME = 1.0; | |
| private const int MAX_MEMORY_USAGE = 10 * 1024 * 1024; // 10MB | |
| private CsvConfig $defaultConfig; | |
| private string $testFile; | |
| class SplCsvWriterTest extends TestCase | |
| { | |
| private const TEST_DATA_DIR = __DIR__ . '/data'; | |
| private string $testFile; |
🤖 Prompt for AI Agents
In tests/Writers/SplCsvWriterTest.php around lines 17 to 21, the class constant
TEST_DATA_DIR is declared with an invalid typed syntax 'private const string',
which is not supported in PHP 8.3 and causes a parse error. Remove the 'string'
type declaration from the constant definition, changing it to 'private const
TEST_DATA_DIR = __DIR__ . '/data';' to fix the syntax and allow the test suite
to run.
| class CsvWriterTest extends TestCase | ||
| { | ||
| private const string TEST_DATA_DIR = __DIR__.'/data'; | ||
| private const string TEST_DATA_DIR = __DIR__ . '/data'; | ||
|
|
||
| private const string TEST_OUTPUT_FILE = self::TEST_DATA_DIR.'/test_output.csv'; | ||
| private string $testFile; |
There was a problem hiding this comment.
Invalid typed class-constant syntax will break the test suite
Same issue as in SplCsvWriterTest: typed class constants aren’t allowed.
- private const string TEST_DATA_DIR = __DIR__ . '/data';
+ private const TEST_DATA_DIR = __DIR__ . '/data';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| class CsvWriterTest extends TestCase | |
| { | |
| private const string TEST_DATA_DIR = __DIR__.'/data'; | |
| private const string TEST_DATA_DIR = __DIR__ . '/data'; | |
| private const string TEST_OUTPUT_FILE = self::TEST_DATA_DIR.'/test_output.csv'; | |
| private string $testFile; | |
| class CsvWriterTest extends TestCase | |
| { | |
| private const TEST_DATA_DIR = __DIR__ . '/data'; | |
| private string $testFile; |
🤖 Prompt for AI Agents
In tests/Writers/CsvWriterTest.php around lines 17 to 21, the class constant
TEST_DATA_DIR is declared with a type (string), which is invalid syntax in PHP
and will break the test suite. Remove the type declaration from the constant
definition so it is declared simply as a constant without a type.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Chores