I will quote some intro from this issue https://www.drupal.org/project/drupal/issues/2744069
Views allows magic query arguments to be used and translates them when the query is run so you can cache the original query, but keep the functionality of adding in the current user's ID for example.
The problem is that if we use expressions inside such methods $query->addField()
or $query->addOrderBy()
we can't use placeholders for later substitutions from hook_views_query_substitutions()
.
Here are an examples:
// Custom views field based on Boolean.
public function query() {
$table_alias = $this->ensureMyTable();
$this->query->addField(
NULL,
"FIND_IN_SET(:langcode, $table_alias.langs) > 0",
'translation_status',
[
':langcode' => '***TRANSLATION_VIEWS_TARGET_LANG***',
]
);
}
or
// Custom views sorter based on Standard.
public function query() {
$table_alias = $this->ensureMyTable();
// Mysql FIND_IN_SET func will return position of element,
// when no element was found then it returns 0,
// so we use "> 0" as condition to filter out untranslated rows.
$this->query->addOrderBy(
NULL,
"FIND_IN_SET(:langcode, $table_alias.langs) > 0",
$this->options['order'],
'translation_status_sorter',
[
':langcode' => '***TRANSLATION_VIEWS_TARGET_LANG***',
]
);
}
NOTE: Also I find very strange to pass query parameters in first case using 'placeholders'
option and in the second one without it, I feel very confused)
Anyway the problem sits in the views_query_views_alter()
function, I will reference it here: https://cgit.drupalcode.org/drupal/tree/core/modules/views/views.module?h=8.6.x#n704
We check substitutions for joins, conditions, but not for the fields and orderBy (technically orderBy just adds expressions to the fields' list)
Proposed solution:
Add $expressions = &$query->getExpressions()
in the top of the function and add support for expressions traversing in the $tables's foreach.
Or refactor and move foreach into separate function and call twice: once - for this new function for tables, second - for exprssions.
Comment | File | Size | Author |
---|---|---|---|
#14 | interdiff-2944278-14.txt | 641 bytes | Sardis |
#14 | 2944278-14.patch | 2.1 KB | Sardis |
#9 | 2944278-9.patch | 7.51 KB | Manuel Garcia |
#9 | interdiff-2944278-2-9.txt | 5.41 KB | Manuel Garcia |
#4 | 2944278-4-test-only.patch | 5.41 KB | vlad.dancer |
Comments
Comment #2
vlad.dancerComment #3
joelpittetI've not run into needing this yet and can't comment on the solution but it will need tests to show it's a bug currently and so that it doesn't get broken in the future.
Comment #4
vlad.dancerAttached initial test.
Comment #6
vlad.dancerTest show us that the above functionality doesn't support yet. So we can move to the patch review of solution.
Comment #7
vlad.dancerComment #8
vlad.dancerComment #9
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commentedHere is a patch with the test from #4 in it, so as to verify the solution to the bug.
Comment #10
vlad.dancerThanks a lot @Manual for pushing it forward!
Comment #11
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commentedComment #12
Sardis CreditAttribution: Sardis at Drupalway, Drupal Ukraine Community for Drupal Ukraine Community, Drupalway commentedThanks for the good work! I've changed description of the second parameter of
_views_query_replace_substitutions
method. Also checked code for Coding Standard. And i have nothing against the actual approach used here. So +1 RTBC.Comment #13
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commentedThanks, I think this is almost ready, only thing I'd change:
We already declare the parameter to be an array on the previous line. Let's say instead something like "Substitution values to be used in the replacements."
Comment #14
Sardis CreditAttribution: Sardis at Drupalway, Drupal Ukraine Community for Drupal Ukraine Community, Drupalway commentedSorry for the delay. Okay, let's put it like you suggest.
Comment #15
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commentedThanks! Not sure if we're allowed to add a new function here, but I think this is ready for maintainer review at least.
Comment #16
LendudeThanks for working on this!
The test coverage is missing from the latest patch.
And sorry, but yuck! Underscore prefixed functions in the module file are very D7, lets not do that here. Lets make this a service or lets use \Drupal::classResolver() and just add this to a class (see #2949965: \Drupal::classResolver() could be more helpful for an example of use). And then move _views_query_tag_alter_condition there too (should be a quick follow up probably).
Comment #21
osopolarThere is a module to workaround this issue if you just need the expression for sort Views Sort Expression.
In my case I need to sort by a term length, so I added the Expression sort handler to the taxonomy term view and set the expression to CHAR_LENGTH(taxonomy_term_field_data.name).
Comment #25
larowlanComment #27
smustgrave CreditAttribution: smustgrave at Mobomo commentedTicket is a little old. Is the best bet still to create a service? Running into this issue now using regex in an orderby
Comment #28
wombatbuddy CreditAttribution: wombatbuddy commentedIt looks like we can't use placeholders inside addField() at all. The code from the example produced the following error message: "SQLSTATE[HY093]: Invalid parameter number: number of bound variables does not match number of tokens:...".