Problem/Motivation
We need a view for our content authors which displays content revisions that are filterable by moderation_state.
The (current) problem with this is that, per node, there could be several revisions with the same state (i.e "Needs Review"). We need a way to filter a view of content revisions to only the latest revision.
Proposed resolution
TBC, Either:
* A views_data hook
* Or a Filter plugin
Remaining tasks
Decide on approach.
Implement.
Test.
User interface changes
None.
API changes
TBC.
Data model changes
TBC.
Original report by acbramley
As we are beginning to implement workbench_moderation in our project we are coming across small things that are missing.
It would be good to be able to filter a content revision view by the current (i.e latest) revision.
Comment | File | Size | Author |
---|---|---|---|
#42 | 2674520-latest-revision-table.patch | 32.96 KB | Crell |
Comments
Comment #2
acbramley CreditAttribution: acbramley commentedComment #3
dawehnerAfaik you can totally achieve this:
Comment #4
acbramley CreditAttribution: acbramley commentedThe latest revision is not necessarily published when using workbench_moderation so that filter isn't going to work.
Also adding that relationship and those fields adds a join to node_field_data so you are getting the revision that's referenced in that table.
Comment #5
dawehnerWell, then move this issue to workbench_moderation, it is a concept they should solve.
Comment #6
acbramley CreditAttribution: acbramley commentedMoving.
EDIT: after thinking about it a bit I'm not entirely sure this is a workbench_moderation thing since revisions are core, but we'll see what the WBM people think.
Comment #7
Crell CreditAttribution: Crell at Palantir.net for Acquia commentedHere's a first stab at a latest-revision filter. It's not working yet, though.
Background: In Drupal 7, WBM maintained its own history table to track various and sundry things, which included a flag to indicate what revision was the latest revision. In D8, we don't have such a table so we have nowhere to store that flag. Sad panda.
In IRC, dawehner suggested using max(vid) and GROUP BY to produce the same result. Amazingly enough, that seems to work in my testing. The SQL query we want to generate is:
select nid, max(vid) as max_vid, max(revision_timestamp) as max_timestamp from node_revision group by nid
(Or equivalent.)
I do NOT know if this will scale performance-wise. I've only run it on a table with a dozen revisions, so anything will be fast. Further testing there is needed.
Trying to replicate that with Views, I got to the attached patch. (Hat tip to larwolan along the way.) However, although I am calling addGroupBy() on the query the GROUP BY clause is not showing up in the resulting query, and thus the query is invalid. I am unclear why this is, but my suspicion is that it relates to this excerpt from \Drupal\views\Plugin\views\query\SQL::query():
Is that perhaps a bug in Views? It does seem like hasAggregate should be true if there is a group by field. (The getOption() call is also returning false.) I will at this point ask for help from dawehner. Halp???
Comment #8
lokapujyaI didn't have time to test this ( I will later) , but it at least produces a valid query. I also turned aggregation on via the UI:
Comment #9
dawehnerYeah, no idea, without having properly time to look at it. This might be related with https://stackoverflow.com/questions/1225144/why-does-mysql-allow-group-b...
Comment #10
Crell CreditAttribution: Crell at Palantir.net for Acquia commentedlokapujya: That doesn't work for me either, whether I check "Aggregate" in the UI or not. The GROUP BY clause is still not added to the query string.
If I hack the Views code above to set hasAggregate if $this->groupby has a value, too, I get this error:
Which appears to be on the count query version of the View, which I think is only running to render the pager in the preview. The resulting query is
It seems to be grouping by everything, which seems odd. Also, what the heck is the node_field_revision table? :-)
dawehner: I'm pretty sure this is an issue on the Views end, as it's simply not adding a GROUP BY clause at all, which is guaranteed wrong anyway.
Comment #11
Crell CreditAttribution: Crell at Palantir.net for Acquia commentedLet's be honest, this is kind of a big deal...
Comment #12
lokapujyaCrell, did you make the changes to your addField() calls that I posted in #8? I don't think the max() function should be in the "group by". After doing that, I see the group by is added to the query and don't get the group by errors.
I am on mySQL 5.7 though. mySQL 5.7 became more strict so that you basically are not allowed to select a field that is not either in an aggregate function or in the group by.
After doing #8, from Views preview:
Comment #13
acbramley CreditAttribution: acbramley commentedThanks for picking this up @Crell!
node_field_revision seems to be where all the extra data about the revision is stored, I find it odd that revision_log is still in the node_revision table. I've opened #2674518: Add revision log field to views as well, it would be a whole lot easier if that field was in the node_field_revision table!
Comment #14
dawehner@Crell
The symptoms you describe in #7 basically read like #2674480: Views does not properly ignore the query cache during preview.
Comment #15
EclipseGc CreditAttribution: EclipseGc at Acquia commentedSo I worked on this a bit this morning to see if I could help move it along, interesting points of interest (which could be repetitively redundant).
1.) The query currently generated is essentially: https://gist.github.com/EclipseGc/f8d61f8eeaf043db9131
This query has max() calls in the group by which do not appear to work.
2.) Everything else in the query is mostly a unique identifier.
Title could be unique, vid definitely is, date almost certainly is, so all the results are going to appear, we need to group by nid
3.) The title and changed appear to be added based upon the fields in the view.
I'm not certain why this is. I suspected NodeRevision wizard to be the culprit but haven't found any evidence to support that suspicion. Whatever the case, the fields we add appear to be affecting the group by logic.
4.) I only know all this because I got the group to actually apply.
Deep in the bowels of \Drupal\views\Plugin\views\query\Sql::query() you'll find this line:
Unfortunately the query plugin's addGroupBy method does not update the display's options to reflect that we want to do a group_by, so I added a line to the latest revision handler provided by this patch that after running $query->addGroupBy("{$table}.nid"); we do a:
This passes the buck to the pager though. I simply forced a no-pager scenario to debug further, but we'll have to update the pager's query as well.
Hope this is helpful in some way.
Eclipse
Comment #16
Crell CreditAttribution: Crell at Palantir.net for Acquia commentedThanks, Kris.
After walking through this this morning with EclipseGc, and brainstorming the performance implications further, I decided to abandon the GROUP BY approach. I don't want to fight Views that badly, plus we decided the likely workarounds would run into performance issues at scale.
I am therefore now working on the backup plan, which is to maintain our own tracking table of what the latest revision is and expose that to Views, allowing us to just join against it like a normal person. Completely new patch coming when I've written it...
Comment #17
arknoll CreditAttribution: arknoll commented++ @crell
Comment #18
lahoosascoots CreditAttribution: lahoosascoots commentedHey! We really need this!
We are trying to add the Moderation State of the latest revision to the Content overview page. This functionality would give us the exact relationship we need to get that done.
Comment #19
Crell CreditAttribution: Crell at Palantir.net for Acquia commentedWork in progress. Still doesn't actually work. At the moment I'm hoping to avoid having to manually map the new table to every base table that exists and use direct query modification in the filter itself, but I may not be able to avoid that. (Anyone with more Views-fu, your recommendations are welcome.)
Comment #21
jibranThanks for the docs. :P
Comment #22
Crell CreditAttribution: Crell at Palantir.net for Acquia commentedNow you get a glimpse into how Crell develops for complex hooks... :-P
Comment #23
jibranJust for the record
hook_views_data
should go to*.views.inc
.Comment #24
Crell CreditAttribution: Crell at Palantir.net for Acquia commentedI expect to push it to a service anyway, so it shouldn't really matter at that point, should it?
Comment #25
jibranMoving it to service is fine but having a *.views.inc file in the modules just shows that there is a views integration by looking at file system that's why I'd prefer having it.
Comment #26
Crell CreditAttribution: Crell at Palantir.net for Acquia commentedOK, so as seen in the above snapshot of not-workingness, I'm unsure which of two annoying approaches to take. I'd love some input from someone who knows Views better than I do on the best way to do this.
Situation: I have a table that consists of entity type, entity id, revision id, language, and an int-boolean flag (latest). I want to add a filter to a "revision" view of any revisionable content type, such that when making a view of revisions only those with the "Latest" bit set are included. There's a single table for all entity types, since there seems to be no value-add to making a separate table. Now I need to build that filter. Options:
1) Do everything through hook_views_data(), and create "latest" as a virtual field in views. This will require setting up joins against every revision base table dynamically. Because the keys in every entity are different, this involves a ton of indirection and lookup up values from the entity definition in entity type manager, and I'm not entirely clear how to also build a join clause for "and this field is this literal value", because for, eg, nodes I want to join where entity_type="node" and all the other fields match up with the record in the base table. (And it's debatable if the "latest" column should be hard coded join, or if that should be exposed as a checkbox option for latest/is not latest. I cannot think of a case where you'd want is-not-latest, but that doesn't prove it doesn't exist.) Problem: Fugly massive array iteration and dynamic assignment, and I don't know how to do static value compound joins.
2) Skip the table definition and just modify the query directly in the plugin. This feels like it would be lighter weight and less code, and make the static parts easier. Or rather, would if I were dealing with vanilla DBTNG Select queries. As is, the best I can do is a long list of WHERE expressions that I hope come out right. Problem: Again, I'm not sure if my knowledge of the Views API is even remotely correct here, or if this kind of direct query manipulation is one of the things parents warn their children about when sending them off to school for the first time, along with not taking candy from strangers.
Someone from the Views world: which approach is less-bad, and any pointers on how to take that approach in the least-bad way? What API calls do I not know exist that I should be using? (The inline documentation is unfortunately thinner than I'd like.)
Comment #27
larowlanComment module has a similar conundrum, and does option 1
Comment #28
dawehnerRegarding the question of code vs.
hook_views_data()
I would use the following decision tree:a) If its sort of easy, use hook_views_data(), note I added #2699721: Add a domain object to easy defining hook_views_data which at least removes the argument of array handling.
b) Use a domain specific join plugin
c) Fallback onto filter plugin, if still needed.
This is a bad code design, rather you should do something what core does: lazy create the table when the actual query fails.
What about using
updateLatestRevision
which maybe makes it clearer that this does some DB operation as well.lol
Comment #29
Crell CreditAttribution: Crell at Palantir.net for Acquia commentedMarked as a duplicate of this issue: #2626166: Create a view for content revisions
Comment #30
jibranI think scope of #2626166: Create a view for content revisions and this issue are different. That one is about adding a view it is about adding a view handler.
Comment #31
acbramley CreditAttribution: acbramley commentedI agree with @jibran, #2626166: Create a view for content revisions by the looks of it is targetted at creating a view for the revisions page of a node, entirely separate issue to what I created this issue for. I am going to update the issue summary now since we have some traction.
Comment #32
acbramley CreditAttribution: acbramley commentedComment #33
Crell CreditAttribution: Crell at Palantir.net for Acquia commentedOK, with much help from dawehner and EclipseGc, here's what I think is a tested and working approach! :-)
It adds a filter for "is latest revision". There's no configuration or negation because I don't know why you'd want to do that, and it's harder to make that work. Add that filter to a node_revision or block_content_revision view and poof, no more older revisions.
Can I get an Amen, RTBC, or turkey sandwich?
Comment #34
jibranDocs missing.
It should be good if we are able to add something helpful here.
Is this really needed?
Space missing.
One enter two many.
I think entity_revision_test would be a better candidate for this view.
Comment #35
jibranShouldn't this be 'entity_id'? An entity can only have one latest revision.
Why not revision id?
Comment #36
Crell CreditAttribution: Crell at Palantir.net for Acquia commented1) Added
2) I don't think there is anything to add there; the title itself is sufficient to explain what its presence means in the UI.
3) Nope.
4) Fixed.
5) Fixed.
6) Eh, maybe. But the rest of the module's tests are done against nodes, and I never quite figured out what entity test entities were appropriate in what situation. That's maybe something to tweak another time.
#35: The table is really a simple key-value lookup. entity type, entity id, and langcode form the key, and never change. (Eg, "node:5:en".) The value is the revision ID that is the current latest revision. However, that latest revision is contextually local to the entity type and langcode, hence why all 3 keys are needed. This also allows the primary key index to be optimized, as it's those 3 keys that we will be looking up against. Also, having every column in a table be part of the primary key is, while legal, weird feeling. :-) (I don't know how else to describe that.)
Comment #37
acbramley CreditAttribution: acbramley commentedAwesome work @Crell and co, I'll give this a test and report back soon.
Comment #38
jibranMy issue with the table is that it can return more then one revision id for "node:5:en".
Comment #39
acbramley CreditAttribution: acbramley commentedVery happy to report the filter works well for me. Thank you so much for all your hard work everyone!
Comment #40
Crell CreditAttribution: Crell at Palantir.net for Acquia commentedjibran: How? node:5:en is the PKey, so there's no way for there to be more than one of that record.
Comment #41
jibranThanks @Crell for clearing my confusion.
Oh we are just updating the revision_id here. Sorry I read it wrong last time.
Comment #42
Crell CreditAttribution: Crell at Palantir.net for Acquia commentedYay!
I had to rebase (because I merged some other stuff this morning), so to be safe let's run this by testbot one last time. Will merge if it passes.
Comment #44
Crell CreditAttribution: Crell at Palantir.net for Acquia commentedAnd done. Go team!
Comment #46
joseph.olstadNice work above, this is needed for Drupal 8 core 'content_moderation'
if anyone has done this, please private message me, or add a link below with a link to the patch or to an open issue for this.
otherwise I may be forced to create one myself.