-
-
Notifications
You must be signed in to change notification settings - Fork 16
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update dependencies #405
Update dependencies #405
Conversation
… parameter values
- Prepare for PHPUnit 10 - Add Doctrine DBAL support for Laravel 11 - Update some dependencies
'sqlite' => 'SqliteDriver', | ||
'sqlsrv' => 'SqlServerDriver', | ||
}; | ||
$className = '\Ecotone\Laravel\Config\PDO\\' . $className; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we instead of reimplementing Driver classes, make use of real ones?
E.g. if dbal packages is not installed by default in Laravel 11, then check for class existance here, and if not installed tell something like "Install doctrine/dbal to make use of this"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dgafka I dragged the implementation of adapters from Laravel 10. These classes were in the framework itself
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I see make sense then :)
@@ -15,7 +15,7 @@ jobs: | |||
strategy: | |||
matrix: | |||
operating-system: [ ubuntu-latest ] | |||
php-versions: [ '8.0', '8.3' ] | |||
php-versions: [ '8.1', '8.3' ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know PHP 8.0 is not longer supported, but I know people are still using that.
Is there a specific reason to drop it now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does not allow us to avoid deprecating and testing with the latest versions of the Laravel and Symfony frameworks. What I think is more commercially important. Upgrade from 8.0 to 8.1 simple, and current quickstart examples depend on PHP 8.2. For example readonly classes. But I think it is necessary to bump the version gradually.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, upgrading from 7.x to 8.x could be pain, 8.0 to 8.1 isn't so hard thought
use PDOException; | ||
use PDOStatement; | ||
|
||
class Connection implements ServerInfoAwareConnection |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dgafka Main connection wrapper for DBAL based on PDO connection
* | ||
* @throws \InvalidArgumentException | ||
*/ | ||
public function connect(array $params, $username = null, $password = null, array $driverOptions = []) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/doctrine/dbal/blob/3.9.x/src/Driver/PDO/PgSQL/Driver.php
The current drivers are final classes and they are designed to instantiate a connection, not to use existing pdo instance
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oki, make sense we should be reusing connection, in order to ensure transactional boundaries.
If we want to continue supporting Laravel, we will have to take on the support of the pdo adapter in the current connection class in Laravel. To be honest, since version Ecoton 2, I have abandoned Laravel support. Perhaps this package should be separated from the base Ecotone packages in ecotone-dev. |
use InvalidArgumentException; | ||
use PDO; | ||
|
||
trait ConnectsToDatabase |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/**
* licence Apache-2.0
*/
Can you add lincensing to new files?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I can. I wanted to clarify what to do with the fact that, in fact, this is code from Laravel, and there is no copyright in their files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, for MIT we are obligated to carry on licence terms for those files.
So there should be readme in that catalog, that classes from this namespace comes from https://github.com/laravel/laravel and were licenced under MIT
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dgafka what about this?:
/**
* This file is a modified version of a class from the Laravel framework.
*
* Laravel is licensed under the MIT License.
* Original authors: Taylor Otwell and the Laravel contributors.
*
* @license MIT (https://opensource.org/licenses/MIT)
*
* Modifications were made as part of the Ecotone framework under the Apache 2.0 License.
* See LICENSE file for the Apache 2.0 License details.
*/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep make sense, we can do it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dgafka done)
Description
The deprecation warning in the DbalReconnectableConnectionFactory::reconnect() method remains unpleasant. I suggest rewriting it using Reflection: