Problem/Motivation
In Drupal 7, the equivalent of \Drupal\views\Plugin\views\query\Sql
had the $table_queue
member defined as public, so modules like CPS were able to alter the order of the tables in the query. This functionality was needed when a query alter had to add a new join and place it before an existing one, as can be seen in the CPS code here: http://cgit.drupalcode.org/cps/tree/includes/query.inc?h=7.x-1.x#n558
In Drupal 8, this was changed in #2183417: In Sql Rename Views properties to core standards and the new $tableQueue
property is now protected, which means that views query alters will not be able to work as before.
Proposed resolution
Change the \Drupal\views\Plugin\views\query\Sql::$tableQueue
property to have public access again.
Remaining tasks
Discuss, review.
User interface changes
Nope.
API changes
Not really.
Data model changes
Nope.
Comment | File | Size | Author |
---|---|---|---|
#16 | interdiff-16.txt | 738 bytes | amateescu |
#16 | 2929835-16.patch | 1.08 KB | amateescu |
#14 | 2929835-14.patch | 1.03 KB | amateescu |
#2 | 2929835.patch | 545 bytes | amateescu |
Comments
Comment #2
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedThis is the minimal change we can do to fix the regression.
Comment #3
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedComment #4
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedEven though the CPS module has been basically ported to D8 in the Workspace module (the 8.x-2.x branch), this is still technically a contrib project blocker.
Comment #5
timmillwoodI was thinking about if we should have methods to interface with
$tableQueue
, but seeing as$tables
,$relationships
,$where
,$having
,$orderBy
,$groupBy
,$fields
,$distinct
, and$tags
are all public, then it seems fine for$tableQueue
too.Comment #6
dawehnerHere is a question: Is there any way to define operations you would make on $tableQueue? Based upon that we could define an actual API.
Comment #7
larowlanYeah, I agree with #6, lets make an API here instead of making it public.
Making it public doesn't give us a chance to validate the format of the queue.
oh no, your stuff is borked
Lets add a setter if we need to change the value, and then we can at least assert that the new queue value is sane.
Comment #8
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedIt's very easy to define the operations that we need to do on
$tableQueue
: everything :PWe need to:
1) access all its data from the outside, to check if a table in the queue is an entity revision table
2) add a new table to the queue *before* another table
3) shuffle around the order of the tables
See http://cgit.drupalcode.org/workspace/tree/workspace.module?h=8.x-2.x#n272 and http://cgit.drupalcode.org/workspace/tree/workspace.module?h=8.x-2.x#n523
This issue was meant to be a quick-fix for #2784921-83: Add Workspaces experimental module, not a full blown API design issue for
\Drupal\views\Plugin\views\query\Sql::$tableQueue
.. so setting to NR to see if there's a chance to fix the regression quickly and maybe open a followup to design a proper API for this.Comment #9
catchGiven the other properties on this are public, making it public to be inline then opening an issue for a proper API seems OK to me - we might want to add methods for the other properties as well.
Comment #10
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedSo.. back to RTBC then? :)
Comment #11
larowlanCouldn't we just add a setter?
Yes, we have existing public properties, but I'm not sure that warrants adding more.
A lot of this code comes from PHP4 era.
Comment #12
amateescu CreditAttribution: amateescu for Pfizer, Inc. commented@larowlan, adding a setter could undermine the effort of adding a proper API for interacting with the table queue.
For example, some API methods could look like
queueTable()
andqueueTableBefore($table_name)
for allowing users to add a table to the queue before the one specified as a parameter. In these examples, there is no need to override the entire table queue, so maybe a generic setter would not be needed at all.Comment #13
dawehnerWhat about using a getter with a reference?
Comment #14
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedDiscussed a bit with @dawehner and I agree with #13 so let's use a getter with a reference, just like we do in the DB layer.
Comment #15
dawehnerThat's not really true. It is just needed, when you want to manipulate it, right?
Comment #16
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedRight!
Comment #17
catchTests still running on #16 but good compromise I think.
Comment #18
larowlanThe advantage to a setter is that we can deprecate it and add a trigger_error when we want to get rid of it in favour of a real API.
With the public property, once its public - we're stuck with it forever, with no way to communicate that it is going to go away.
With the getter by reference we can deprecate it and add a trigger_error, but not sure we can change it from being by reference to a normal getter at a later date. But we can deprecate it all together and add a different API.
So that's a happy compromise in my book.
Thanks for bearing with me on this, but I'm sure you agree, having an exit strategy is important with semver.
Comment #19
amateescu CreditAttribution: amateescu for Pfizer, Inc. commented@larowlan, that's a very good explanation of the problem (with semver), thanks for insisting on it :)
Comment #20
larowlanCrediting dawehner
Comment #22
larowlanCommitted 6930e1b and pushed to 8.5.x.
As per https://www.drupal.org/core/release-cycle-overview#current-development-c... 8.4 is in 'critical fixes only', so this can't be backported to 8.4