Problem/Motivation

Catchable fatal error: Method Drupal\Core\Database\Driver\mysql\Merge::__toString() must return a string value

That's caused by \Drupal\Core\Database\Query\Merge::__toString() that returns nothing

Steps to reproduce

Run phpstan without the baseline

Proposed resolution

Return an empty string instead of nothing

Remaining tasks

Address feedback in #29

User interface changes

N/A

API changes

\Drupal\Core\Database\Query\Merge::__toString() returns an empty string.

Data model changes

N/A

Release notes snippet

N/A

Issue fork drupal-2888978

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

J V created an issue. See original summary.

andypost’s picture

Version: 8.3.4 » 8.5.x-dev
Issue summary: View changes
Issue tags: -php7 +PHP 7.0 (duplicate)
amateescu’s picture

The last submitted patch, 3: 2888978-test-only.patch, failed testing. View results

amateescu’s picture

Title: Drupal\Core\Database\Query\Query::__toString() must return a string value as of PHP 7 » Drupal\Core\Database\Query\Query::__toString() must return a string value
Issue tags: -PHP 7.0 (duplicate)

Looking at the failures on various environments, this is not only a php 7 problem.

andypost’s picture

Version: 8.4.x-dev » 8.5.x-dev
Status: Needs review » Reviewed & tested by the community

Awesome! Missed upsert in initial check, I think that should go to 8.5 first

alexpott’s picture

Status: Reviewed & tested by the community » Needs review

Is this the right behaviour? Before the system produced an error and now it doesn't. Obviously one massive problem is that you can't trigger an exception in __toString(). But I'm not sure that this is an improvement. Ah... reading the issue summary does this only occur in PHP7? So this code was designed with PHP5 in mind. Looking at https://3v4l.org/dutJ8 PHP has been throwing errors for this since at least 5.2.0 so nope.

amateescu’s picture

Version: 8.5.x-dev » 8.4.x-dev
Status: Needs review » Reviewed & tested by the community

@alexpott:

Before the system produced an error and now it doesn't.

Isn't that the definition of a bug fix? :D

I don't really understand the concern, why would you want to trigger an exception instead of returning an empty string?

As for PHP 7, see #5. The bug was discovered on PHP 7 but, as you found out with the 3v4l snippet, it applies to 5.5 as well.

@andypost, the patch will surely be committed to 8.5.x first, but bug fixes are marked with the current version.

xjm’s picture

Status: Reviewed & tested by the community » Needs review

The test failures in https://www.drupal.org/pift-ci-job/792965 and https://www.drupal.org/pift-ci-job/792967 for the fixed patch look relevant...

amateescu’s picture

daffie’s picture

Status: Needs review » Needs work
+++ b/core/tests/Drupal/KernelTests/Core/Database/UpsertTest.php
@@ -53,4 +53,26 @@ public function testUpsert() {
+    if ($connection->driver() === 'pgsql' && version_compare($connection->version(), '9.5', '<')) {
+      $this->assertEmpty((string) $query);
+    }
+    else {
+      $this->assertNotEmpty((string) $query);
+    }

With all do respect. This is wrong. Everybody expects that the query will run against all by Drupal supported databases. And if there is something wrong with a query an exception should be thrown. If it is not possible to throw one, the query should not be supported by Drupal or Drupal should not support those database versions that cannot handle the query. In this case PostgreSQL versions 9.4 and older.

amateescu’s picture

Status: Needs work » Needs review

@daffie, the query runs just fine, what makes you think otherwise?

Only its string representation can't be derived because it is composed from multiple queries. It is exactly the same query that is tested in the test method above the one I added.

daffie’s picture

Status: Needs review » Needs work

@amateescu: You are right. I thought that the query was silently changed to an empty query that will do nothing.

The patch looks good. I have two minor points:

+++ b/core/tests/Drupal/KernelTests/Core/Database/MergeTest.php
@@ -230,4 +230,13 @@ public function testInvalidMerge() {
+  public function testMergeToString() {
+    $query = db_merge('test_people');
+
+    $this->assertEmpty((string) $query);
+  }

Can we change the db_merge query so that there is a difference between PostgreSQL 9.4 and other databases. See the added test for Upsert. And add some documentation why there is a difference. It is a bit confusing to me.

+++ b/core/tests/Drupal/KernelTests/Core/Database/UpsertTest.php
@@ -53,4 +53,26 @@ public function testUpsert() {
+    if ($connection->driver() === 'pgsql' && version_compare($connection->version(), '9.5', '<')) {
+      $this->assertEmpty((string) $query);
+    }
+    else {
+      $this->assertNotEmpty((string) $query);
+    }

Can we add some documentation on that there is a difference and how it is handled by Drupal.

alexpott’s picture

@amateescu I was having the same thoughts as @daffie - that the query was not going to work. The thing is though, we can get a string representation of multiple queries in a string. Just have to have multiple query strings joined together with a ; - no? However, looking at \Drupal\Core\Database\Driver\pgsql\Upsert::execute that does not seem a good route to go. Perhaps we should just update \Drupal\Core\Database\Query\Query::__toString() to document that in instances that the query cannot be represented as a single statement this will return an empty string.

+++ b/core/lib/Drupal/Core/Database/Query/Merge.php
@@ -338,15 +338,12 @@ public function key($field, $value = NULL) {
+    // In the degenerate case, there is no string-able query as this operation

I'm not sure what "degenerate" means here. I know this is not added by the patch but it certainly is not helping me understand what's going on.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

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

Drupal 8 is end-of-life as of November 17, 2021. There will not be further changes made to Drupal 8. Bugfixes are now made to the 9.3.x and higher branches only. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

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

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

Drupal 9.3.15 was released on June 1st, 2022 and is the final full bugfix release for the Drupal 9.3.x series. Drupal 9.3.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.4.x-dev branch from now on, and new development or disruptive changes should be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.9 was released on December 7, 2022 and is the final full bugfix release for the Drupal 9.4.x series. Drupal 9.4.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.5.x-dev branch from now on, and new development or disruptive changes should be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

mstrelan made their first commit to this issue’s fork.

mstrelan’s picture

Version: 9.5.x-dev » 10.1.x-dev
Issue tags: +Bug Smash Initiative

Moving to 10.1.x.

  1. +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Upsert.php
    @@ -77,7 +77,9 @@ public function execute() {
    -    // Nothing to do.
    +    // There is no string-able query as this operation executes multiple
    +    // queries.
    +    return '';
    

    This is no longer applicable.

  2. +++ b/core/tests/Drupal/KernelTests/Core/Database/UpsertTest.php
    @@ -53,4 +53,26 @@ public function testUpsert() {
    +    if ($connection->driver() === 'pgsql' && version_compare($connection->version(), '9.5', '<')) {
    +      $this->assertEmpty((string) $query);
    +    }
    

    Postgres 9.5 is no longer supported.

I'm not sure what "degenerate" means here. I know this is not added by the patch but it certainly is not helping me understand what's going on.

Can't help here, seems to be a mathematics term. See https://en.wikipedia.org/wiki/Degeneracy_(mathematics)

mstrelan’s picture

Rerolled for 10.1.x and updated the documentation of \Drupal\Core\Database\Query\Query::__toString() as mentioned in #14.

mondrake’s picture

Issue tags: +PHPStan-0
daffie’s picture

Status: Needs review » Reviewed & tested by the community

All code changes look good to me.
Testing has been added.
The PHP-Stan exception has been removed.
For me it is RTBC.

mondrake’s picture

Status: Reviewed & tested by the community » Needs work

NW for a helping hand to make contrib db-drivers' developers life easier :) see inline comment in the MR

mstrelan’s picture

Issue summary: View changes

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

quietone’s picture

I have moved the fix for the @return in Merge to #2888978: Drupal\Core\Database\Query\Query::__toString() must return a string value, which is a coding standards issue.