-
Notifications
You must be signed in to change notification settings - Fork 4
Refactor sql abstraction #46
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
base: 3.0.x
Are you sure you want to change the base?
Conversation
…tion # Conflicts: # test/integration/Platform/MysqlFixtureLoader.php # test/unit/Sql/Predicate/NotBetweenTest.php # test/unit/Sql/SelectTest.php # test/unit/TableGateway/Feature/MasterSlaveFeatureTest.php
@@ -61,7 +62,7 @@ public function __construct( | |||
|
|||
if (is_array($driver)) { | |||
$parameters = $driver; | |||
if ($profiler === null && isset($parameters['profiler'])) { | |||
if (!$profiler instanceof \Laminas\Db\Adapter\Profiler\ProfilerInterface && isset($parameters['profiler'])) { |
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.
Is this not a violation of the coding standard?
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.
In what way?
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.
Also... this is being rewritten anyway isn't 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.
Yes, so its going to be a conflict that needs resolved either way.
The coding standard requires use statements vs fully qualified paths iirc. I'm pretty sure that was a Rector change which is the only reason I was pointing it out.
*/ | ||
public function getLastGeneratedValue($name = null) | ||
#[Override] public function getLastGeneratedValue($name = null): string|null |
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.
Didn't we have a discussion last week or so about nullable return types being, for example, ?string
.
@@ -250,7 +243,7 @@ private function createErrorHandler() | |||
* @throws ErrorException if error is not within the error_reporting mask. | |||
*/ | |||
return function ($errno, $errstr, $errfile, $errline) { | |||
if (! (error_reporting() & $errno)) { | |||
if ((error_reporting() & $errno) === 0) { |
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.
Could a constant be used here, instead of 0?
) { | ||
$this->isBuffered = true; | ||
} | ||
} elseif ($resource instanceof mysqli || $resource instanceof mysqli_result |
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.
Would it be worth splitting the conditions across multiple lines so that they're that much easier to read?
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.
Just a few small things stood out for me, mate.
This is a major update to the Laminas\Db\Sql components, working specifically on the following refactoring:-
The new ExpressionPart allows a string specification (e.g. %s LIKE %s) as well as storing Argument values. This new storage container makes typing these values more robust and more flexible.
The ExpressionData container takes 1 or more ExpressionPart components and generates a space-delimited SQL string as required.
NOTE: Please ignore
Rector
files, the Rector requirement in composer.json and the temporary change in PHP to 8.3 - this is only for the convenience of using dev tools and will be reverted back to 8.1 in due course.