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
Any handlers using the DefaultSelection handler are able to either easily extend DefaultSelection and override entityQueryAlter() or alter the query themselves using hook_query_TAG_alter, like system module does to actually invoke entityQueryAlter().
However, when using the ViewsSelection handler these tags and metadata or not added to the view. This means that hook_query_TAG_alter and subsequently the entityQueryAlter() method on the interface itself are never called.
I was looking at using this functionality in OG, but hit a road block with this problem, as we cannot currently rely on using the SelectionInterface.
Proposed resolution
Make sure ViewsSelection behaves the same as the DefaultSelection handler, and SelectionInterface::alterEntityQuery() actually works.
The current problem is that the views query plugins are not capable of adding arbitrary metadata like our Query objects can. We should have a method for adding this that behaves like addTag. We should also move addTag() to the QueryPluginBase method instead of the Sql plugin so we can rely on it being there in a more generic way, regardless of the query backend.
We can then add the correct tag and metadata. So there will be parity with the DefaultSelection handler, and thus, peace will be restored.
Remaining tasks
User interface changes
N/A
API changes
New method on QueryPluginBase in views. Technically I think this is fine, as QueryPluginBase is typehinted in a few places, so anyone would have to extend this anyway.
Data model changes
| Comment | File | Size | Author |
|---|---|---|---|
| #2 | 2666202.patch | 3.27 KB | damiankloip |
Comments
Comment #2
damiankloip CreditAttribution: damiankloip commentedComment #3
damiankloip CreditAttribution: damiankloip commentedThis could go against views or base(ER) system I guess.
Comment #4
dawehnerComment #5
jibranviews it is.
Comment #6
jibranOnly tests are remaining here.
Comment #8
damiankloip CreditAttribution: damiankloip commentedI don't really agree this is a disruptive change at all.
Comment #9
damiankloip CreditAttribution: damiankloip commentedThis is technically a bug fix so could be in 8.1?
Comment #11
xjm@dawehner, @tim.plunkett, @alexpott, @cilefen, and I discussed this issue awhile back and agreed to handle it as a major task, since this issue can potentially harden Views against future access bypass issues. It should be targeted against the development minor since the fix includes internal API changes and public API additions. Thanks!