-
Notifications
You must be signed in to change notification settings - Fork 6
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
Fix/deprecations #27
Fix/deprecations #27
Conversation
Method "Doctrine\DBAL\Driver::connect()" might add "DriverConnection" as a native return type declaration in the future. Do the same in implementation "Drift\DBAL\Driver\Mysql\EmptyDoctrineMysqlDriver" now to avoid errors or add an explicit @return annotation to suppress this message. D:\BUAS\code\MSPChallenge-Server\vendor\symfony\error-handler\DebugClassLoader.php@325 Method "Doctrine\DBAL\Connection::quote()" might add "mixed" as a native return type declaration in the future. Do the same in child class "Drift\DBAL\Mock\MockedDBALConnection" now to avoid errors or add an explicit @return annotation to suppress this message. D:\BUAS\code\MSPChallenge-Server\vendor\symfony\error-handler\DebugClassLoader.php@325 Method "Doctrine\DBAL\Connection::lastInsertId()" might add "string|int|false" as a native return type declaration in the future. Do the same in child class "Drift\DBAL\Mock\MockedDBALConnection" now to avoid errors or add an explicit @return annotation to suppress this message. D:\BUAS\code\MSPChallenge-Server\vendor\symfony\error-handler\DebugClassLoader.php@325 Method "Doctrine\DBAL\Connection::beginTransaction()" might add "bool" as a native return type declaration in the future. Do the same in child class "Drift\DBAL\Mock\MockedDBALConnection" now to avoid errors or add an explicit @return annotation to suppress this message. D:\BUAS\code\MSPChallenge-Server\vendor\symfony\error-handler\DebugClassLoader.php@325 Method "Doctrine\DBAL\Connection::commit()" might add "bool" as a native return type declaration in the future. Do the same in child class "Drift\DBAL\Mock\MockedDBALConnection" now to avoid errors or add an explicit @return annotation to suppress this message. D:\BUAS\code\MSPChallenge-Server\vendor\symfony\error-handler\DebugClassLoader.php@325 Method "Doctrine\DBAL\Connection::rollBack()" might add "bool" as a native return type declaration in the future. Do the same in child class "Drift\DBAL\Mock\MockedDBALConnection" now to avoid errors or add an explicit @return annotation to suppress this message. D:\BUAS\code\MSPChallenge-Server\vendor\symfony\error-handler\DebugClassLoader.php@325 Method "Doctrine\DBAL\Driver::connect()" might add "DriverConnection" as a native return type declaration in the future. Do the same in implementation "Drift\DBAL\Mock\MockedDriver" now to avoid errors or add an explicit @return annotation to suppress this message. D:\BUAS\code\MSPChallenge-Server\vendor\symfony\error-handler\DebugClassLoader.php@325 Method "Doctrine\DBAL\Driver::getDatabasePlatform()" might add "AbstractPlatform" as a native return type declaration in the future. Do the same in implementation "Drift\DBAL\Mock\MockedDriver" now to avoid errors or add an explicit @return annotation to suppress this message. D:\BUAS\code\MSPChallenge-Server\vendor\symfony\error-handler\DebugClassLoader.php@325 The "Doctrine\DBAL\Driver\AbstractException" class is considered internal. It may change without further notice. You should not use it from "Drift\DBAL\Driver\Exception". D:\BUAS\code\MSPChallenge-Server\vendor\symfony\error-handler\DebugClassLoader.php@325
@mmoreram Another PR to review. 😅 |
|
||
/** | ||
* Class Exception. | ||
*/ | ||
class Exception extends AbstractException |
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.
@mmoreram please check the annotation given by AbstractException, it says:
* @internal
So it is not "expected" to be extended on...
So I just copied AbstractException's implementation
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.
That could introduce potential BC Breaks. I'm I right?
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 do not see how.
It is used in the files (Mysql|PostgreSQL|SQLite)Driver.php as:
use Drift\DBAL\Driver\Exception as DoctrineException;
$this->exceptionConverter->convert(new DoctrineException(...), ...)
and that ExceptionConverter is an interface accepting a Exception class:
interface ExceptionConverter
{
public function convert(Exception $exception, ?Query $query): DriverException;
}
So , it seems to work out?
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 convert
method requests a Doctrine\DBAL\Driver\Exception
instance as first parameter.
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.
We should check the potential BC Break, and if you want, delete the 7.4 PHP version and add the 8.1.
|
||
/** | ||
* Class Exception. | ||
*/ | ||
class Exception extends AbstractException |
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.
That could introduce potential BC Breaks. I'm I right?
*/ | ||
public function quote($input, $type = ParameterType::STRING) | ||
public function quote($input, $type = ParameterType::STRING)/*: mixed // <--- from php 8*/ |
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.
We can perfectly increase the PHP dependency version.
Even add 8.1 in the circleci raw.
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 for Drift, but no for us, since currently for MSP CHallenge we are at php 7.4. So if reactphp-dbal would require 8.1 this would be a breaking change for projects already using it, so a major change, so we should then release that a new major version?
So I suggest to keep it compatible with the current php , and do a seperate php 8 release later
fixes deprecations notices below. And one "internal" notice.