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.

CommentFileSizeAuthor
#16 interdiff-16.txt738 bytesamateescu
#16 2929835-16.patch1.08 KBamateescu
#14 2929835-14.patch1.03 KBamateescu
#2 2929835.patch545 bytesamateescu
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

amateescu created an issue. See original summary.

amateescu’s picture

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented
Status: Active » Needs review
FileSize
2929835.patch545 bytes

This is the minimal change we can do to fix the regression.

amateescu’s picture

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented
Issue summary: View changes
amateescu’s picture

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented
Issue tags: +Contributed project blocker

Even 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.

timmillwood’s picture

timmillwood
Primary language English
Location 🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK
CreditAttribution: timmillwood at Appnovation for Pfizer, Inc. commented
Status: Needs review » Reviewed & tested by the community

I 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.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner at Chapter Three commented

Here is a question: Is there any way to define operations you would make on $tableQueue? Based upon that we could define an actual API.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented
Status: Reviewed & tested by the community » Needs work

Yeah, 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.

$sql->tableQueue = new SomeRandomStuffThatBreaksThings();

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.

amateescu’s picture

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented
Status: Needs work » Needs review

It's very easy to define the operations that we need to do on $tableQueue: everything :P

We 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.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented

Given 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.

amateescu’s picture

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented
Status: Needs review » Reviewed & tested by the community

So.. back to RTBC then? :)

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented

Couldn'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.

amateescu’s picture

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() and queueTableBefore($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.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner at Chapter Three commented

What about using a getter with a reference?

amateescu’s picture

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented
FileSize
2929835-14.patch1.03 KB

Discussed 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.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner at Chapter Three commented 
+++ b/core/modules/views/src/Plugin/views/query/Sql.php
@@ -199,6 +199,26 @@ public function init(ViewExecutable $view, DisplayPluginBase $display, array &$o
+   * Note that this method must be called by reference as well:
+   *

That's not really true. It is just needed, when you want to manipulate it, right?

amateescu’s picture

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented
FileSize
2929835-16.patch1.08 KB
interdiff-16.txt738 bytes

Right!

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented

Tests still running on #16 but good compromise I think.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented

The 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.

amateescu’s picture

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented

@larowlan, that's a very good explanation of the problem (with semver), thanks for insisting on it :)

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented

Crediting dawehner

  • larowlan committed 6930e1b on 8.5.x 
    Issue #2929835 by amateescu, dawehner: [regression] Modules can no...
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented
Status: Reviewed & tested by the community » Fixed

Committed 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

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.