Conversation
|
""" WalkthroughThe codebase has been refactored to support PHP versions 8.1 through 8.4 by conditionally loading version-specific implementations of Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ExtendedPdo
participant ExtendedPdoPHP83
participant ExtendedPdoPHP84
User->>ExtendedPdo: instantiate or reference ExtendedPdo
alt PHP < 8.4.0
ExtendedPdo->>ExtendedPdoPHP83: require and delegate
else PHP >= 8.4.0
ExtendedPdo->>ExtendedPdoPHP84: require and delegate
end
ExtendedPdoPHP83-->>User: provide PDO functionality (no static connect)
ExtendedPdoPHP84-->>User: provide PDO functionality (with static connect)
Assessment against linked issues
Poem
✨ Finishing Touches
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. 🪧 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 (
|
francislavoie
force-pushed
the
php-83-support
branch
from
37c7e7b to
a73265c
Compare
|
Scrutinizer is complaining, but that's not new, looks like it has been failing for a while on the 6.x branch. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
phpunit.xml.dist (1)
2-3: Suggest documenting rationale for disabling coverage
Add an XML comment above the<coverage>element to explain thatprocessUncoveredFilesis disabled to avoid coverage errors for conditional, version-specific files. This will help future maintainers understand the intent.CHANGELOG.md (1)
3-5: Changelog entry accurately documents the changes.The version 6.1.0 entry correctly summarizes the reintroduction of PHP 8.1-8.3 support and importantly notes the limitation that
ExtendedPdo::connect()requires PHP 8.4+. Proper attribution and PR reference are included.Consider minor formatting improvements based on static analysis hints:
-- Reintroduce PHP 8.1 - 8.3 support; ExtendedPdo::connect() is not supported in those versions, and requires PHP 8.4+, by @francislavoie in https://github.com/auraphp/Aura.Sql/pull/245 +- Reintroduce PHP 8.1–8.3 support; ExtendedPdo::connect() is not supported in those versions, and requires PHP 8.4+, by @francislavoie in <https://github.com/auraphp/Aura.Sql/pull/245>🧰 Tools
🪛 LanguageTool
[typographical] ~5-~5: If you want to indicate numerical ranges or time ranges, consider using an en dash.
Context: ...ANGELOG ## 6.1.0 - Reintroduce PHP 8.1 - 8.3 support; ExtendedPdo::connect() is n...(DASH_RULE)
🪛 markdownlint-cli2 (0.17.2)
5-5: Bare URL used
null(MD034, no-bare-urls)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
.github/workflows/continuous-integration.yml(1 hunks)CHANGELOG.md(1 hunks)composer.json(1 hunks)phpunit.xml.dist(1 hunks)src/ExtendedPdo.php(1 hunks)src/ExtendedPdoPHP83.php(1 hunks)src/ExtendedPdoPHP84.php(1 hunks)src/PdoInterface.php(1 hunks)src/PdoInterfacePHP83.php(1 hunks)src/PdoInterfacePHP84.php(1 hunks)tests/DecoratedPdoTest.php(1 hunks)tests/ExtendedConnectPdoTest.php(1 hunks)tests/Parser/AbstractParserTest.php(3 hunks)tests/Parser/MysqlParserTest.php(1 hunks)tests/Parser/PgsqlParserTest.php(5 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
tests/DecoratedPdoTest.php (2)
src/Exception.php (1)
Exception(18-20)src/Exception/CannotDisconnect.php (1)
CannotDisconnect(21-23)
tests/Parser/MysqlParserTest.php (3)
tests/Parser/AbstractParserTest.php (1)
rebuild(10-14)src/Parser/AbstractParser.php (1)
rebuild(98-108)src/Parser/ParserInterface.php (1)
rebuild(33-33)
tests/Parser/PgsqlParserTest.php (4)
tests/Parser/AbstractParserTest.php (1)
rebuild(10-14)src/Parser/AbstractParser.php (1)
rebuild(98-108)src/Parser/ParserInterface.php (1)
rebuild(33-33)src/Parser/NullParser.php (1)
rebuild(32-35)
src/PdoInterfacePHP84.php (3)
src/PdoInterfacePHP83.php (14)
beginTransaction(32-32)commit(43-43)errorCode(51-51)errorInfo(60-60)exec(73-73)getAttribute(84-84)getAvailableDrivers(93-93)inTransaction(104-104)lastInsertId(118-118)prepare(133-133)query(150-150)quote(165-165)rollBack(176-176)setAttribute(189-189)src/ExtendedPdoPHP83.php (1)
connect(88-91)src/ExtendedPdoPHP84.php (1)
connect(88-95)
🪛 LanguageTool
CHANGELOG.md
[typographical] ~5-~5: If you want to indicate numerical ranges or time ranges, consider using an en dash.
Context: ...ANGELOG ## 6.1.0 - Reintroduce PHP 8.1 - 8.3 support; ExtendedPdo::connect() is n...
(DASH_RULE)
🪛 markdownlint-cli2 (0.17.2)
CHANGELOG.md
5-5: Bare URL used
null
(MD034, no-bare-urls)
🔇 Additional comments (24)
phpunit.xml.dist (1)
3-3: Approve disabling processUncoveredFiles for version-specific files
ChangingprocessUncoveredFilestofalseprevents PHPUnit from scanning version-specific implementations that would trigger class redeclaration errors, which directly addresses the PHP 8.1–8.3 support reintroduction.tests/Parser/MysqlParserTest.php (1)
15-16: Consistent SQL heredoc indentation intestBacktickString
The added spaces before theSELECTline and closingSQL;align the heredoc with the surrounding test code, improving readability and matching the style of other parser tests.Also applies to: 22-23
tests/Parser/PgsqlParserTest.php (4)
15-17: Align heredoc indentation intestUnicodeDoubleQuotedIdentifier
Indenting the SQL block by four spaces enhances visual consistency with other parser tests without altering test logic.
26-27: Uniform indentation intestCStyleStringConstantsheredocs
Both C-style escaping tests now use consistent indentation, improving readability while preserving the original SQL content and assertions.Also applies to: 32-34
67-68: Indentation adjustment intestTypeCasting
The heredoc for the type casting test is now indented to match the surrounding code, with no functional changes.
77-81: Consistent formatting intestArrayAccessorheredoc
The SQL block is indented uniformly, aligning with other tests and enhancing maintainability, without affecting behavior.tests/Parser/AbstractParserTest.php (3)
61-62: Indent heredocs intestDoubleQuotedIdentifier
Both SQL heredoc blocks are now indented by four spaces to align with method scope, matching the style used across parser tests.Also applies to: 67-68
77-78: Align SQL heredocs intestStringConstants
All three string-constant heredoc blocks have been uniformly indented, improving readability and consistency without altering assertions.Also applies to: 84-85, 90-93
102-109: Consistent indentation intestEscapedCharactersInStringConstants
The heredoc and nowdoc blocks—including the commented-out example—are now uniformly indented, aligning with project style guidelines and ensuring visual consistency.Also applies to: 113-122, 125-126, 131-132, 137-138
.github/workflows/continuous-integration.yml (1)
18-20: LGTM! CI matrix properly expanded for multi-version testing.The addition of PHP 8.1, 8.2, and 8.3 to the CI matrix correctly aligns with the expanded PHP version support and ensures compatibility testing across all supported versions.
composer.json (1)
25-25: PHP version requirement correctly broadened.The change from
"^8.4"to">=8.1"appropriately expands compatibility to support PHP 8.1 through 8.3+ as intended. This aligns with the conditional loading strategy for version-specific implementations mentioned in the PR objectives.tests/DecoratedPdoTest.php (1)
18-18: Critical syntax error fixed.Correctly changed
::CLASSto::class. The uppercase::CLASSis invalid PHP syntax and would cause a fatal error. The lowercase::classmagic constant is the proper way to reference the class name.tests/ExtendedConnectPdoTest.php (2)
7-13: LGTM! Well-implemented version check.The
setUp()method correctly checks for PHP version compatibility and provides a clear skip message. The parent method is properly called after the version check.
17-17: Clear and helpful documentation.The comment effectively explains the PHP version-specific behavior of the
connectmethod.src/PdoInterface.php (1)
11-15:Details
✅ Verification successful
Clean version-specific loading implementation.
The conditional loading approach using
PHP_VERSION_IDis appropriate for maintaining backward compatibility. Usingrequire_oncewith__DIR__ensures proper file inclusion.Verify that both version-specific interface files exist and maintain consistent API:
🏁 Script executed:
#!/bin/bash # Description: Verify the existence and consistency of version-specific PdoInterface files # Test 1: Check if both required files exist echo "Checking for version-specific interface files..." fd -t f "PdoInterfacePHP83.php|PdoInterfacePHP84.php" src/ # Test 2: Verify both files declare the PdoInterface in the same namespace echo -e "\nChecking interface declarations..." rg -A 2 "interface PdoInterface" src/PdoInterfacePHP83.php src/PdoInterfacePHP84.php # Test 3: Compare method signatures between the two interfaces (excluding connect method which is version-specific) echo -e "\nComparing common method signatures..." ast-grep --pattern 'public function $_($$$);' src/PdoInterfacePHP83.php | grep -v "connect" | sort > /tmp/php83_methods.txt ast-grep --pattern 'public function $_($$$);' src/PdoInterfacePHP84.php | grep -v "connect" | sort > /tmp/php84_methods.txt diff -u /tmp/php83_methods.txt /tmp/php84_methods.txt || echo "Differences found in method signatures"Length of output: 961
🏁 Script executed:
#!/bin/bash # Verify namespace declarations in both version-specific interface files echo "Checking namespace declarations in PdoInterface files..." rg -H '^namespace\s' src/PdoInterfacePHP83.php src/PdoInterfacePHP84.php || echo "No namespace declarations found"Length of output: 289
Approve version-specific PDO interface loading
The conditional check on
PHP_VERSION_IDand therequire_once __DIR__approach correctly handle backward compatibility without risking duplicate inclusions.Verified:
- Both
src/PdoInterfacePHP83.phpandsrc/PdoInterfacePHP84.phpexist- Both declare
namespace Aura\Sql;- Common method signatures are identical (aside from any version-specific methods)
No further action required.
src/ExtendedPdo.php (1)
11-15:Details
✅ Verification successful
Consistent version-specific loading pattern.
The implementation correctly mirrors the approach used in
PdoInterface.php, maintaining consistency across the codebase.Verify that both version-specific ExtendedPdo files exist and maintain consistent implementation:
🏁 Script executed:
#!/bin/bash # Description: Verify the existence and consistency of version-specific ExtendedPdo files # Test 1: Check if both required files exist echo "Checking for version-specific ExtendedPdo files..." fd -t f "ExtendedPdoPHP83.php|ExtendedPdoPHP84.php" src/ # Test 2: Verify both files declare the ExtendedPdo class in the same namespace echo -e "\nChecking class declarations..." rg -A 2 "class ExtendedPdo" src/ExtendedPdoPHP83.php src/ExtendedPdoPHP84.php # Test 3: Verify both classes extend AbstractExtendedPdo (as mentioned in the summary) echo -e "\nChecking inheritance..." ast-grep --pattern 'class ExtendedPdo extends $_' src/ExtendedPdoPHP83.php src/ExtendedPdoPHP84.php # Test 4: Verify both classes implement PdoInterface echo -e "\nChecking interface implementation..." rg "implements.*PdoInterface" src/ExtendedPdoPHP83.php src/ExtendedPdoPHP84.php # Test 5: Verify the connect method is only in PHP 8.4 version echo -e "\nChecking connect method presence..." echo "PHP 8.3 version (should not have connect or should throw exception):" rg -A 5 "function connect" src/ExtendedPdoPHP83.php || echo "No connect method found in PHP 8.3 version" echo -e "\nPHP 8.4 version (should have static connect method):" rg -A 5 "static.*function connect" src/ExtendedPdoPHP84.phpLength of output: 17070
ExtendedPdo version-specific loading: verified
All version-specific files exist and share a consistent API:
- src/ExtendedPdoPHP83.php and src/ExtendedPdoPHP84.php are present.
- Both declare
class ExtendedPdo extends AbstractExtendedPdo.- Constructor signature,
lazyConnect(),disconnect(),__debugInfo(), andgetPdo()implementations match.- PHP 8.3 version’s
connect()correctly throws the deprecation exception.- PHP 8.4 version provides the new
public static function connect()factory.No further changes needed.
src/PdoInterfacePHP83.php (1)
21-190: Excellent interface design for PHP 8.3 compatibility.The interface correctly defines all standard PDO methods without the static
connect()method, which is appropriate for PHP 8.3 and below. The use of union types, nullable types, and comprehensive documentation aligns well with modern PHP standards while maintaining backward compatibility.src/ExtendedPdoPHP83.php (4)
54-86: Constructor implementation is well-structured.The constructor properly handles default error mode, stores arguments for lazy connection, initializes profiler, sets up parser, and configures identifier quoting. The implementation is clean and follows good practices.
99-115: lazyConnect() implementation is correct.The lazy connection pattern is properly implemented with profiling, connection establishment only when needed, and execution of connection-time queries. The logic is sound and follows the expected lazy loading pattern.
138-149: Good security practice with __debugInfo().The method correctly masks sensitive username and password information while preserving other debugging information. This prevents credential leakage in stack traces and debug output.
88-91:Details
✅ Verification successful
Verify the connect() method deprecation strategy.
The
connect()method throws a RuntimeException directing users to uselazyConnect()instead. However, this creates an inconsistency with the interface expectation and may break existing code that expects this method to work.Let me verify how this connects with the overall architecture:
🏁 Script executed:
#!/bin/bash # Description: Check if there are references to the connect() method that might be affected # Expected: Find usage patterns and ensure the deprecation strategy is consistent # Search for connect() method calls in the codebase echo "=== Searching for connect() method calls ===" rg -A 3 -B 1 "->connect\(\)" --type php echo -e "\n=== Searching for connect method definitions ===" rg -A 5 "function connect\(" --type php echo -e "\n=== Check if there are tests that might be affected ===" rg -A 5 -B 2 "connect.*test|test.*connect" --type php -iLength of output: 4637
Deprecation strategy is consistent across PHP versions
I’ve verified that:
- The only instance
connect(): voidexists insrc/ExtendedPdoPHP83.php, which is only used on PHP 8.3 and earlier.- On PHP 8.4+,
connect()is a static factory method insrc/ExtendedPdoPHP84.phpandsrc/DecoratedPdo.php.- No codebase calls the deprecated instance method (
->connect()), and tests forconnect()are explicitly skipped on PHP < 8.4 (seetests/ExtendedConnectPdoTest.php).This confirms the exception-based deprecation in
ExtendedPdoPHP83is safe and won’t break existing users.src/ExtendedPdoPHP84.php (1)
54-167: Ensure consistency between PHP versions.The rest of the implementation (constructor, lazyConnect, disconnect, __debugInfo, getPdo) appears identical to the PHP 8.3 version, which is good for maintaining consistency. The only intended difference should be the static connect() method.
src/PdoInterfacePHP84.php (2)
64-64: Good use of SensitiveParameter attribute.The
#[\SensitiveParameter]attribute on the password parameter is an excellent security practice that will prevent the password from appearing in stack traces and debug output in PHP 8.2+.
32-212: Interface design is consistent and comprehensive.The interface properly declares all PDO methods with appropriate type hints and return types. The addition of the static
connect()method aligns with PHP 8.4+ capabilities while maintaining all existing functionality.
| public static function connect( | ||
| string $dsn, | ||
| ?string $username = null, | ||
| #[\SensitiveParameter] ?string $password = null, | ||
| ?array $options = null | ||
| ): static; |
There was a problem hiding this comment.
Fix parameter signature inconsistency and consider parameter completeness.
There are two issues with the static connect() method:
-
Type inconsistency: The interface declares
?array $options = nullbut the implementation inExtendedPdoPHP84.phpuses?array $options = []. This creates a contract violation. -
Incomplete parameter set: The interface only includes basic connection parameters but the constructor supports additional parameters like
$queriesand$profiler.
Apply this diff to fix the type inconsistency:
public static function connect(
string $dsn,
?string $username = null,
#[\SensitiveParameter] ?string $password = null,
- ?array $options = null
+ ?array $options = []
): static;Consider whether the interface should also include the $queries and $profiler parameters for completeness, or document the limitation that the static factory method only supports basic connection parameters.
📝 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.
| public static function connect( | |
| string $dsn, | |
| ?string $username = null, | |
| #[\SensitiveParameter] ?string $password = null, | |
| ?array $options = null | |
| ): static; | |
| public static function connect( | |
| string $dsn, | |
| ?string $username = null, | |
| #[\SensitiveParameter] ?string $password = null, | |
| ?array $options = [] | |
| ): static; |
🤖 Prompt for AI Agents
In src/PdoInterfacePHP84.php around lines 61 to 66, fix the parameter signature
inconsistency by changing the default value of the $options parameter from null
to an empty array to match the implementation in ExtendedPdoPHP84.php.
Additionally, review whether to extend the interface method signature to include
the $queries and $profiler parameters for completeness; if not, add
documentation clarifying that the static connect method only supports basic
connection parameters.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/PdoInterfacePHP84.php (1)
66-73:⚠️ Potential issueAddress parameter signature inconsistency identified in previous reviews.
The static
connect()method has a parameter signature inconsistency that was previously identified. The interface declares?array $options = nullbut implementations use?array $options = [].Apply this diff to fix the type inconsistency:
public static function connect( #[\SensitiveParameter] string $dsn, ?string $username = null, #[\SensitiveParameter] ?string $password = null, - ?array $options = null, + ?array $options = [], array $queries = [], ?ProfilerInterface $profiler = null ): static;
🧹 Nitpick comments (1)
CHANGELOG.md (1)
3-8: Comprehensive changelog documentation with minor formatting improvements needed.The changelog entry accurately documents all the key changes in version 6.1.0. However, there are some markdown formatting issues that should be addressed.
Apply this diff to fix the bare URL markdown formatting issues:
- Reintroduce PHP 8.1–8.3 support; `ExtendedPdo::connect()` is not supported in those versions, and requires PHP 8.4, by @francislavoie in https://github.com/auraphp/Aura.Sql/pull/245 - `ExtendedPdo::connect()` now supports `$queries` and `$profiler` like the constructor, by @francislavoie in https://github.com/auraphp/Aura.Sql/pull/245 - Mark all `$dsn` and `$password` parameters as `#[\SensitiveParameter]` to prevent leaking sensitive information in error messages, by @francislavoie in https://github.com/auraphp/Aura.Sql/pull/245 + Reintroduce PHP 8.1–8.3 support; `ExtendedPdo::connect()` is not supported in those versions, and requires PHP 8.4, by @francislavoie in [#245](https://github.com/auraphp/Aura.Sql/pull/245) + `ExtendedPdo::connect()` now supports `$queries` and `$profiler` like the constructor, by @francislavoie in [#245](https://github.com/auraphp/Aura.Sql/pull/245) + Mark all `$dsn` and `$password` parameters as `#[\SensitiveParameter]` to prevent leaking sensitive information in error messages, by @francislavoie in [#245](https://github.com/auraphp/Aura.Sql/pull/245)🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
5-5: Bare URL used
null(MD034, no-bare-urls)
6-6: Bare URL used
null(MD034, no-bare-urls)
7-7: Bare URL used
null(MD034, no-bare-urls)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
CHANGELOG.md(1 hunks)phpunit.xml.dist(1 hunks)src/DecoratedPdo.php(1 hunks)src/ExtendedPdoPHP83.php(1 hunks)src/ExtendedPdoPHP84.php(1 hunks)src/PdoInterfacePHP84.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- phpunit.xml.dist
- src/ExtendedPdoPHP83.php
- src/ExtendedPdoPHP84.php
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
CHANGELOG.md
5-5: Bare URL used
null
(MD034, no-bare-urls)
6-6: Bare URL used
null
(MD034, no-bare-urls)
7-7: Bare URL used
null
(MD034, no-bare-urls)
🔇 Additional comments (2)
src/DecoratedPdo.php (1)
48-50: Excellent security enhancement with SensitiveParameter attributes.The addition of
#[\SensitiveParameter]attributes to the$dsnand$passwordparameters is a valuable security improvement that prevents sensitive information from being exposed in stack traces or error logs. This aligns with PHP 8.2+ security best practices.src/PdoInterfacePHP84.php (1)
1-221: Well-designed interface with comprehensive PDO method coverage.The interface provides excellent coverage of PDO functionality with modern PHP 8.4 features:
- Proper use of union types and nullable types
- Appropriate SensitiveParameter attributes for security
- Comprehensive method signatures matching PDO expectations
- Good documentation for each method
|
Ah interesting, I broke something with trying to fix one of the complaints from the AI bot. Will fix. |
francislavoie
force-pushed
the
php-83-support
branch
from
8e8f3d7 to
f95e25d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
CHANGELOG.md (1)
3-7: Improve changelog readability and completeness
- Markdownlint flagged bare URLs (MD034) on lines 5–6; wrap them in inline links or angle brackets for compliance.
- Optionally, document the PHP version constraint update in
composer.jsonto reflect the widened support for PHP 8.1–8.3.--- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,8 @@ ## 6.1.0 -- Reintroduce PHP 8.1–8.3 support; `ExtendedPdo::connect()` is not supported in those versions, and requires PHP 8.4, by @francislavoie in https://github.com/auraphp/Aura.Sql/pull/245 -- Mark all `$dsn` and `$password` parameters as `#[\SensitiveParameter]` to prevent leaking sensitive information in error messages, by @francislavoie in https://github.com/auraphp/Aura.Sql/pull/245 +- Reintroduce PHP 8.1–8.3 support; `ExtendedPdo::connect()` is not supported in those versions (requires PHP 8.4), by @francislavoie in [#245](https://github.com/auraphp/Aura.Sql/pull/245) +- Mark all `$dsn` and `$password` parameters as `#[\SensitiveParameter]` to prevent leaking sensitive information in error messages, by @francislavoie in [#245](https://github.com/auraphp/Aura.Sql/pull/245) +- CHG: Update `composer.json` to require PHP >= 8.1, enabling support for PHP 8.1–8.3.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
5-5: Bare URL used
null(MD034, no-bare-urls)
6-6: Bare URL used
null(MD034, no-bare-urls)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
CHANGELOG.md(1 hunks)phpunit.xml.dist(1 hunks)src/DecoratedPdo.php(1 hunks)src/ExtendedPdoPHP83.php(1 hunks)src/ExtendedPdoPHP84.php(1 hunks)src/PdoInterfacePHP84.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- phpunit.xml.dist
- src/DecoratedPdo.php
- src/ExtendedPdoPHP83.php
- src/ExtendedPdoPHP84.php
- src/PdoInterfacePHP84.php
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
CHANGELOG.md
5-5: Bare URL used
null
(MD034, no-bare-urls)
6-6: Bare URL used
null
(MD034, no-bare-urls)
|
I don't know how well we can support this. The interface is changing based on php version and that is actually a breaking change according to semversion. That is why I have not replied very quickly. I am eager to hear from others what they think. |
|
It's not a breaking change because it's introduced to v6 only which already had PHP 8.1-8.3 support removed anyway. It's purely an addition of new functionality, which is not a break. Nothing changes at all for people on PHP 8.4+. |
|
I'm also hesitant about this approach for the same semver concerns @harikt mentioned. Additionally, the conditional file loading pattern adds complexity that hasn't been part of Aura.Sql's design philosophy. (Sorry for the delayed response.) |
|
This is not really "added complexity". This is a very common pattern which is used by Symfony polyfills for example which is used by almost everyone. But anyway, because I didn't want to wait anymore, I copied my fork into our project as a package replacement since we needed it right away. So I'm not blocked by this not being merged anymore. But it's still quite annoying that proper care wasn't taken to keep PHP version compatibility between Aura versions when it's not that difficult to keep. |
|
Thanks for understanding and for implementing your own solution. Good luck with your project! |
3 participants
Closes #238
Pretty straight forwards. Makes version-specific copies of
ExtendedPdoandPdoInterfaceso that they continue to work on older PHP versions.The
ExtendedConnectPdoTesttest is skipped since::connect()isn't a thing for earlier PHP versions.I had to disable the PHPUnit
processUncoveredFilescoverage option because it would scan these version-specific files and throwPHP Fatal error: Cannot declare class Aura\Sql\ExtendedPdo.I adjusted the heredocs in the tests to use the PHP 7.3+ indented heredoc syntax, it was just bothering me to look at 😅
The difference between the two files are as such:
In english:
PdoInterfacehas the::connect()method dropped entirely, andExtendedPdoadds a non-staticconnect(): voidwhich throws an exception warning that it can no longer be used as such in earlier PHP versions (was deprecated in v5 anyway).Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Style
Chores