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
In Drupal\Core\Database\Query\Select constructor, $alias parameter has a null default value but next parameter Connection $connection is required.
public function __construct($table, $alias = NULL, Connection $connection, $options = array()) {
// ...
}
As $connection is required, $alias is always setted, even with NULL.
Proposed resolution
There is two ways to fix this:
- Remove the default value
- Switch $alias and $connection.
- Make the connection parameter the first parameter
2nd force to change all database drivers. As $connection is required, default value can safely be removed.
So proposed solution is 1/.
| Comment | File | Size | Author |
|---|---|---|---|
| #18 | select_query_builder-2799911-18.patch | 2.25 KB | sokru |
| #12 | select_query_builder-2799911-12.patch | 2.79 KB | GoZ |
Comments
Comment #2
GoZ CreditAttribution: GoZ at Centarro commentedComment #3
GoZ CreditAttribution: GoZ at Centarro commentedComment #4
daffie CreditAttribution: daffie commentedWe can remove the default value for the parameter $alias, because he parameter $alias must always be set. That is so because the third parameter must also be set. This is an API-change, but one that does not require any changes to D8 core or D8 contrib.
The other proposed solution is one we cannot do because it is an api change, that will require changes to D8 core and D8 contrib.
Comment #5
daffie CreditAttribution: daffie commentedI thought about it so more and the best solution is to make the \Drupal\Core\Database\Connection $connection parameter the first parameter. This would be an API change and we can only do that in a new major version. So moving this to D9.
@GoZ: If you do not agree, please speak up.
Comment #6
daffie CreditAttribution: daffie commentedUpdating title and summary.
Comment #7
catchConstructors are considered @internal, and none of the core database drivers override __construct, so this could potentially be done in a minor release.
Comment #8
GoZ CreditAttribution: GoZ at Centarro commentedComment #9
daffie CreditAttribution: daffie commented@GoZ: The problem that I have with your solution is that you will always have to set the value for $alias. Even when $alias has no value. The other thing is that constructors from classes in the namespace Drupal\Core\Database that need the connection parameter have it as their first parameter. Not always, I am sad to say. So for me the best solution would be number #3 and make the connection the first parameter.
Comment #10
GoZ CreditAttribution: GoZ at Centarro commented@daffie
You are right, and it's already the case since the following $connection argument is required.
There is no big issue to make #3, for core drivers. But in case custom drivers are made by people in projects, this will break. So can we assume in 8.3.x that a change can break existing project with custom drivers ?
Comment #11
daffie CreditAttribution: daffie commented@catch was very clear in comment #7:
So I do not see any problems for #3. We have the signoff from a Drupal core maintainer/committer.
Comment #12
GoZ CreditAttribution: GoZ at Centarro commentedHere is patch for solution 3
Comment #13
daffie CreditAttribution: daffie commentedLooks good to me.
Comment #14
alexpottDiscussed with @catch we agreed that the disruption vs benefit of this means we should wait to Drupal 9 for this. Code quality is not a good enough reason to break people's code in a minor release.
Comment #15
Crell CreditAttribution: Crell at Platform.sh commentedThis makes Select consistent with the other query builder objects, so I'm fine with it. Thanks.
Comment #17
catchNo longer applies.
Comment #18
sokru CreditAttribution: sokru as a volunteer commentedJust a reroll
Comment #19
daffie CreditAttribution: daffie commentedBack to RTBC.
Comment #20
catchThis needs a change record just in case it does affect a contrib driver somewhere.
Comment #21
daffie CreditAttribution: daffie commentedAdded a change record.
Comment #22
alexpottNote neither sqlsrv nor oracle will have to make a change - see:
I couldn't find any code that will have to change using that search.
Comment #23
alexpottCommitted 258f88a and pushed to 9.0.x. Thanks!