Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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
Comment | File | Size | Author |
---|
Issue fork drupal-2888978
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:
Comments
Comment #2
andypostComment #3
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedThis is such a cute little bug :) Let's fix it!
I reviewed all the
__toString()
implementations in the db layer and the Upsert class for pgsql < 9.5 has the same problem.Comment #5
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedLooking at the failures on various environments, this is not only a php 7 problem.
Comment #6
andypostAwesome! Missed upsert in initial check, I think that should go to 8.5 first
Comment #7
alexpottIs 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.
Comment #8
amateescu CreditAttribution: amateescu for Pfizer, Inc. commented@alexpott:
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.
Comment #9
xjmThe 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...
Comment #10
amateescu CreditAttribution: amateescu for Pfizer, Inc. commented@xjm, yup, that's because we were trying to get the string representation of an invalid query :) Should be fixed with this small update.
Comment #11
daffie CreditAttribution: daffie commentedWith 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.
Comment #12
amateescu CreditAttribution: amateescu for Pfizer, Inc. commented@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.
Comment #13
daffie CreditAttribution: daffie commented@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:
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.
Can we add some documentation on that there is a difference and how it is handled by Drupal.
Comment #14
alexpott@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.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.
Comment #24
mstrelan CreditAttribution: mstrelan at PreviousNext commentedMoving to 10.1.x.
This is no longer applicable.
Postgres 9.5 is no longer supported.
Can't help here, seems to be a mathematics term. See https://en.wikipedia.org/wiki/Degeneracy_(mathematics)
Comment #26
mstrelan CreditAttribution: mstrelan at PreviousNext commentedRerolled for 10.1.x and updated the documentation of
\Drupal\Core\Database\Query\Query::__toString()
as mentioned in #14.Comment #27
mondrakeComment #28
daffie CreditAttribution: daffie at Finalist commentedAll code changes look good to me.
Testing has been added.
The PHP-Stan exception has been removed.
For me it is RTBC.
Comment #29
mondrakeNW for a helping hand to make contrib db-drivers' developers life easier :) see inline comment in the MR
Comment #30
mstrelan CreditAttribution: mstrelan at PreviousNext commentedComment #32
quietone CreditAttribution: quietone at PreviousNext commentedI 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.