Problem/Motivation

From #3137883-91: Deprecate passing a StatementInterface object to Connection::query

\Drupal\Core\Database\Driver\pgsql\Upsert::execute() should return the last insert ID but atm it returns TRUE!

From the Upsert constructor...

 public function __construct(Connection $connection, $table, array $options = []) {
    $options['return'] = Database::RETURN_AFFECTED;
    parent::__construct($connection, $options);
    $this->table = $table;
  }

So all implementations should return the affected rows. This kinda makes sense because the last insert ID is useless - in order to an upsert you need to have all the IDs yourself anyway.

One thing that is tricky if we determine that we should fix it is that MySQL does something nonsensical - see https://dev.mysql.com/doc/c-api/8.0/en/mysql-affected-rows.html ... an updated row counts double???

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#5 3185400-5.patch4.26 KBalexpott
#5 3185400-5.test-only.patch3.56 KBalexpott
#4 3185400-4.patch4.32 KBalexpott
#4 3185400-4.test-only.patch3.56 KBalexpott

Issue fork drupal-3185400

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:

About issue forks

Comments

mondrake created an issue. See original summary.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Title: Drupal\Core\Database\Driver\pgsql\Upsert::execute returns a bool instead of las inert id » Drupal\Core\Database\Driver\pgsql\Upsert::execute returns a bool instead of last insert id
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Title: Drupal\Core\Database\Driver\pgsql\Upsert::execute returns a bool instead of last insert id » Test uposert return values and ensure that they are consistent regardless of database type
Issue summary: View changes
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Active » Needs review
StatusFileSize
new 3185400-4.test-only.patch3.56 KB
new 3185400-4.patch4.32 KB
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
StatusFileSize
new 3185400-5.test-only.patch3.56 KB
new 3185400-5.patch4.26 KB

Let's try that again with an updated codebase...

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Title: Test uposert return values and ensure that they are consistent regardless of database type » Test upsert return value and ensure that they are consistent regardless of database type
daffie’s picture

daffie commented
Status: Needs review » Needs work

The patch looks good to me. Just 2 minor nitpicks:

  1. +++ b/core/tests/Drupal/KernelTests/Core/Database/UpsertTest.php
@@ -75,7 +76,12 @@ public function testUpsertWithKeywords() {
+    $return_value = $upsert->execute();
...
+    $this->assertSame($this->connection->driver() === 'mysql' ? 3 : 2, $return_value);

    We are not using the variable $return_value anywhere else. Can we do it on a single line. like:

    $this->assertSame($this->connection->driver() === 'mysql' ? 3 : 2, $upsert->execute());

  2. +++ b/core/tests/Drupal/KernelTests/Core/Database/UpsertTest.php
@@ -83,8 +89,18 @@ public function testUpsertWithKeywords() {
+    $return_value = $upsert->execute();
+    $this->assertSame(1, $return_value);

    The same for these 2 lines.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs work » Needs review

I'm not sure about #7. I like having the bit where the upsert is executed separate from the assertion on the return value for two reasons...

  1. It get's buried in $this->assertSame($this->connection->driver() === 'mysql' ? 3 : 2, $upsert->execute());
  2. All the other asserts after the $upsert->execute() are about the effects of calling that code too - it's nicer to be able to see it without having to discern it from another assert.

Also this is a test so there's no benefit in not assigning a variable.

daffie’s picture

daffie commented
Status: Needs review » Needs work

The explanation of @alexpott in comment #8 is good enough for me.

The testbot is failing for MariaDB 10.2.7.

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

mondrake
Location 🇮🇹
commented

I think this was superceded by #3211866: Upsert::execute() return values are inconsistent, but not sure if there are some tests that would be worth having from the patch here

mondrake opened merge request !676

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs work » Needs review

This now just adds a new test for Upsert, after #3211866: Upsert::execute() return values are inconsistent.

mondrake’s picture

mondrake
Location 🇮🇹
commented
daffie’s picture

daffie commented
Component: postgresql db driver » database system
Category: Bug report » Task
Priority: Normal » Minor
Status: Needs review » Reviewed & tested by the community

This issue is no longer a bugfix, lets change it to a task.
A small improvement in testing.
It all looks good to me.

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Needs a rebase.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs reroll

Done, just pressed Rebase in Gitlab and there were no conflicts requiring manual rebase.

  • catch committed d8ab4a9 on 9.3.x 
    Issue #3185400 by mondrake, alexpott, daffie: Test upsert return value...

  • catch committed cdf89cb on 9.2.x 
    Issue #3185400 by mondrake, alexpott, daffie: Test upsert return value...
catch’s picture

catch
he/him
Primary language English
commented
Version: 9.3.x-dev » 9.2.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 9.3.x and cherry-picked to 9.2.x, thanks!

Status: Fixed » Closed (fixed)

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

mondrake closed merge request !676