Skip to content
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

Merged
merged 7 commits into from
Nov 4, 2024
Merged

Conversation

lifinsky
Copy link
Contributor

@lifinsky lifinsky commented Nov 2, 2024

Description

  • Remove PHP 8.0 support
  • Prepare for PHPUnit 10 (Using PHPUnit 10 is currently blocked by Behat)
  • Add Doctrine DBAL support for Laravel 11 (Due to a conflict with PHPUnit, Laravel in current tests is used with version 10, which has built-in Doctrine support, which was removed in Laravel 11)
  • Update some dependencies

The deprecation warning in the DbalReconnectableConnectionFactory::reconnect() method remains unpleasant. I suggest rewriting it using Reflection:

 // Doctrine\DBAL\Connection (v3.9)
    public function connect()
    {
        Deprecation::triggerIfCalledFromOutside(
            'doctrine/dbal',
            'https://github.com/doctrine/dbal/issues/4966',
            'Public access to Connection::connect() is deprecated.',
        );

        if ($this->_conn !== null) {
            return false;
        }

        try {
            $this->_conn = $this->_driver->connect($this->params);
        } catch (Driver\Exception $e) {
            throw $this->convertException($e);
        }

        if ($this->autoCommit === false) {
            $this->beginTransaction();
        }

        if ($this->_eventManager->hasListeners(Events::postConnect)) {
            Deprecation::trigger(
                'doctrine/dbal',
                'https://github.com/doctrine/dbal/issues/5784',
                'Subscribing to %s events is deprecated. Implement a middleware instead.',
                Events::postConnect,
            );

            $eventArgs = new Event\ConnectionEventArgs($this);
            $this->_eventManager->dispatchEvent(Events::postConnect, $eventArgs);
        }

        return true;
    }

'sqlite' => 'SqliteDriver',
'sqlsrv' => 'SqlServerDriver',
};
$className = '\Ecotone\Laravel\Config\PDO\\' . $className;
Copy link
Member

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"

Copy link
Contributor Author

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

Copy link
Member

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' ]
Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member

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
Copy link
Contributor Author

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 = [])
Copy link
Contributor Author

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

@dgafka

Copy link
Member

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.

@lifinsky
Copy link
Contributor Author

lifinsky commented Nov 2, 2024

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.

dgafka
dgafka previously approved these changes Nov 2, 2024
use InvalidArgumentException;
use PDO;

trait ConnectsToDatabase
Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member

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

Copy link
Contributor Author

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.
 */

Copy link
Member

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dgafka done)

@dgafka dgafka merged commit 389c355 into ecotoneframework:main Nov 4, 2024
32 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants