feat: add support for network types inet, _inet, cidr, _cidr, macaddr, _macaddr
#310
Conversation
… `macaddr`, `_macaddr`
WalkthroughThis pull request introduces several PostgreSQL types: Changes
Sequence Diagram(s)sequenceDiagram
participant App as PHP Application
participant Converter as Type Converter
participant Validator as Validation Logic
participant Exception as Exception Handler
App->>Converter: convertToDatabaseValue(value)
alt Value is null
Converter-->>App: return null
else
Converter->>Validator: Check type & format
alt Value valid
Validator-->>Converter: Validated
Converter-->>App: return normalized value
else Value invalid
Validator->>Exception: Generate error details
Exception-->>App: throw specific exception
end
end
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (14)
src/MartinGeorgiev/Doctrine/DBAL/Types/MacaddrArray.php (1)
43-46: Consider providing context in throwInvalidItemExceptionThe
throwInvalidItemExceptionmethod is passing an empty string toforInvalidFormat(), which might lead to a less informative error message for users.protected function throwInvalidItemException(): never { - throw InvalidMacaddrArrayItemForPHPException::forInvalidFormat(''); + throw InvalidMacaddrArrayItemForPHPException::forInvalidFormat('invalid array item'); }src/MartinGeorgiev/Doctrine/DBAL/Types/CidrArray.php (1)
39-42: Consider providing context in throwInvalidItemExceptionSimilar to the
MacaddrArrayclass, thethrowInvalidItemExceptionmethod is passing an empty string toforInvalidFormat(), which might lead to a less informative error message.protected function throwInvalidItemException(): never { - throw InvalidCidrArrayItemForPHPException::forInvalidFormat(''); + throw InvalidCidrArrayItemForPHPException::forInvalidFormat('invalid array item'); }src/MartinGeorgiev/Doctrine/DBAL/Types/InetArray.php (1)
39-42: Consider providing more context in the empty string caseWhen throwing an invalid item exception with an empty string, consider providing more context in the error message to help with debugging.
protected function throwInvalidItemException(): never { - throw InvalidInetArrayItemForPHPException::forInvalidFormat(''); + throw InvalidInetArrayItemForPHPException::forInvalidFormat('empty or null item in array'); }src/MartinGeorgiev/Doctrine/DBAL/Types/Exceptions/InvalidInetForPHPException.php (1)
1-30: Well-designed exception handling for INET validationThe exception class follows best practices with factory methods, secure message formatting, and clear error messages.
If you plan to add more similar exception classes in the future, consider creating an abstract base exception class or a trait to reduce code duplication across these exception types, while maintaining their distinct semantic meanings.
src/MartinGeorgiev/Doctrine/DBAL/Types/Traits/MacaddrValidationTrait.php (1)
14-28: Consider adding dot notation validationThe
isValidMacAddressmethod correctly validates MAC addresses with colons, hyphens, or no separators. However, there's a minor inconsistency since thenormalizeFormatmethod removes dots (.) as delimiters, but the validation method doesn't check for dot notation (e.g.,0000.1111.2222).protected function isValidMacAddress(string $value): bool { // Check if it's using colons consistently if (\preg_match('/^([0-9A-Fa-f]{2}:){5}[0-9A-Fa-f]{2}$/', $value)) { return true; } // Check if it's using hyphens consistently if (\preg_match('/^([0-9A-Fa-f]{2}-){5}[0-9A-Fa-f]{2}$/', $value)) { return true; } + // Check if it's using dots consistently + if (\preg_match('/^([0-9A-Fa-f]{4}\.){2}[0-9A-Fa-f]{4}$/', $value)) { + return true; + } // Check for no separators return (bool) \preg_match('/^[0-9A-Fa-f]{12}$/', $value); }src/MartinGeorgiev/Doctrine/DBAL/Types/Traits/NetworkAddressValidationTrait.php (1)
14-21: Consider renaming for clarityThe method name
isValidPlainIpAddresscould be more intuitive as simplyisValidIpAddresssince it's checking whether a string is a valid IP address (either IPv4 or IPv6).tests/MartinGeorgiev/Doctrine/DBAL/Types/CidrArrayTest.php (1)
96-98: PHPDoc return type can be more specificThe PHPDoc for
provideInvalidTransformationsspecifies the return type asarray<string, mixed>, but it would be more accurate to specify it asarray<string, array{0: mixed}>orarray<string, array<int, mixed>>to match the actual structure returned./** - * @return array<string, mixed> + * @return array<string, array{0: mixed}> */tests/MartinGeorgiev/Doctrine/DBAL/Types/MacaddrTest.php (1)
56-58: Fix duplicate key in PHPDoc array typeThere's a duplicate key
postgresValueAfterNormalisationin the PHPDoc forprovideValidTransformations. The third parameter should be named differently to avoid confusion./** - * @return array<string, array{phpInput: string|null, postgresValueAfterNormalisation: string|null, postgresValueAfterNormalisation: string|null}> + * @return array<string, array{phpInput: string|null, postgresValueAfterNormalisation: string|null, phpValueAfterRetrievalFromDatabase: string|null}> */tests/MartinGeorgiev/Doctrine/DBAL/Types/MacaddrArrayTest.php (2)
96-99: Correct PHPDoc return typeThe PHPDoc for
provideInvalidTransformationsspecifies the return type asarray<string, mixed>, but it would be more accurate to match the actual implementation./** - * @return array<string, mixed> + * @return array<string, array{0: mixed}> */
100-109: Consider adding test for mixed MAC address formats in arrayThe test cases for invalid transformations are comprehensive, but you might want to add an additional test case for a mixed array containing both valid and invalid MAC addresses to verify that validation checks all elements in the array.
src/MartinGeorgiev/Doctrine/DBAL/Types/Macaddr.php (1)
43-58: Consider normalizing MAC addresses inconvertToPHPValuefor consistency.The
convertToPHPValuemethod validates the MAC address but doesn't normalize its format, whileconvertToDatabaseValuedoes normalize the format. This might lead to inconsistencies if PostgreSQL returns MAC addresses in various formats.public function convertToPHPValue($value, AbstractPlatform $platform): ?string { if ($value === null) { return null; } if (!\is_string($value)) { throw InvalidMacaddrForDatabaseException::forInvalidType($value); } if (!$this->isValidMacAddress($value)) { throw InvalidMacaddrForDatabaseException::forInvalidFormat($value); } - return $value; + return $this->normalizeFormat($value); }src/MartinGeorgiev/Doctrine/DBAL/Types/Exceptions/InvalidCidrArrayItemForPHPException.php (1)
16-19: Consider adding context to exception messages.The current implementation uses
var_exportto include the problematic value in the error message. While this is helpful, additional context like the expected format could make debugging easier.private static function create(string $message, mixed $value): self { - return new self(\sprintf($message, \var_export($value, true))); + return new self(\sprintf($message, \var_export($value, true)) . ' Expected format examples: 192.168.0.0/24, 2001:db8::/32'); }src/MartinGeorgiev/Doctrine/DBAL/Types/Exceptions/InvalidMacaddrArrayItemForPHPException.php (1)
16-19: Ensure sensitive data is sufficiently protected in exception messages.Using
\var_export($value, true)can be verbose and may expose internal details if$valuecontains sensitive data. Confirm that$valueis safe to log, or otherwise consider sanitizing or limiting the output in real-world scenarios.docs/AVAILABLE-TYPES.md (1)
14-19: Documentation entries look good.The new types are clearly listed, which helps developers quickly understand usage. Consider adding links or usage examples for each network type to aid discoverability.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
docs/AVAILABLE-TYPES.md(1 hunks)src/MartinGeorgiev/Doctrine/DBAL/Types/BaseArray.php(3 hunks)src/MartinGeorgiev/Doctrine/DBAL/Types/BaseNetworkTypeArray.php(1 hunks)src/MartinGeorgiev/Doctrine/DBAL/Types/Cidr.php(1 hunks)src/MartinGeorgiev/Doctrine/DBAL/Types/CidrArray.php(1 hunks)src/MartinGeorgiev/Doctrine/DBAL/Types/Exceptions/InvalidCidrArrayItemForPHPException.php(1 hunks)src/MartinGeorgiev/Doctrine/DBAL/Types/Exceptions/InvalidCidrForDatabaseException.php(1 hunks)src/MartinGeorgiev/Doctrine/DBAL/Types/Exceptions/InvalidCidrForPHPException.php(1 hunks)src/MartinGeorgiev/Doctrine/DBAL/Types/Exceptions/InvalidInetArrayItemForPHPException.php(1 hunks)src/MartinGeorgiev/Doctrine/DBAL/Types/Exceptions/InvalidInetForDatabaseException.php(1 hunks)src/MartinGeorgiev/Doctrine/DBAL/Types/Exceptions/InvalidInetForPHPException.php(1 hunks)src/MartinGeorgiev/Doctrine/DBAL/Types/Exceptions/InvalidMacaddrArrayItemForPHPException.php(1 hunks)src/MartinGeorgiev/Doctrine/DBAL/Types/Exceptions/InvalidMacaddrForDatabaseException.php(1 hunks)src/MartinGeorgiev/Doctrine/DBAL/Types/Exceptions/InvalidMacaddrForPHPException.php(1 hunks)src/MartinGeorgiev/Doctrine/DBAL/Types/Inet.php(1 hunks)src/MartinGeorgiev/Doctrine/DBAL/Types/InetArray.php(1 hunks)src/MartinGeorgiev/Doctrine/DBAL/Types/Macaddr.php(1 hunks)src/MartinGeorgiev/Doctrine/DBAL/Types/MacaddrArray.php(1 hunks)src/MartinGeorgiev/Doctrine/DBAL/Types/Traits/CidrValidationTrait.php(1 hunks)src/MartinGeorgiev/Doctrine/DBAL/Types/Traits/InetValidationTrait.php(1 hunks)src/MartinGeorgiev/Doctrine/DBAL/Types/Traits/MacaddrValidationTrait.php(1 hunks)src/MartinGeorgiev/Doctrine/DBAL/Types/Traits/NetworkAddressValidationTrait.php(1 hunks)tests/MartinGeorgiev/Doctrine/DBAL/Types/CidrArrayTest.php(1 hunks)tests/MartinGeorgiev/Doctrine/DBAL/Types/CidrTest.php(1 hunks)tests/MartinGeorgiev/Doctrine/DBAL/Types/InetArrayTest.php(1 hunks)tests/MartinGeorgiev/Doctrine/DBAL/Types/InetTest.php(1 hunks)tests/MartinGeorgiev/Doctrine/DBAL/Types/MacaddrArrayTest.php(1 hunks)tests/MartinGeorgiev/Doctrine/DBAL/Types/MacaddrTest.php(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: PHP 8.2 + Doctrine ORM 3.0 + Doctrine Lexer 3.0
- GitHub Check: PHP 8.4 + Doctrine ORM 2.14 + Doctrine Lexer latest
- GitHub Check: PHP 8.4 + Doctrine ORM latest + Doctrine Lexer latest
- GitHub Check: PHP 8.4 + Doctrine ORM 2.18 + Doctrine Lexer latest
- GitHub Check: PHP 8.3 + Doctrine ORM 2.14 + Doctrine Lexer latest
- GitHub Check: PHP 8.3 + Doctrine ORM latest + Doctrine Lexer latest
- GitHub Check: sloth
🔇 Additional comments (45)
src/MartinGeorgiev/Doctrine/DBAL/Types/MacaddrArray.php (2)
9-18: Well-implemented class with clear documentationThe class documentation effectively describes its purpose and includes a relevant link to PostgreSQL documentation. Good job extending
BaseNetworkTypeArrayto keep the implementation consistent with other network type arrays.
21-31: Solid MAC address validation implementationThe validation method correctly checks for both colon-separated and hyphen-separated MAC address formats, which aligns with PostgreSQL's requirements. The comments explaining the validation logic are helpful.
src/MartinGeorgiev/Doctrine/DBAL/Types/CidrArray.php (2)
10-18: Well-structured class with appropriate documentationThe class documentation is clear and includes relevant links to PostgreSQL documentation. Good job extending
BaseNetworkTypeArrayfor consistent implementation.
20-23: Good use of trait compositionUsing the
CidrValidationTraitfollows the DRY principle and promotes code reuse. This is a good approach to sharing validation logic across different classes.src/MartinGeorgiev/Doctrine/DBAL/Types/Traits/CidrValidationTrait.php (1)
12-20: Effective use of trait compositionThis trait follows single responsibility principle by focusing solely on CIDR validation. Good job using delegation to
isValidNetworkAddressWithCidrfrom theNetworkAddressValidationTrait, which promotes code reuse.src/MartinGeorgiev/Doctrine/DBAL/Types/Exceptions/InvalidInetArrayItemForPHPException.php (2)
14-19: Well-designed exception classThe exception class is well-structured and follows good practices by extending the appropriate base
ConversionExceptionclass. The privatecreatemethod efficiently formats error messages with detailed context usingvar_export.
21-34: Clear and specific exception factory methodsThe static factory methods provide descriptive, specific exceptions for different error cases. This approach improves code readability and makes error handling more maintainable.
src/MartinGeorgiev/Doctrine/DBAL/Types/InetArray.php (1)
1-43: Well-structured implementation for PostgreSQL INET[] data typeThe class correctly implements the handling of PostgreSQL's INET[] type, with good inheritance from a base class and proper use of the validation trait. The exception handling is thorough with specific methods for different error scenarios.
src/MartinGeorgiev/Doctrine/DBAL/Types/Traits/InetValidationTrait.php (1)
1-26: Clean implementation of INET validation logicThe trait provides a well-structured approach to validating INET addresses by checking both plain IP addresses and CIDR notation. The code is readable, well-commented, and follows good practices for code organization.
src/MartinGeorgiev/Doctrine/DBAL/Types/Exceptions/InvalidCidrForPHPException.php (1)
1-30: Well-designed exception handling for CIDR validationThe exception class follows best practices with:
- Appropriate extension of Doctrine's ConversionException
- Factory methods with descriptive names
- Secure message formatting using sprintf and var_export
- Detailed error messages that include the problematic value
src/MartinGeorgiev/Doctrine/DBAL/Types/Exceptions/InvalidInetForDatabaseException.php (1)
1-30: Well-structured exception class implementation.The
InvalidInetForDatabaseExceptionclass is well-designed with a private factory method and public static methods for specific error cases. The implementation follows good practices for exception handling in Doctrine DBAL.I notice this uses the
mixedtype hint, which requires PHP 8.0+. Ensure this requirement is documented in your project's README or composer.json if it's not already.src/MartinGeorgiev/Doctrine/DBAL/Types/Cidr.php (1)
1-59: Robust implementation of PostgreSQL CIDR type.The implementation correctly handles:
- Null values
- Type validation (ensuring strings)
- Format validation with appropriate exceptions
- Proper distinction between PHP and database error contexts
This follows a clean pattern and provides good error messages for troubleshooting conversion issues.
src/MartinGeorgiev/Doctrine/DBAL/Types/Exceptions/InvalidCidrForDatabaseException.php (1)
1-30: Clean exception class implementation for CIDR validation.The
InvalidCidrForDatabaseExceptionclass follows the same well-structured pattern as the INET exception class, maintaining consistency across the codebase.The error messages are clear and informative, which will be helpful for debugging conversion issues.
src/MartinGeorgiev/Doctrine/DBAL/Types/Inet.php (1)
1-59: Solid implementation of PostgreSQL INET type.The code correctly handles all expected conversion scenarios:
- Null value handling
- Type validation
- Format validation with clear error messages
- Proper separation of PHP and database validation contexts
The implementation follows the same pattern as the CIDR type, maintaining consistency across the library.
src/MartinGeorgiev/Doctrine/DBAL/Types/Exceptions/InvalidMacaddrForPHPException.php (1)
1-30: Well-structured exception class for MAC address validationThis class follows best practices with factory methods that have clear descriptive names, consistent error message formatting, and proper type declarations. The implementation pattern aligns well with Doctrine's error handling approach.
tests/MartinGeorgiev/Doctrine/DBAL/Types/CidrTest.php (1)
1-122: Comprehensive test coverage for CIDR typeThe test class provides thorough coverage with appropriate test cases for both valid and invalid inputs. It properly tests IPv4 and IPv6 formats with various netmask sizes, including edge cases (0 and max values).
src/MartinGeorgiev/Doctrine/DBAL/Types/Traits/MacaddrValidationTrait.php (1)
30-37: Consistent format normalizationThe normalization method effectively standardizes MAC addresses by removing all delimiters and reformatting with colons. This approach ensures consistent handling of MAC addresses regardless of input format.
src/MartinGeorgiev/Doctrine/DBAL/Types/Exceptions/InvalidMacaddrForDatabaseException.php (1)
1-30: Well-implemented exception for database conversionThis exception class mirrors the structure of
InvalidMacaddrForPHPExceptionwith clear error messages that distinguish database-to-PHP conversion issues. The consistent implementation pattern across these related exception classes makes the code more maintainable.src/MartinGeorgiev/Doctrine/DBAL/Types/Traits/NetworkAddressValidationTrait.php (2)
1-73: Well-structured trait for network address validationThe implementation of the
NetworkAddressValidationTraitprovides a clean and reusable set of validation methods for IP addresses and CIDR notation. The code is well-organized with logical separation of concerns and good use of PHP's built-in validation functions.
53-62: Efficient CIDR format validationGood use of string functions to validate the CIDR format before attempting to parse it. The check for numeric netmask is also important to prevent type errors in the subsequent validation steps.
tests/MartinGeorgiev/Doctrine/DBAL/Types/CidrArrayTest.php (2)
1-110: Comprehensive test class for CidrArray typeThe test class is well-structured and thoroughly tests the functionality of the
CidrArraytype, including valid transformations and error handling for various edge cases.
106-107: Good test cases for CIDR validationThe test cases for invalid netmasks are particularly important as they verify the validation logic handles boundary conditions correctly (netmask > 32 for IPv4 and > 128 for IPv6).
tests/MartinGeorgiev/Doctrine/DBAL/Types/MacaddrTest.php (2)
67-96: Thorough test cases for MAC address formatsExcellent test coverage for different MAC address formats (colon-separated, hyphen-separated, no separators) and case normalization. The expected normalization to lowercase and canonical format is well-tested.
116-126: Comprehensive invalid format testingThe test cases for invalid transformations cover a wide range of potential errors, which is crucial for robust validation of MAC addresses. Good job testing various edge cases like incorrect length, invalid characters, and wrong separators.
tests/MartinGeorgiev/Doctrine/DBAL/Types/MacaddrArrayTest.php (1)
74-81: Verify consistent MAC address normalizationThere appears to be an inconsistency in MAC address handling. In
MacaddrTest.php, hyphen-separated addresses are normalized to colon-separated format, but here they remain hyphenated. Additionally, theMacaddrTest.phpaccepts no-separator format as valid, while here it's tested as invalid.Please ensure consistent behavior between
MacaddrandMacaddrArraytypes.#!/bin/bash # Check how MAC addresses are normalized in both classes # Find the Macaddr class implementation echo "=== Macaddr class implementation ===" rg -A 10 "class Macaddr" --type php # Find the MacaddrArray class implementation echo "=== MacaddrArray class implementation ===" rg -A 10 "class MacaddrArray" --type php # Look for MAC address validation logic echo "=== MAC address validation logic ===" rg -A 5 "isValidMac" --type phptests/MartinGeorgiev/Doctrine/DBAL/Types/InetTest.php (1)
1-121: Well-structured and comprehensive test implementation.This test class is well-organized and follows good testing practices. It covers various IPv4 and IPv6 formats with appropriate data providers for both valid and invalid cases. The test methods are properly isolated, and the assertions are clear.
A few observations:
- Good test coverage including edge cases
- Proper setup of test fixtures
- Clear and meaningful test method names
- Comprehensive data providers with descriptive keys
src/MartinGeorgiev/Doctrine/DBAL/Types/Macaddr.php (1)
26-41: Confirm normalization behavior inconvertToDatabaseValuemethod.The method correctly normalizes MAC addresses using
normalizeFormat()before storing them in the database. This is good practice to ensure consistent data format.Consider verifying that the normalization doesn't alter valid MAC addresses unexpectedly. It would be helpful to understand what transformations
normalizeFormat()performs and ensure it matches PostgreSQL's expected format.#!/bin/bash # Check implementation of normalizeFormat method in the MacaddrValidationTrait rg -A 15 "normalizeFormat.*function" --type phptests/MartinGeorgiev/Doctrine/DBAL/Types/InetArrayTest.php (2)
113-124: Comprehensive test cases for invalid transformations.The invalid transformation test cases are thorough and cover a wide range of potential error conditions, including mixed valid and invalid entries. This is a good practice that helps ensure robust validation.
105-105: Investigate reason for PHPStan ignore directive.There's a PHPStan ignore directive on this line. This may indicate a type-checking issue that's being suppressed rather than properly resolved.
It would be good to understand why this ignore directive is necessary and whether it can be resolved in a more type-safe way.
#!/bin/bash # Run PHPStan on this file to see the actual error echo "Checking what PHPStan error is being suppressed..." rg --files-with-matches "phpstan" | grep -v "vendor" | head -n 3 rg -A 3 -B 3 "phpstan-ignore-line"src/MartinGeorgiev/Doctrine/DBAL/Types/Exceptions/InvalidCidrArrayItemForPHPException.php (1)
21-34: Good separation of exception factory methods.The separation of different error cases into distinct factory methods makes the code more maintainable and the error messages more specific. It's a good practice that improves readability and makes error handling more precise.
src/MartinGeorgiev/Doctrine/DBAL/Types/Exceptions/InvalidMacaddrArrayItemForPHPException.php (3)
21-24: Explicit exception creation method is handled well.The approach is consistent with custom exceptions for invalid types. This makes error handling clearer.
26-29: Consistent naming beneficial for exception clarity.The dedicated exception method (
forInvalidFormat) simplifies debugging by providing a clear, specific message for malformed MAC addresses.
31-34: Well-structured method for array-type violations.The
forInvalidArrayTypemethod provides a focused message for non-array values, improving diagnostic feedback to developers.src/MartinGeorgiev/Doctrine/DBAL/Types/BaseArray.php (8)
34-34: Clear separation of error handling.Throwing a dedicated exception improves maintainability and keeps the conversion logic concise.
39-39: Consistent invalid item check.Applying an exception method for invalid array items fosters uniform error messaging across the class.
48-55: Dedicated utility for invalid type exceptions is well-structured.Encapsulating the creation/throwing logic ensures better consistency and testability for error handling paths.
56-62: Unified approach for item-level errors.This method streamlines throwing item-level conversion exceptions, reinforcing consistent exception handling patterns.
64-72: Comprehensive handling of PostgreSQL type conversion issues.Throwing a
ConversionExceptionwith a clear message helps pinpoint mismatches between expected string types and actual types in database columns.
74-79:createInvalidTypeExceptionis neatly encapsulated.This method effectively separates message creation from the actual throw site. This improves maintainability of error messages in case of future refactors.
81-84:createInvalidItemExceptionfollows a similar pattern.Consistent usage of
ConversionExceptionfosters uniform error handling across array-based DBAL types.
116-117: Consistent invocation of typed-exception throwers.Using the dedicated
throwInvalidPostgresTypeExceptionkeeps the code base cohesive and less error-prone.src/MartinGeorgiev/Doctrine/DBAL/Types/BaseNetworkTypeArray.php (4)
21-32: Robust handling of null and string values for PostgreSQL conversion.Quoting valid strings and returning
'NULL'for null items is a clean way to map PHP values to PostgreSQL network types while preserving data integrity.
34-41: Validation ensures consistent input for database storage.Overriding
isValidArrayItemForDatabasehelps confirm correct data before insertion, reducing potential type mismatch errors.
43-61: Correct trimming and format checks for PHP conversion.Removing surrounding quotes and verifying network address format ensures data integrity when reading from PostgreSQL arrays.
63-72: Abstract methods clarify custom validation.Forcing subclasses to define
isValidNetworkAddressandthrowInvalidFormatExceptionfosters specialization and maintainable design for specific network types.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/MartinGeorgiev/Doctrine/DBAL/Types/CidrTest.php (1)
136-157: Consider adding more database-side invalid test cases.While you test for invalid type and format from database, you might want to consider adding more specific test cases for invalid database values, similar to the comprehensive list for PHP values. This could help maintain better test coverage and explain the coverage decrease mentioned in the PR comments.
public static function provideInvalidDatabaseValues(): array { return [ 'invalid type' => [123], 'invalid format from database' => ['invalid-cidr-format'], + 'empty string' => [''], + 'whitespace string' => [' '], + 'malformed CIDR with spaces' => ['192.168.0.0 / 24'], + 'CIDR without netmask' => ['192.168.0.0/'], + 'CIDR with non-numeric netmask' => ['192.168.0.0/xyz'], ]; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/MartinGeorgiev/Doctrine/DBAL/Types/Traits/NetworkAddressValidationTrait.php(1 hunks)tests/MartinGeorgiev/Doctrine/DBAL/Types/CidrArrayTest.php(1 hunks)tests/MartinGeorgiev/Doctrine/DBAL/Types/CidrTest.php(1 hunks)tests/MartinGeorgiev/Doctrine/DBAL/Types/InetArrayTest.php(1 hunks)tests/MartinGeorgiev/Doctrine/DBAL/Types/InetTest.php(1 hunks)tests/MartinGeorgiev/Doctrine/DBAL/Types/MacaddrArrayTest.php(1 hunks)tests/MartinGeorgiev/Doctrine/DBAL/Types/MacaddrTest.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- tests/MartinGeorgiev/Doctrine/DBAL/Types/CidrArrayTest.php
- tests/MartinGeorgiev/Doctrine/DBAL/Types/InetArrayTest.php
- src/MartinGeorgiev/Doctrine/DBAL/Types/Traits/NetworkAddressValidationTrait.php
- tests/MartinGeorgiev/Doctrine/DBAL/Types/MacaddrArrayTest.php
- tests/MartinGeorgiev/Doctrine/DBAL/Types/InetTest.php
- tests/MartinGeorgiev/Doctrine/DBAL/Types/MacaddrTest.php
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: PHP 8.1 + Doctrine ORM 2.14 + Doctrine Lexer 1.2
- GitHub Check: PHP 8.4 + Doctrine ORM latest + Doctrine Lexer 3.0
- GitHub Check: PHP 8.4 + Doctrine ORM 3.0 + Doctrine Lexer latest
- GitHub Check: PHP 8.4 + Doctrine ORM 2.14 + Doctrine Lexer latest
- GitHub Check: PHP 8.4 + Doctrine ORM 2.14 + Doctrine Lexer 2.1
- GitHub Check: PHP 8.4 + Doctrine ORM 2.18 + Doctrine Lexer 2.1
- GitHub Check: PHP 8.3 + Doctrine ORM 2.14 + Doctrine Lexer latest
- GitHub Check: PHP 8.3 + Doctrine ORM 2.14 + Doctrine Lexer 2.1
- GitHub Check: sloth
🔇 Additional comments (6)
tests/MartinGeorgiev/Doctrine/DBAL/Types/CidrTest.php (6)
1-13: Well-structured file header with appropriate imports.The file includes all necessary imports and uses strict types declaration, which is good practice for PHP 7.4+ code.
14-27: Test class setup looks good.The test class properly extends TestCase and has a clear setUp method that initializes the necessary dependencies. The type hint with PHPDoc annotation for the platform mock is also a good practice.
29-35: Basic name test is appropriate.This test ensures the type name is correctly defined as 'cidr', which is important for Doctrine type registration.
37-55: Transformation tests are well-structured.The bidirectional transformation tests use data providers effectively and follow good PHPUnit practices.
57-100: Comprehensive test data for valid transformations.The test data covers a good range of valid CIDR notations including:
- NULL values
- IPv4 with various netmasks
- IPv6 with various notations and netmasks
- Edge cases (min/max netmask values)
This provides good coverage for the type conversion functionality.
102-134: Thorough validation for PHP to database conversions.The test cases for invalid inputs are comprehensive, covering a wide range of potential error scenarios including empty strings, malformed inputs, invalid IP addresses, and invalid netmask values.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/MartinGeorgiev/Doctrine/DBAL/Types/BaseNetworkTypeArrayTest.php (2)
12-20: Consider adding a class docblock.Adding a class-level docblock would help document the purpose of this test class and its relationship to the
BaseNetworkTypeArrayabstract class you're testing.+/** + * Tests for the BaseNetworkTypeArray abstract class. + * This test validates the common functionality for all network address array types. + */ class BaseNetworkTypeArrayTest extends TestCase
1-121: Consider adding tests for additional edge cases.The test suite is missing tests for:
- An array containing invalid network addresses
- Handling malformed JSON in
convertToPHPValueThese additional tests would improve coverage and ensure the class handles error cases correctly.
Here's an example implementation for these additional tests:
/** * @test */ public function throws_exception_for_array_with_invalid_items(): void { $this->expectException(\InvalidArgumentException::class); $this->expectExceptionMessage('Invalid item'); $this->fixture->convertToDatabaseValue(['invalid_address'], $this->platform); } /** * @test */ public function throws_exception_for_malformed_json(): void { $this->expectException(\InvalidArgumentException::class); $this->expectExceptionMessage('Invalid format'); $this->fixture->convertToPHPValue('{malformed_json', $this->platform); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/MartinGeorgiev/Doctrine/DBAL/Types/BaseNetworkTypeArrayTest.php(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: PHP 8.3 + Doctrine ORM 2.18 + Doctrine Lexer 2.1
- GitHub Check: PHP 8.1 + Doctrine ORM 2.18 + Doctrine Lexer 2.1
- GitHub Check: sloth
🔇 Additional comments (6)
tests/MartinGeorgiev/Doctrine/DBAL/Types/BaseNetworkTypeArrayTest.php (6)
1-11: File structure and imports look good.The file correctly uses strict typing and includes all the necessary imports for the test case.
21-48: Good approach for testing an abstract class.The setup method effectively creates a mock platform and a concrete implementation of the abstract class for testing. The implementation of validation and exception methods is appropriate for the test scenarios.
49-56: Test methods are well-structured and named clearly.The test method for checking the type name is properly implemented with a clear assertion.
57-83: Database value conversion tests are comprehensive.The tests for converting null, empty array, and valid array to database values are well implemented and cover the basic scenarios.
84-94: Exception test is correctly implemented.The test for invalid type conversion properly verifies both the exception type and message.
95-121: PHP value conversion tests are comprehensive.The tests for converting null, empty JSON string, and valid JSON string to PHP values are well implemented.
Summary by CodeRabbit
New Features
Documentation
Refactor
Tests