Problem/Motivation

From #3174662: Encapsulate \PDOStatement instead of extending from it.

The Statement* classes have public properties like $dbh and $allowRowCount, historically to support tight integration with PDO.

With #3174662: Encapsulate \PDOStatement instead of extending from it, the PDO integration was relaxed, and we could revisit that in order to implement a more clean approach with protected properties and getters/setters.

Proposed resolution

move $dbh and $allowRowCount to the magic getter for BC with deprecation trigger, make an alternative set of protected properties for that, introduce getters (and setters if necessary), and use them across - will have to touch the other StatementInterface implementations

Remaining tasks

User interface changes

API changes

The method Drupal/Core/Database/Connection::prepareStatement() has a new parameter called $allow_row_count, which is a boolean value and default to FALSE. When set to TRUE allows the method Drupal\Core\Database\StatementInterface::rowCount() return the number of affected rows in the query.

Data model changes

Release notes snippet

Issue fork drupal-3177660

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mondrake created an issue. See original summary.

mondrake’s picture

Issue summary: View changes
Status: Active » Postponed
mondrake’s picture

Assigned: Unassigned » mondrake
Status: Postponed » Active

On this.

mondrake’s picture

andypost’s picture

Queued pgsql, because only mysql got mass fail

mondrake’s picture

#5 it's expected, only deprecation for now

mondrake’s picture

mondrake’s picture

Wonder if it's worth deprecating $dbh and $allowRowCount also in StatementEmpty and StatementPrefetch, and adding the new methods to StatementInterface

The last submitted patch, 7: 3177660-7.patch, failed testing. View results

andypost’s picture

+ $stmt->enableRowCount();
           return $stmt->rowCount()

Enable may return $this so this code could be simplified with chaining

OTOH it's not clear to me how this flag works

mondrake’s picture

#10: that flag was introduced in #2146733: Select queries should not use rowCount() to calculate number of rows...

Personally I find this toggle to be code babysitting, if one wants to use rowCount on selects and shoot their own foot, be it... and in any case the toggle is little protection, one could always enable it and so back to starting blocks.

andypost’s picture

I suggest to mark both methods @internal as this is a way to set custom flag on wrapper to allow getting number of affected rows (which has lots of bugs on different DBs)

+++ b/core/lib/Drupal/Core/Database/StatementEmpty.php
@@ -22,6 +22,38 @@ class StatementEmpty implements \Iterator, StatementInterface {
+   * Sets the statement to allow returning the row count.
+   *
+   * This method should normally be used only within database driver code.
+   */
+  public function enableRowCount(): void {

+++ b/core/lib/Drupal/Core/Database/StatementPrefetch.php
@@ -133,6 +133,38 @@ public function __construct(\PDO $pdo_connection, Connection $connection, $query
+  public function enableRowCount(): void {
...
+  public function isRowCountEnabled(): bool {

It's normally used to prevent consumer of trying to use rowCount on select-based queries

andypost’s picture

and that's only ugly way to pass this flag from connection (calling code) to statement wrapper - to flag the only case when row count allowed

mondrake’s picture

#13 if it’s really necessary we may consider to pass it in the constructor of the statement

andypost’s picture

As I see it done this way specifically to set it exactly after execution if requested by result type

mondrake’s picture

#15: I see, but why? We can pass $options['return'] to the StatementInterface object on construction, since we know what type of query we are doing at that moment... at least in today's Drupal. Largely revised patch, interdiff wouldn't help.

mondrake’s picture

Title: Remove public properties from StatementWrapper » Remove public properties from StatementInterface implementations
andypost’s picture

That looks much more cleaner but leads to code duplication in constructors

+++ b/core/lib/Drupal/Core/Database/StatementPrefetch.php
@@ -124,13 +124,61 @@ class StatementPrefetch implements \Iterator, StatementInterface {
+    if ($return_type === Database::RETURN_AFFECTED) {
+      $this->rowCountEnabled = TRUE;

+++ b/core/lib/Drupal/Core/Database/StatementWrapper.php
@@ -43,10 +43,15 @@ class StatementWrapper implements \IteratorAggregate, StatementInterface {
+    if ($return_type === Database::RETURN_AFFECTED) {
+      $this->rowCountEnabled = TRUE;

I think it was public to prevent change every constructor, no idea how to make it different way

Status: Needs review » Needs work

The last submitted patch, 16: 3177660-16.patch, failed testing. View results

mondrake’s picture

Status: Needs work » Needs review
mondrake’s picture

Assigned: mondrake » Unassigned
daffie’s picture

Status: Needs review » Needs work

Patch looks good!

  1. +++ b/core/lib/Drupal/Core/Database/StatementEmpty.php
    @@ -20,7 +20,48 @@ class StatementEmpty implements \Iterator, StatementInterface {
    +      @trigger_error("StatementEmpty::\${$name} should not be accessed in drupal:9.2.0 and will error in drupal:10.0.0. TODO. See https://www.drupal.org/node/TODO", E_USER_DEPRECATED);
    ...
    +      @trigger_error("StatementEmpty::\${$name} should not be written in drupal:9.2.0 and will error in drupal:10.0.0. TODO. See https://www.drupal.org/node/TODO", E_USER_DEPRECATED);
    
    +++ b/core/lib/Drupal/Core/Database/StatementPrefetch.php
    @@ -124,13 +124,61 @@ class StatementPrefetch implements \Iterator, StatementInterface {
    +      @trigger_error("StatementPrefetch::\${$name} should not be accessed in drupal:9.2.0 and will error in drupal:10.0.0. TODO. See https://www.drupal.org/node/TODO", E_USER_DEPRECATED);
    ...
    +      @trigger_error("StatementPrefetch::\${$name} should not be accessed in drupal:9.2.0 and will error in drupal:10.0.0. TODO. See https://www.drupal.org/node/TODO", E_USER_DEPRECATED);
    ...
    +      @trigger_error("StatementPrefetch::\${$name} should not be written in drupal:9.2.0 and will error in drupal:10.0.0. TODO. See https://www.drupal.org/node/TODO", E_USER_DEPRECATED);
    

    Why do we use the variable $name? We know what its value is!

  2. +++ b/core/lib/Drupal/Core/Database/StatementPrefetch.php
    @@ -124,13 +124,61 @@ class StatementPrefetch implements \Iterator, StatementInterface {
    
    -  public function __construct(\PDO $pdo_connection, Connection $connection, $query, array $driver_options = []) {
    +  public function __construct(\PDO $pdo_connection, Connection $connection, $query, array $driver_options = [], int $return_type = Database::RETURN_STATEMENT) {
    

    I think when we touch method with a patch and the method has no docblock we should add one.

  3. +++ b/core/tests/Drupal/KernelTests/Core/Database/StatementWrapperLegacyTest.php
    @@ -20,7 +21,7 @@ class StatementWrapperLegacyTest extends DatabaseTestBase {
    -    $this->statement = $this->connection->prepareStatement('SELECT * FROM {test}', []);
    +    $this->statement = $this->connection->prepareStatement('SELECT * FROM {test}', ['return' => Database::RETURN_STATEMENT]);
    

    Why do we do this change? AFAIK Database::RETURN_STATEMENT is the default value.

  4. +++ b/core/lib/Drupal/Core/Database/StatementEmpty.php
    @@ -20,7 +20,48 @@ class StatementEmpty implements \Iterator, StatementInterface {
    +   * @deprecated in drupal:9.1.0 and is removed from drupal:10.0.0. Access the
    
    +++ b/core/lib/Drupal/Core/Database/StatementPrefetch.php
    @@ -124,13 +124,61 @@ class StatementPrefetch implements \Iterator, StatementInterface {
    +   * @deprecated in drupal:9.1.0 and is removed from drupal:10.0.0. Access the
    

    9.1.0 should become 9.2.0

  5. We are missing a number of deprecation tests.
anmolgoyal74’s picture

Status: Needs work » Needs review
FileSize
15.07 KB
4.81 KB

Addressed 23.1, 23.2, 23.3, 23.4.

Status: Needs review » Needs work

The last submitted patch, 24: 3177660-24.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

anmolgoyal74’s picture

Status: Needs work » Needs review
FileSize
14.92 KB
1012 bytes

Status: Needs review » Needs work

The last submitted patch, 26: 3177660-26.patch, failed testing. View results

anmolgoyal74’s picture

Status: Needs work » Needs review
FileSize
14.08 KB
615 bytes
daffie’s picture

Status: Needs review » Needs work

All the changes in the patch from comment #28 look good. Only point #23.5: "We are missing a number of deprecation tests." still needs to be done.

mondrake’s picture

Assigned: Unassigned » mondrake

Yes, and some todos to fix. Working on this.

mondrake’s picture

mondrake’s picture

Assigned: mondrake » Unassigned
Status: Needs work » Needs review
Issue tags: +Needs change record
daffie’s picture

Status: Needs review » Needs work
  1.     // @todo in Drupal 10, this should be always set, remove this check.
        if (!isset($options['return'])) {
          $options['return'] = Database::RETURN_STATEMENT;
        }
    

    Could you explain why this check is necessary and why can it be removed in D10?

  2. For the deprecations in the classes StatementEmpty and StatementPrefetch, there is no testing and they are linked to https://www.drupal.org/node/TODO.

@mondrake: You are properly right that working with merge requests is going to be better then with patches. Only I do not know how to get my comment on the merge request to this issue queue.

mondrake’s picture

#34.2 we need to write a CR before completing that. The deprecation tests are there, now with the deprecation moved from annotation to method you can expect patterns instead of exact strings.

mondrake’s picture

Status: Needs work » Needs review
daffie’s picture

Status: Needs review » Needs work

The patch looks good, only my point #34.2 is still open.

mondrake’s picture

Status: Needs work » Needs review
mondrake’s picture

Assigned: Unassigned » mondrake
Status: Needs review » Needs work

Not reviewable atm.

mondrake’s picture

Assigned: mondrake » Unassigned
Status: Needs work » Needs review
daffie’s picture

Status: Needs review » Needs work

I like the current solution. For me it the way to fix this issue.

mondrake’s picture

Status: Needs work » Needs review
daffie’s picture

Quick review:

  1. Does the class core/lib/Drupal/Core/Config/DatabaseStorage.php need some changes as it used the constant Database::RETURN_AFFECTED 4 times?
  2. Are we going to deprecate the constant Database::RETURN_AFFECTED?
mondrake’s picture

mondrake’s picture

mondrake’s picture

Issue tags: -Needs change record

Added CR and adjusted the MR.

daffie’s picture

Status: Needs review » Needs work

Back to needs work.

mondrake’s picture

Status: Needs work » Needs review
alexpott’s picture

core/tests/Drupal/KernelTests/Core/Database/StatementWrapperLegacyTest.php is skipped for sqlite so I think we don't have any coverage of the new code in StatementPrefetch or StatementEmpty.

daffie’s picture

Status: Needs review » Needs work

Added a new comment on the MR.

mondrake’s picture

Status: Needs work » Needs review

Thanks.

mondrake’s picture

From discussion in Slack:

@daffie

@mondrake: AFAIK removing the $this->queryOptions['throw_exception'] ?? TRUE check will be BC break. Now queries have that check and with the current MR that check will be removed. When I do a code search for 'throw_exception' it is only used by the Oracle and AutoSlave database driver modules. See: http://grep.xnddx.ru/search?text=%27throw_exception%27&filename=. I think that the best options is to have those checks in the patch and create a followup issue to deprecate the option. I leave the decision to @alexpott.

@mondrake: In a lot of places in the MR we have code like:

      $message = $e->getMessage() . ": " . (string) $this . "; ";
      // Match all SQLSTATE 23xxx errors.
      if (substr($e->getCode(), -6, -3) == '23') {
        throw new IntegrityConstraintViolationException($message, $e->getCode(), $e);
      }
      throw new DatabaseExceptionWrapper($message, 0, $e);

Is it in your eyes a solution to replace that code with a call to the method Connection::handleQueryException()? In that way database drivers can easily override the default exception handling, like the current MySQL and SQLite driver do.

mondrake’s picture

removing the $this->queryOptions['throw_exception'] ?? TRUE check will be BC break

well in the MR we are no longer using Connection::query to execute the query, but the Statement directly. So per se we are taking a different route here, even if we had that option set it would be ineffective in here. And yes, we should deprecate it IMHO since in core it is not really doing anything at the moment.

Is it in your eyes a solution to replace that code with a call to the method Connection::handleQueryException()?

No. What I fancy is a 'ExceptionHandler' class in the db/driver namespace, that both Connection::query and operation::execute methods could invoke, using the ::getDriverClass mechanism. Then Connection::handleQueryException() could be deprecated. That will help normalizing exception management per driver.

mondrake’s picture

daffie’s picture

Issue summary: View changes
mondrake’s picture

Status: Needs review » Postponed
daffie’s picture

@mondrake: Why postening this issue? I am ready to give it a RTBC.

mondrake’s picture

#57 yes, sorry I should have explained. I think doing #3186934: Introduce an ExceptionHandler class in the database API, deprecate Connection::handleQueryException first would improve the patch here, by reducing the code duplication when dealing with exceptions.

mondrake’s picture

mondrake’s picture

mondrake’s picture

Status: Postponed » Needs review

Back to NR.

mondrake’s picture

Replied in MR.

mondrake’s picture

Rerolled

mondrake’s picture

Rerolled and removed changes to StatementEmpty, now deprecated, so no longer needed.

andypost’s picture

Filed 10.x issue for added TODOs - #3210310: Adjust Database API to remove deprecated Drupal 9 code in Drupal 10

Fixed test (StatementEmpty no longer affected by the patch)

Added full namespace to deprecations

daffie’s picture

Status: Needs review » Needs work

The patch looks good to me.
Just 1 minor nitpick.
After that one it is RTBC for me.

mondrake’s picture

Status: Needs work » Needs review
daffie’s picture

Status: Needs review » Reviewed & tested by the community

The patch looks good to me.
The 2 class variables are now protected.
There is a BC layer for the 2 class variables.
There are deprecation messages for the 2 class variables.
The Is and the CR look good.
For me it is RTBC.

@mondrake: Thanks.

andypost’s picture

Added strict comparison but have no idea hot to trigger sqlite testing on MR

mondrake’s picture

@andypost you need to wait a few minutes after pushing the commit to branch for the integration to check thta the branch is mergeable. Then, an ‘add test’ link appears and you can select test configs
to run as usual.

andypost’s picture

@mondrake thank you! now waiting for results...

mondrake’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll
andypost’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll

I used to rebase changes as solving merge conflics was too log

mondrake’s picture

Status: Needs review » Reviewed & tested by the community

thanks

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

mondrake’s picture

Version: 9.3.x-dev » 9.2.x-dev

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

A couple of things added to a review on gitlab...

mondrake’s picture

Status: Needs work » Needs review

Thanks @alexpott, replies in MR

mondrake’s picture

daffie’s picture

Status: Needs review » Needs work

The testbot is failing the MR.

mondrake’s picture

Status: Needs work » Needs review

I tend to prefer Upsert::execute changes at version 42, given latest findings - in reality now in HEAD we're returning the number of affected rows (which is returned by the Connection::query() call with Database::RETURN_AFFECTED 'return' option), even if the variable is misnamed. With the latest changes (removing the Database::RETURN_AFFECTED 'return' option from Upsert), we're returning the Statement instead.

Or we can postpone this issue on the hardening to be done in #3211866: Upsert::execute() return values are inconsistent.

andypost’s picture

Somehow pgsql and sqlite failed

daffie’s picture

Status: Needs review » Postponed

The current MR is (partly) fixing #3211866: Upsert::execute() return values are inconsistent. Therefore to me, we need to do the other issue first.

mondrake’s picture

Status: Postponed » Needs review
andypost’s picture

Only sqlite test failed

mondrake’s picture

Assigned: Unassigned » mondrake
Status: Needs review » Needs work

Something's broken with $allowRowCount in StatementPrefetch, on it.

mondrake’s picture

Assigned: mondrake » Unassigned
Status: Needs work » Needs review

Looks like you cannot really use rowCount() with StatementPrefetch when a SELECT is executed. In SQLite it would return 0, which is not the count of rows. So here changed the test to execute an UPDATE. It does not matter for the test itself.

andypost’s picture

@mondrake yes, that's why affected rows should be internal (implementation details) it applies only for non-select queries

I wondered that it returns number of results for Mysql and PG

daffie’s picture

Status: Needs review » Needs work

2 remarks for now. Will do another review tomorrow.

mondrake’s picture

Status: Needs work » Needs review

see commnets in MR

daffie’s picture

Status: Needs review » Needs work

Back to you @mondrake.

mondrake’s picture

Status: Needs work » Needs review

@daffie indeed, I should have noticed that :)

Fixed and also changed the code touched in SQLite's Connection:nextId to use square brackets syntax.

daffie’s picture

Status: Needs review » Needs work

@mondrake: Almost there. Just one fix is wrong.

mondrake’s picture

Status: Needs work » Needs review

Whoooops... thanks @daffie, fixed.

daffie’s picture

Status: Needs review » Reviewed & tested by the community

Now it all looks good to me.

mondrake’s picture

Status: Reviewed & tested by the community » Needs work
mondrake’s picture

Status: Needs work » Needs review

Last MR version tested with DruDbal experimental driver and BC now seems there.

daffie’s picture

Status: Needs review » Reviewed & tested by the community

All the changes look good to me.
We shall have to wait for D10 to remove those $options['return'] = Database::RETURN_AFFECTED;.
Back to RTBC.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

I moved the deprecations to 9.3.0 on commit because 9.2.0 is in alpha and we shouldn't be added new deprecations in that branch.

Committed 201212c and pushed to 9.3.x. Thanks!

  • alexpott committed 201212c on 9.3.x
    Issue #3177660 by mondrake, andypost, anmolgoyal74, daffie, alexpott:...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.