To reproduce:

* Publish node 123 tagged with taxonomy term 'example'
* Visit the taxonomy term view page for 'example'
* Note that the view contains node 123
* Save a new draft for node 123
* Revisit the 'example' page and it no longer contains node 123, even though node 123 still has a published revision

Under the hood, I see that saving a draft removes node 123 from the taxonomy_index table. Happy to help there's a good way to chip in.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

eeeschwartz created an issue. See original summary.

eeeschwartz’s picture

Title: Creating a draft removes published node from the taxonomy_index » Creating a draft removes published node from taxonomy view
Crell’s picture

Project: Workbench Moderation » Drupal core
Version: 8.x-1.0 » 8.1.x-dev
Component: Code » taxonomy.module

Hm. This could be a core bug; it shouldn't be removing items if the default revision is published. Basically, the code that manages that table is not forward-revision aware.

Reassigning to core, where I believe the bug needs to be fixed.

jibran’s picture

Issue tags: +VDC

How can we reproduce this in core?

Crell’s picture

I doubt it could be replicated exclusively with core, because nothing in core creates forward revisions. With core's facilities, default revision == most recent revision is always true. That means this situation could not be created. It's only when using modules such as Workbench Moderation, which create forward revisions, that such issues would be noticed.

dawehner’s picture

Here is a test, I just wanted to see what is going on, and yeah this is totally reproducible.

Crell’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 6: 2708735-5.patch, failed testing.

unstatu’s picture

Assigned: Unassigned » unstatu
Issue tags: +DrupalCampES
unstatu’s picture

Assigned: unstatu » Unassigned

Dawehner told me in the DrupalCampES that the Core API implements the forward revisions, but not the UI for handling them.

Modules like Workbench Moderation provide the needed UI.

I have done some research and I think the problem is in the Context Filter defined in the Taxonomy Term view (admin/structure/views/view/taxonomy_term).

eeeschwartz’s picture

@unstatu I wouldn't _think_ the issue would be at the view level.

As far as I can tell the view is behaving correctly: it looks at taxonomy_index for published nodes related to a given tid. The problem seems to be that saving a draft removes the published nid from the taxonomy_index table.

catch’s picture

Priority: Normal » Major
Issue tags: +Needs tests, +Workflow Initiative

This is a difference between the forward revision approach of drafty (which was done only out of necessity) vs. the one in core.

Drafty first saves the draft revision, then re-saves the live revision - resulting in all hook_entity_*() hooks being fired again, so that any tables which aren't revision-aware get restored to their pre-draft/published state. This means that a revision-unaware hook like taxonomy_node_update() in 7.x with drafty doesn't have to do anything special (and it can't even if we wanted it to due to lack of forward revision support in core).

Core 8.x saves the new revision without touching the live revision at all, but then taxonomy_node_update() needs to 1. know it's getting a draft revision, not a live one 2. return early.

Short version of that:

taxonomy_node_update() should check $entity->isDefaultRevision(). But we should also consider adding docs to the entity CRUD hooks to point out they fire both when saving default and non-default revisions.

markdorison’s picture

Status: Needs work » Needs review
FileSize
3.78 KB

Implemented a basic version of @catch's suggestion in #12 to verify the previously submitted test.

Status: Needs review » Needs work

The last submitted patch, 13: creating_a_draft-2708735-13.patch, failed testing.

jeqq’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 13: creating_a_draft-2708735-13.patch, failed testing.

amateescu’s picture

Status: Needs work » Needs review
Issue tags: -VDC, -Needs tests
FileSize
3.53 KB
4.23 KB
2.74 KB
+++ b/core/modules/taxonomy/tests/src/Kernel/ForwardRevisionTest.php
@@ -0,0 +1,111 @@
+    $node->setNewRevision(TRUE);
+    $node->isDefaultRevision(FALSE);
+    $node->field_tags->target_id = $term->id();
+    $node->save();
+
+    $taxonomy_index = $this->getTaxonomyIndex();
+    $this->assertEquals($term->id(), $taxonomy_index[$node->id()]->tid);

I think this assertion is wrong, the taxonomy index should not change when we're not dealing with the default revision. That's the whole point of this issue :)

Fixed the point above in this patch and also cleaned it up a little.

The last submitted patch, 17: 2708735-17-test-only.patch, failed testing.

jibran’s picture

Status: Needs review » Reviewed & tested by the community

Looks good now.

amateescu’s picture

Issue tags: +DevDaysMilan

Forgot the tag :)

  • catch committed c2217cd on 8.2.x
    Issue #2708735 by amateescu, dawehner, markdorison: Creating a draft...

  • catch committed 102225a on 8.1.x
    Issue #2708735 by amateescu, dawehner, markdorison: Creating a draft...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.2.x and cherry-picked to 8.1.x. Thanks!

Status: Fixed » Closed (fixed)

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