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.
There is two ways to fix this:
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 commentedComment #3
goz commentedComment #4
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 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 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 commentedComment #9
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 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 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 commentedHere is patch for solution 3
Comment #13
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 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 commentedJust a reroll
Comment #19
daffie commentedBack to RTBC.
Comment #20
catchThis needs a change record just in case it does affect a contrib driver somewhere.
Comment #21
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!