Problem/Motivation

Identified problem detailed in #962664-9: Taxonomy Index for unpublished entities:

It is possible to query by field value in views. One of the drawbacks is, by default, you can only query by TID, not Term Name. Having the values in the taxonomy_index table gets around this issue.

And #962664-47: Taxonomy Index for unpublished entities:

In Drupal prior to 7.x there was an easy way to get all the terms associated with a particular node. In 7.x and beyond, it is very difficult, if not impossible, to get that list of terms. I maintain the tac_lite module, which controls access to nodes based on the terms they are tagged with. Currently there are edge cases where tac_lite cannot learn all the terms a node is tagged with.

And described in #962664-50: Taxonomy Index for unpublished entities:

So a relationship bewteen the contents and taxonomy terms is only recognized within a view, when the contents are published, which looks like a bug to me though. I didn't want my contents to be available as separate pages, but only as the source for the view(s). Since there is the filter criteria "Content: Published (Yes/No)" for views, they seem to be designed to support the output of the not published contents as well, so I think the relations to taxonomy terms should work for such not published contents as well.

Please note that a contrib module was also introduced to fix this in #962664-16: Taxonomy Index for unpublished entities:

I have written http://drupal.org/project/taxonomy_entity_index in the meantime to index all entities regardless of published or not. I've opened an issue to add views integration for it.

Per #962664-27: Taxonomy Index for unpublished entities:

Well, I think leaving out unpublished nodes was a design feature to reduce overhead, since this table can get crazy-big on a large site. Technically you could use the EFQ to look up unpublished nodes by taxonomy--fetch a list of all fields, look for taxonomy fields, look up which bundles have these fields, and loop to query against values in all these fields. But that's kinda wild.

Drupal 7 version of this issue: #2878046: D7 Taxonomy Index for unpublished entities

Steps to reproduce

I believe this edge-case is defined in #962664-50: Taxonomy Index for unpublished entities best:

So a relationship bewteen the contents and taxonomy terms is only recognized within a view, when the contents are published, which looks like a bug to me though. I didn't want my contents to be available as separate pages, but only as the source for the view(s). Since there is the filter criteria "Content: Published (Yes/No)" for views, they seem to be designed to support the output of the not published contents as well, so I think the relations to taxonomy terms should work for such not published contents as well.

Proposed resolution

Proposed change should update the taxonomy index for unpublished entities. Example use case is where editors manipulate the taxonomy terms of unpublished nodes to be used by a view that searches unpublished nodes by taxonomy term name (as opposed to tid, which can be search by taxonomy_field_name_instance).

Remaining tasks

See #962664-179: Taxonomy Index for unpublished entities:

We need to check the views query on taxonomy/term/{term} pages before and after the patch (with a lot of records in the index table), and compare the before/after EXPLAIN.

And per @xjm on Slack:

  • the taxonomy API, and Drupal overall, is very, very different than it was when this issue was proposed, so we really need people to think seriously about whether it makes sense anymore

User interface changes

@tbd

API changes

@tbd

Data model changes

Change record as seen in #962664-159: Taxonomy Index for unpublished entities: https://www.drupal.org/node/3133343

Release notes snippet

@tbd

CommentFileSizeAuthor
#206 962664-206.patch5.64 KBRichardDavies
#204 patch-failed.jpeg86.51 KBRichardDavies
#203 962664-203.patch5.64 KBshweta__sharma
#198 962664-198.patch5.64 KBeugene.brit
#195 962664-195.patch5.68 KBowenbush
#192 962664-192.patch.txt4.29 KBAkhil Yadav
#190 interdiff_185-186.txt649 bytesNivethaSubramaniyan
#190 962664-186.patch5.64 KBNivethaSubramaniyan
#185 reroll_diff_182-185.txt2.09 KBravi.shankar
#185 962664-185.patch6.04 KBravi.shankar
#182 962664-182.patch6.08 KBsmustgrave
#180 962664-180.patch6.07 KBranjith_kumar_k_u
#172 962664-drupal-add-unpublished-nodes-to-taxonomy-index-172.patch6.04 KBmsti
#168 962664-drupal-add-unpublished-nodes-to-taxonomy-index-168.patch6.08 KBsokru
#168 interdiff-157-168.txt513 bytessokru
#167 interdiff_157_167.txt93 bytesluca_loguercio
#167 962664-drupal-add-unpublished-nodes-to-taxonomy-index-167.patch6.08 KBluca_loguercio
#165 962664-drupal-add-unpublished-nodes-to-taxonomy-index-159.patch6.04 KBpotassium
#165 962664-drupal-add-unpublished-nodes-to-taxonomy-index-159.patch6.04 KBpotassium
#164 962664-drupal-add-unpublished-nodes-to-taxonomy-index-158.patch6.05 KBarawashdeh
#163 patch157_works_on_89x.png56.6 KBjoseph.olstad
#157 interdiff_156-157.txt572 bytesErik Frèrejean
#157 962664-drupal-add-unpublished-nodes-to-taxonomy-index-157.patch6.08 KBErik Frèrejean
#156 interdiff_154-156.txt2.56 KBravi.shankar
#156 962664-156.patch6.08 KBravi.shankar
#154 962664-drupal-add-unpublished-nodes-to-taxonomy-index-154.patch5.7 KBmaacl
#149 962664-drupal-add-unpublished-nodes-to-taxonomy-index-149.patch5.73 KBsokru
#149 interdiff_146-149.txt625 bytessokru
#146 interdiff_130-146.txt4.11 KBsokru
#146 962664-drupal-add-unpublished-nodes-to-taxonomy-index-146.patch5.8 KBsokru
#144 interdiff_130-144.txt4.06 KBsokru
#144 962664-drupal-add-unpublished-nodes-to-taxonomy-index-144.patch5.76 KBsokru
#142 interdiff_130-142.txt4.04 KBsokru
#142 962664-drupal-add-unpublished-nodes-to-taxonomy-index-142.patch5.73 KBsokru
#130 interdiff_126-8.8.x_130.diff1.13 KBmxr576
#130 drupal-add-unpublished-nodes-to-taxonomy-index-962664-130.patch5.71 KBmxr576
#126 interdiff_120_126.diff3.16 KBmxr576
#126 drupal-add-unpublished-nodes-to-taxonomy-index-962664-126-8.8.x.patch5.69 KBmxr576
#126 drupal-add-unpublished-nodes-to-taxonomy-index-962664-126.patch5.69 KBmxr576
#125 drupal-add-unpublished-nodes-to-taxonomy-index-962664-125.patch5.31 KBmxr576
#124 drupal-add-unpublished-nodes-to-taxonomy-index-962664-124.patch5.3 KBmxr576
#123 interdiff_116_120.txt1.1 KBmxr576
#120 drupal-add-unpublished-nodes-to-taxonomy-index-962664-120.patch5.25 KBdksdev01
#117 drupal-add-unpublished-nodes-to-taxonomy-index-962664-117.patch5.24 KBdksdev01
#116 interdiff_108_116.txt459 bytesmxr576
#116 drupal-add-unpublished-nodes-to-taxonomy-index-962664-116.patch5.38 KBmxr576
#110 drupal-add-unpublished-nodes-to-taxonomy-index-962664-103.patch5.26 KBdeepdru
#108 interdiff_107_108.txt620 bytesstefanos.petrakis@gmail.com
#108 drupal-add-unpublished-nodes-to-taxonomy-index-962664-108.patch5.26 KBstefanos.petrakis@gmail.com
#107 drupal-add-unpublished-nodes-to-taxonomy-index-962664-107.patch5.16 KBstefanos.petrakis@gmail.com
#106 drupal-add-unpublished-nodes-to-taxonomy-index-962664-106.patch5.16 KBstefanos.petrakis@gmail.com
#102 drupal-add-unpublished-nodes-to-taxonomy-index-962664-102.patch5.15 KBdeepdru
#101 drupal-add-unpublished-nodes-to-taxonomy-index-962664-101.patch5.13 KBErik Frèrejean
#100 drupal-add-unpublished-nodes-to-taxonomy-index-962664-100.patch2.9 KBErik Frèrejean
#96 D7-add-unpublished-nodes-to-taxonomy-index-962664-96.patch2.74 KBpmchristensen
#90 drupal_default-storage-and-index-unpublished_962664_90.patch77.21 KBmgalalm
#81 drupal-add-unpublished-nodes-to-taxonomy-index-962664-81.patch2.84 KBakalata
#76 interdiff_69_to_76.txt2.27 KBjoseph.olstad
#76 D7-add-unpublished-nodes-to-taxonomy-index-962664-76.patch2.68 KBjoseph.olstad
#70 interdiff_60_to_69.txt1.36 KBjoseph.olstad
#70 D7-add-unpublished-nodes-to-taxonomy-index-962664-69.patch2.65 KBjoseph.olstad
#68 D7-add-unpublished-nodes-to-taxonomy-index-962664-67_DO_NOT_TEST.patch3.29 KBjoseph.olstad
#68 D7-interdiff_60_to_67.txt2.79 KBjoseph.olstad
#63 drupal-add-unpublished-nodes-to-taxonomy-index-962664-63.patch2.86 KBrecrit
#60 D7-add-unpublished-nodes-to-taxonomy-index-962664-60_DO_NOT_TEST.patch2.3 KBLittleRedHen
#58 D7-add-unpublished-nodes-to-taxonomy-index-962664-58_DO_NOT_TEST.patch2.36 KBjoseph.olstad
#56 drupal-add-unpublished-nodes-to-taxonomy-index-962664-56.patch2.85 KBrecrit
#30 add-unpublished-nodes-to-taxonomy-index-962664-30.patch8.34 KBklaasvw
#28 add-unpublished-nodes-to-taxonomy-index-962664-28.patch8.29 KBklaasvw
#20 taxonomy-index-unpublished-20.patch1.54 KBxjm
#18 taxonomy-index-unpublished-18.patch1.54 KBxjm
#4 taxonomy.module.patch1.53 KBngmaloney
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

yched’s picture

Component: field system » taxonomy.module

recategorizing

ngmaloney’s picture

The code for this indexing happens in /modules/field/field.api.php so one could argue it is belongs there. Regardless of where it lives, it would be helpful to give developers the ability to index non-published fields, particularly in views. The patch could be something as simple as:

(field.api.php:501)

 if ($entity->status) {...

to something like...

if($entity->status || variable_get('index_non_published',false)) {...
yched’s picture

field.api.php is just documentation code that is never executed.
The behavior happens in taxonomy_field_update() in taxonomy.module.

ngmaloney’s picture

FileSize
1.53 KB

Thanks for the info yched. Attached is a patch that would allow developers to override the default behavior (allowing non-published entities to be indexed in taxonomy_index). It is similar to how taxonomy currently handles "taxonomy_maintain_index_table".

yched’s picture

Status: Active » Needs review

catch and bangpound designed the behaviour of the taxonomy index table.
Changes around this would need their seal of approval.

Status: Needs review » Needs work

The last submitted patch, taxonomy.module.patch, failed testing.

yched’s picture

patch needs to be rolled with git diff --no-prefix :-)

Anonymous’s picture

I'm going to need a little orientation with this issue, so please be patient with me.

The taxonomy_index table is used to reproduce the "taxonomy/term/%term" pages similar to how they were done in Drupal 6. Is Views also relying on it for some reason? Why can't it use the field data tables directly? The taxonomy_index table is mostly a bridge back to old functionality. You should still be able to create a view that can show unpublished nodes. These term vallues are stored in the field data tables.

ngmaloney’s picture

bangpound, you are correct, It is possible to query by field value in views. One of the drawbacks is, by default, you can only query by TID, not Term Name. Having the values in the taxonomy_index table gets around this issue. The other way is to write a php argument handler. The patched method seems like a "cleaner" implementation when doing look ups by Term Name.

I made the patch merely as a demo to spark some dialog. While it would be helpful I understand if it creates new issues and isn't worth going any further with.

Dave Reid’s picture

This is also necessary for #741914: Add a [node:term] to work reliably, which is a token that I'm getting a *ton* of requests for.

EDIT: Wrong issue link...fixed.

Martin Alm’s picture

The patch in #4 seems to fix this issue on nodes that are created, but upon saving a new (unpublished) node there still seems to be a problem. Any ideas on how to solve this?

elachlan’s picture

Subscribed.

Is there documentation for module maintainers on how to get taxonomy data for unpublished nodes? That is if this issue is not being followed through.

Shadlington’s picture

Subbing

elachlan’s picture

Has this issue been abandoned? It is holding back #741914: Add a [node:term] and it would be really great to be able to have forums which have a proper naming structure again. If there is a work around could someone please post it?

Mark Trapp’s picture

Version: 7.0-beta2 » 8.x-dev
Issue tags: +Needs backport to D7

Subscribing, re-versioning, tagging, and linking to relevant Views issue: #1182924: Unpublished entities do not appear in views when using a taxonomy relationship

Dave Reid’s picture

I have written http://drupal.org/project/taxonomy_entity_index in the meantime to index all entities regardless of published or not. I've opened an issue to add views integration for it.

RdeBoer’s picture

This is the same sort of trap that D6 core had in node_access(): if a node was not published, all access grants were ignored. Game over.

There are two checks for $entity->status in the Taxonomy module, one when inserting entities and one when updating. In my opinion neither should be there.

It should not be up to the Taxonomy module to do nothing simply because a node/entity is not published.

It's up to the (node) access modules to decide whether (fields of) a node/entity should be displayed to the logged-in user or not.

To give my D7.4 users Views that display taxonomy terms correctly whether the nodes in question are published or not, I ended up implementing in my module hook_node_insert() and hook_node_update() inserting in their bodies calls to a function that basically does what taxonomy_field_update() does, but unconditionally, regardless of publication status.

xjm’s picture

Status: Needs work » Needs review
FileSize
1.54 KB

Rerolled against 8.x, with the following changes:

  1. Renamed the variable to taxonomy_index_unpublished, which is more standard terminology
  2. Changed the default value to FALSE so the default core behavior is maintained when the variable is not set.

Status: Needs review » Needs work

The last submitted patch, taxonomy-index-unpublished-18.patch, failed testing.

xjm’s picture

Status: Needs work » Needs review
FileSize
1.54 KB

Erm. Let's try a valid patch.

xjm’s picture

Status: Needs review » Needs work

taxonomy_select_nodes() (which creates the term page and feed listings) selects all nodes by term from the {taxonomy_index} table. This patch would introduce unpublished nodes into those listings. So, at a minimum, the patch needs to include a fix there as well.

We would also need to identify any other instances where core might be making this assumption. There's no accounting for contrib. The upshot is that the setting would only ever be used by contrib or custom module developers who knew it existed, so for anyone else, no change has been made.

xjm’s picture

From catch on IRC:

I could live with adding unpublished nodes, if we add the status column too and fix the query.
adding a column is not out of the question, the table is built dynamically.

xjm’s picture

So, to add a status column to the table, the following changes would need to be made:

  1. Schema updated with the additional column.
  2. hook_update_N() added to:
    • add the column to the existing table.
    • set the status for existing records to 1.
    • insert records for unpublished nodes, with status 0.
  3. Add a WHERE status = 1 condition to existing selects against this table where relevant.
  4. Remove the if ($entity->status) checks in taxonomy_field_insert() and taxonomy_field_update().
  5. Add the status to the db_insert() queries in taxonomy_field_insert() and taxonomy_field_update().
swentel’s picture

Shameless subscribe - been bitten hard by this one for creating adminisatration pages a client today.

Taxoman’s picture

Subscribing

arski’s picture

subscribing, would love to see this in D7 asap :x thanks!

PS. Agreeing to what http://drupal.org/node/962664#comment-4692122 said, should this not be marked as a bug report? IMO it really is one as it makes it impossible to fetch unpublished nodes through their taxonomies :/

xjm’s picture

Well, I think leaving out unpublished nodes was a design feature to reduce overhead, since this table can get crazy-big on a large site. Technically you could use the EFQ to look up unpublished nodes by taxonomy--fetch a list of all fields, look for taxonomy fields, look up which bundles have these fields, and loop to query against values in all these fields. But that's kinda crazy.

#962664-23: Taxonomy Index for unpublished entities has a pattern for a solution that one of the core taxonomy maintainers has tentatively okayed, so that's the best bet for fixing this. I can try a patch at some point when I have time, but I won't have time this week, so if someone else does have time this week... :)

klaasvw’s picture

Status: Needs work » Needs review
FileSize
8.29 KB

Here's a patch that does the following:

  1. Add a status field to the taxonomy_index table and set it to 1 for existing entries.
  2. Add the status field to term_node index in the taxonomy_index table.
  3. On update, execute a batch job to add all unpublished nodes to the taxonomy_index table.
  4. Index both published and unpublished nodes.
  5. Set the status field in taxonomy_index to that of the node on insert and update.
  6. Make sure taxonomy_select_nodes only returns results for published nodes.
  7. Update documentation and field.api.inc code to reflect all the above changes.

Status: Needs review » Needs work

The last submitted patch, add-unpublished-nodes-to-taxonomy-index-962664-28.patch, failed testing.

klaasvw’s picture

Second try! This patch uses $status instead of $node->status and fixes some newline and whitespace issues.

I have no idea why the hook_update is failing. Looks like testbot sets up a site with the new schema and then executes the update, instead of setting up a site with the old schema.

Does someone have any idea how to fix this?

klaasvw’s picture

Status: Needs work » Needs review

Setting status.

Status: Needs review » Needs work

The last submitted patch, add-unpublished-nodes-to-taxonomy-index-962664-30.patch, failed testing.

arcane’s picture

Has anyone tried #30, really needing it for a project.

xjm’s picture

@arcane: In the interim, you might consider:
http://drupal.org/project/taxonomy_entity_index

arcane’s picture

Thanks, I did get taxonomy_entity_index working

agentrickard’s picture

@xjm - re: taxonomy_select_nodes() update: the current patch includes that change.

klonos’s picture

...friendly bump ;)

Kick to D9? :/

Mołot’s picture

Any reason it's a feature request and not a bug? I mean, really, index gets out of sync with nodes indexed in it... why? Was that some historic, pre-views design from times when adding filter->published for taxonomy listings was difficult? I was sure Drupal matured way past that.

agentrickard’s picture

It's a performance optimization for a common query.

Mołot’s picture

I always believed that performance data was supposed to be kept in cache_* tables. Current approach simplify one query, but breaks other, particularly in views. And given views are going to be in core now, it's certainly a core bug that setting view filters to show unpublished nodes of taxonomy term always returns empty set, isn't it? Question is, is it a bug in views or in taxonomy?

Mołot’s picture

Issue tags: +Views in Core

Adding "Views in Core" tag as this is quite significant incompatibility between views, new in core, and taxonomy. This incompatibility was forgiveable when views was a contrib module not meant to fully support everything out there. And please, consider if it's a views problem (they use table not meant for it in their filters) or taxonomy one (design flaw in table's routines).

agentrickard’s picture

Issue tags: -Views in Core

With Views in core, it is possible that the taxonomy-module page that relies on this table will be removed entirely and replaced with a view. Currently, you should use one or the other.

Mołot’s picture

It's not possible to build sane views with taxonomy filters now, as views are using this very table to join nodes to taxonomy, and it contains only part of the data. That's the very point. Do you suggest that using taxonomy_index in views code is views bug now?

coolestdude1’s picture

Just ran into this issue on a new feature to a site I am working on.

Wanted to avoid installing another module onto the site which includes it's own tables, so comment #30 was the key. I will not even be using the taxonomy index table for page displays so the aspects of status does not apply to my use case but I understand there would be further development after the status row is added.

I applied the patch in #30 pretty cleanly to a Drupal 7 install (minus the sandbox aspects of Drupal 8 upgrade scripts) and it is working exactly as intended. No adverse affects to other parts of the site and the beauty is that views continues to work flawlessly with this patch and is non the wiser.

alison’s picture

Issue summary: View changes

coolestdude1 -- I'm having trouble applying the patch on my D7 site -- hunk #2 of the patch to taxonomy.install fails for me (everything else applies fine). It looks like it isn't working because the patch is trying to act on a different version of taxonomy.install than what I have, which is from Drupal 7.26. Did you manually apply this patch, is that why it worked for you, or? I realize there's some D8 "stuff" going on, but the other parts of the patch applied so cleanly, I feel like I'm missing something...

coolestdude1’s picture

You can manually apply most patches if that works better for you. However module and core maintainers generally require all patches to be against the latest development copies, the process is typically referred to as 'rebase' around here. As mentioned in my previous post I did not require the status field so I left that whole portion of the patch out of my changes and picked which portions would apply here. My thought process for this change was to drop the node status as I do not really care if an entry is made on the taxonomy table for an unpublished node (distinguishing them was not something I found useful in my use cases).

I would say the most useful part of the patch is the changing to the way in which the node saves it's taxonomy information. Typically that information is stored alongside the nodes field tables and is later aggregated into the taxonomy_index table upon node publish. So the best part of the previous patch was getting past the if statement for the node status.

If you have any pages which directly expose taxonomies and nodes to non privileged users I would advise against this change as of now or at least hold off until the status flag is in use or a better solution can be found.

I have also begun to experiment with using this patch and the view_unpublished module to beef up security but have yet to have that particular solution working. Hope that helps.

Dave Cohen’s picture

+1 for this change, for D7 as well.

In browsing the comments in D7 taxonomy.module, it says, "When using other field storage engines or alternative methods of denormalizing this data you should set the variable 'taxonomy_maintain_index_table' to FALSE to avoid unnecessary writes in SQL." But, it fails to suggest an alternative to taxonomy_select_nodes() when 'taxonomy_maintain_index_table' is FALSE. Likewise taxonomy_term_feed() and taxonomy_term_page(), which will behave differently depending on how that variable is set.

I'm opposed to the "taxonomy_index_unpublished" variable, as it creates another option that makes Drupal's behavior unpredictable.

In Drupal prior to 7.x there was an easy way to get all the terms associated with a particular node. In 7.x and beyond, it is very difficult, if not impossible, to get that list of terms. I maintain the tac_lite module, which controls access to nodes based on the terms they are tagged with. Currently there are edge cases where tac_lite cannot learn all the terms a node is tagged with.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 30: add-unpublished-nodes-to-taxonomy-index-962664-30.patch, failed testing.

tr-drupal’s picture

Subscribing. Hope to see this in D7 as well, because I can't call it anyhting else but a bug, sorry.

It made me crazy to figure out what's going on. Here the full story: https://www.drupal.org/node/1107028#comment-9998093

The summary:

[...] So a relationship bewteen the contents and taxonomy terms is only recognized within a view, when the contents are published, which looks like a bug to me though. I didn't want my contents to be available as separate pages, but only as the source for the view(s). Since there is the filter criteria "Content: Published (Yes/No)" for views, they seem to be designed to support the output of the not published contents as well, so I think the relations to taxonomy terms should work for such not published contents as well.

ron_s’s picture

@tr-drupal, have you tried Taxonomy Entity Index, as already mentioned in this thread?
http://drupal.org/project/taxonomy_entity_index

In many cases this will meet the need for D7. Some additional details on this Drupal Answers page: http://drupal.stackexchange.com/questions/146925/indexing-the-term-of-un...

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

coleman.sean.c’s picture

If you're building a View in Drupal 8, there is a workaround.

Taxonomy term fields are just a special case of Entity Reference fields, which have their own views argument handler, which doesn't involve the `taxonomy_index` table. So, if Content-type X has a 'Tags' field, you can build a view based around `Content: Tags (field_tags)`, and that will include unpublished content as you'd expect.

dkre’s picture

@coleman.sean.c great tip - thank you.

You can also expand on this by using attachments for each different field/vocab. Otherwise multiple term contextual filters will clash, only the first will work.

Page > Set to display nothing (through filtering etc) > No results behaviour: Empty text area
Attachment1 > Context Filter: Field_A > Hide display if no results are found
Attachment2 > Context Filter: Field_B > Hide display if no results are found

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

recrit’s picture

In 8.2, the status already is tracked in the taxonomy_index table, see
http://cgit.drupalcode.org/drupal/tree/core/modules/taxonomy/src/TermSto...
This was added per commit 9ae8e2c8 for #2348007: Taxonomy term view needs status filter.
However, taxonomy_build_node_index() still only tracks published nodes.

The attached patch adds the following:
* Update to add unpublished nodes to the taxonomy_index table.
* taxonomy_build_node_index() updated to track unpublished nodes.

joseph.olstad’s picture

ok, can we please get a D7 backport of patch #56 ? I'll maybe take a stab at it myself

joseph.olstad’s picture

ok, here's an untested D7 backport of the D8 patch from comment #56, feel free to try it , I haven't yet tried it.

mellowtothemax’s picture

#56 worked for me on 8.2.3

LittleRedHen’s picture

@joseph.olstad: I had a need for this on D7, so tried the untested D7 backport from #58.
I found a couple of issues in the taxonomy_update_7012 function:

  • node_field_data is not the correct table name in the query fetching the count of unpublished nodes (just 'node' in D7)
  • joining node with taxonomy index when identifying unpublished nids is counterproductive, since these areby definition the nodes which are NOT already in taxonomy index (the join always gives an empty resultset)
  • The foreach loop should be over $nids, not over $nodes (which is undefined, and so the attempt throws an array_flip() error.

Here's a replacement patch with these issues resolved. This worked for me on 7.53

LittleRedHen’s picture

As an FYI for people working with the taxonomy_index-related views filters in D7, you can apply the patch from here to get tids from entityreference fields included in the index as well. The patch in #60 should apply cleanly over the top of that one, and the combination allowed filters to work properly on taxonomy_term entityreference fields on unpublished nodes.

There seems to be a patch-in-progress for the entityreference module with regards to the taxonomy index, but it doesn't include unpublished nodes either: to support those properly, the correction will need to take both types of term-reference into account.

Boobaa’s picture

Issue tags: +Needs tests

The patch in #56 looks proper to me, however, I got some of these messages during a drush updb:

Object of class Drupal\Core\Field\FieldItemList could not be converted to string Statement.php:59

So it seems there is something lurking around the hook_update_N(). Additionally, this will need some tests before getting committed.

recrit’s picture

@Boobaa,
Thanks for testing. There was an error with "$sandbox['last'] = $node->nid;" since $node was being loaded as the node object.
Fix: The attached patch uses $record as the query result row to avoid this issue.

chrisgross’s picture

#60 works for me on D7. Do we need a separate issue to get it committed?

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

joseph.olstad’s picture

@chrisgross , the D7 issue can be handled here.

We're needing the solution proposed in #60

Our need is to be able to display unpublished content for certain roles in views that have contextual relationships with taxonomies. #60 should help us achieve this design goal.

Thanks @LittleRedHen

joseph.olstad’s picture

Have no fear, a new patch is near.

I'm going to upload a new version of the D7 patch, however my suggested changes will probably will be pertinent to the D8 patch as well.

The bulk update is incorrectly indexing ALL unpublished nodes, it should be indexing either published OR default revision. This is consistent with the changes to taxonomy.module patch it'self. Test coverage for this would be likely difficult because the current test bot tests against a new database, the tests create the content. Whereas the test failures would occur when the database already has data and the updates are run and the incorrect stuff gets indexed.

In D8
taxonomy_update_8201

and

D7
taxonomy_update_7012

I will upload an interdiff to better illustrate the changes I speak of.

joseph.olstad’s picture

patch and interdiff for comment 67

For D7: Make sure to also use the @Littleredhen patch as well. #1586428: Integrate entyreference from nodes to terms with core taxonomy_index
comment 40

joseph.olstad’s picture

I added in checks for original node , but not sure if that is needed. More testing/dev tomorrow.

It seems however, that to be consistent the hook_update should be indexing only default revision content. Not all unpublished.

joseph.olstad’s picture

For patch 69, I removed the isOriginal stuff.

interdiff is from 60 to 69

also using this patch by @LittleRedHen

also using this module for 'Published or roles' viewing unpublished content.
https://www.drupal.org/project/views_published_or_roles

Status: Needs review » Needs work

The last submitted patch, 70: D7-add-unpublished-nodes-to-taxonomy-index-962664-69.patch, failed testing.

lokapujya’s picture

Is the D7 code dependent on Entity API?

joseph.olstad’s picture

Status: Needs work » Needs review

Patch 69 is working for us with or without the other patch.

Our use case narrative:
Our content editors need to see how their draft content will look on a views page, but we don't want unpriviledged users to see this content.

Alternate scenario 1 (critical to this issue, hence why we need the patch 69)
Our view has a taxonomy contextual filter, we're using the views_published_or_roles module to provide a view filter called 'Published or has role" which allows those with the selected roles to see draft items in the view.

Alternate scenario 2 (not critical to this issue):
content owners who do not want to log into the website will be using a hash key to view draft items in a view page. for this we're thinking https://www.drupal.org/project/access_unpublished

Why we need this patch:
We need patch 69 because of Alternate scenario 1.
Without patch 69, taxonomy terms for unpublished items can not be brought into a view via contextual filters

With patch 69, taxonomy terms brought into a view via contextual filter for unpublished items act as expected because they are indexed thanks to patch 69.

Why the test failures? Suspect that the tests that failed may have to be re-written or updated.

joseph.olstad’s picture

@lokapujya , Currently patch 69 For D7 will only index taxonomies for unpublished nodes if 'entity' is enabled, otherwise it behaves as vanilla D7 7.54 core.

The reason for doing it this way is to mimic what was done for D8, as 'entity' is now in D8 core.

IMHO, 'entity' should be added to D7 core as part of the new policy of adding functionality to D7 like what was done for 7.50 when version 7.45, 7.46, 7.47, 7.48 , 7.49 were skipped to indicate a semantic D7 friendly versioning.

lokapujya’s picture

Some minor review. In addition to fixing the test fails:

  1. +++ b/modules/taxonomy/taxonomy.install
    @@ -939,7 +939,7 @@ function taxonomy_update_7011(&$sandbox) {
    - * Add unpublished nodes to the taxonomy index table.
    + * Add entity_revision_is_default nodes to the taxonomy index table.
    

    The comment should describe the nodes, not use a variable name to describe the nodes.

  2. +++ b/modules/taxonomy/taxonomy.install
    @@ -958,12 +958,20 @@ function taxonomy_update_7012(&$sandbox) {
    +        $isDefaultRevision = (int) entity_revision_is_default($entity_type, $entity);
    

    is the cast needed? maybe it is, but it seems funny to me.

  3. +++ b/modules/taxonomy/taxonomy.install
    @@ -958,12 +958,20 @@ function taxonomy_update_7012(&$sandbox) {
    +      if($isDefaultRevision) {
    

    need a space after the if.

joseph.olstad’s picture

@lokapujya , thanks for the review. Here's a new D7 patch and an interdiff from 69 to 76. I've made the appropriate changes with respect to your feedback, and I also renamed a variable that was using camel case.

to @all, we're happily using patch 69 beautifully with contextual filter on taxonomy terms with views and views_published_or_roles filter for views to display unpublished content containing taxonomy term contextual filter to users that have roles.

Made another small change

+        $sandbox['last'] = $node->nid;
       }
-      $sandbox['last'] = $node->nid;

that might please the test bot, although not sure yet.

Status: Needs review » Needs work

The last submitted patch, 76: D7-add-unpublished-nodes-to-taxonomy-index-962664-76.patch, failed testing.

joseph.olstad’s picture

For D7 have a look at testDisabledNodeType, not sure yet why this is failing.

Also 129 code standard warnings? Looks like test bot runs coder review automatically now.

lathan’s picture

This was quite an issue we were facing as non administrators could not view unpublished content in the workbench view. This solved that for us using patch #63 against D8.2

joseph.olstad’s picture

Patch 76 is doing the job for us. Our client is very pleased now that we've implemented views_published_or_roles which due to our complex taxonomy relationship design in our view requires this patch.

Because of this we were able to convince our client that it is a superior workflow moving them off of deploy /staging and onto editing and moderation of content on a single site. Vastly simplifies our infrastructure.

akalata’s picture

Here's an update of #63 against 8.3.x. We should really be focusing on getting D8 updated before spending time on D7 -- if the maintainers decide this is out-of-scope as a new feature, whether against 8.3 or 8.4, there's very little chance of it getting into D7 core.

joseph.olstad’s picture

spending time on D7

there's very little chance of it getting into D7 core

@akalata, are you being paid by Wordpress or Joomla to say that sort of nonsense? D7 pays my bills (and very nicely at that), my clients are very happy customers, and I intend on getting this patch into D7 if I can, and am even willing to help you get this one into D8 first. You cannot force clients to move to D8 until they are ready to move. It's better to work together than to berate and discourage your D7 friends and D8 co-contributors.

cilefen’s picture

From https://www.drupal.org/node/2715157:

  • Bug reports should be posted against the active minor branch by default (8.1.x at the moment)
  • Where issues affect Drupal 7, a separate issue can be opened, related to the 8.x one
  • Drupal 7 issues should default to 'postponed' until the 8.x issue is fixed to avoid duplicate work, but can be worked on independently if the approach/patch needed is very different
  • 8.x issues will still be applied to the newest branch first (i.e. first 8.2.x then 8.1.x) to avoid regressions between minor releases
DamienMcKenna’s picture

@joseph.olstad: The Drupal core process dictates that any change must go into the current active branch before it is considered for backporting to the previous supported branch. In 2017, this means that changes must go into the active 8.X.x branch before it is considered for backporting into 7.x. This has been the standard process since 2010 when the policy was first defined, if you were unaware of it before now please keep it in mind for the future.

And please do not berate others for suggesting to focus on the current branch, that was uncalled for.

akalata’s picture

Re #82:

Hi Joseph, I understand you may be feeling frustrated, but quoting my responses out of context, making personal attacks, and sending private, threatening emails is not the proper avenue for resolving those feelings.

As #83 notes, I was referring to the documented process for getting a change like this into Drupal core. It was not meant to "berate or discourage," but instead describe the proper steps to follow so that it has the best chance of making it into core for BOTH versions.

joseph.olstad’s picture

@cilefen, yes , here is the D7 issue, thanks!
#2878046: D7 Taxonomy Index for unpublished entities

joseph.olstad’s picture

Is there anything preventing this (the D8 version) from being RTBC ?

Boobaa’s picture

@joseph.olstad: lack of tests is a showstopper, for example.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

mgalalm’s picture

Status: Needs review » Needs work
joseph.olstad’s picture

if you use this core patch, make sure to :
Hide Taxonomy Term Pages not intended for Display

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Pancho’s picture

This related issue prefers to take another direction, moving node specific code such as the {taxonomy_index} denormalization from taxonomy.module to node.module and makes denormalization pluggable for any entity type: #2917209: Remove node code from taxonomy. Make it pluggable

Boobaa’s picture

The patch in #90 seems to be targeted for a different issue.

  • Fist, it has a lot of erroneous cruft (like a bunch of .orig files).
  • The remaining stuff still contains a series of seemingly random changes from here and there throughout core (like Forum module's views and other Views-related things).
  • The upgrade path (the list of changes made to the taxonomy.install file) is totally different between #90 and #81.
  • Additionally, there's no interdiff.
  • Finally, #90 doesn't describe how that patch would solve the problem or why is it any better than the one in #81.

So I'm hiding the patch for #90 in favour of the #81 one (which still doesn't have an interdiff, but at least solves the original problem for me).

pmchristensen’s picture

Made a newer version of the patch from #76. Thanks for the great work - this is working with later version of Drupal 7.

joseph.olstad’s picture

Hi @pmchristiansen, thanks for your updated patch, however for it to be easier to review can you please include an interdiff comparing your patch with my previous patch (for D7)?

Also, there is a seperate D7 child issue that I created specifically for the D7 versions of this patch.
Please upload your patch and interdiff to the D7 child issue here:
#2878046: D7 Taxonomy Index for unpublished entities

Thanks,
joseph.olstad

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

brooke_heaton’s picture

Patch #90 fails on drupal/core 8.6.

Erik Frèrejean’s picture

Erik Frèrejean’s picture

Updated the patch with a testcase.

deepdru’s picture

Updated patch with db_query to truncate taxonomy_index which will eliminate 'SQLSTATE[23000]: Integrity constraint violation error'.

Status: Needs review » Needs work
deepdru’s picture

deepdru’s picture

stefanos.petrakis@gmail.com’s picture

Version: 8.6.x-dev » 8.7.x-dev
Status: Needs work » Needs review
FileSize
5.16 KB

Since this is a feature request, I am rerolling #101 against 8.7.x

stefanos.petrakis@gmail.com’s picture

Renaming the update function to be 8.7.x specific.

stefanos.petrakis@gmail.com’s picture

deepdru’s picture

deepdru’s picture

Status: Needs review » Needs work
stefanos.petrakis@gmail.com’s picture

Version: 8.6.x-dev » 8.7.x-dev
Status: Needs work » Needs review

I am going to set this back to "Needs review" and to 8.7.x, didn't read any comments in #110 and #111 which revolved around patch from #103 which has become outdated with #108.

Something automated was unintentionally involved perhaps?

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

justcaldwell’s picture

I was able to apply #108 to 8.7.1, but it results in a fatal error when running updates:

Fatal error: Cannot redeclare taxonomy_update_8701() (previously declared in /var/www/docroot/core/modules/taxonomy/taxonomy.install:193) in /var/www/docroot/core/modules/taxonomy/taxonomy.install on line 242

Looks like taxonomy ships with taxonomy_update_8701() already declared.

mxr576’s picture

Indeed, the problem exists. I bumped the update hook number.

dksdev01’s picture

sam.spinoy@gmail.com’s picture

Tested patch in #116 against D 8.7.5, works fine. Thanks to all for the time invested in this.

dksdev01’s picture

db_truncate('taxonomy_index'); doesn't actually truncate as it has missing execute(). If we make it work then the following statement taxonomy_build_node_index() only indexes unpublished not the published nodes.

dksdev01’s picture

Instead of using db_truncate(),it's better to use taxonomy_delete_node_index() that will avoid integrity constraint error.

joseph.olstad’s picture

Thanks for the patch @dksdev01 , would be nice to get this issue into some release of Drupal hopefully soon. #120 needs to be rolled for 8.7.x and 8.8.x , also, an interdiff from 116 would be helpful for those just wanting to look at the changes between patches.
Here's the docs on how to create an interdiff

mxr576’s picture

Assigned: Unassigned » mxr576

I'll work on those rerolls.

mxr576’s picture

mxr576’s picture

Reroll of #120, although I think the update hook can be further optimized so I'll work on that.

mxr576’s picture

mxr576’s picture

The last submitted patch, 125: drupal-add-unpublished-nodes-to-taxonomy-index-962664-125.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

joseph.olstad’s picture

Hi mxr576, nice job, almost perfect... I think the problem for 8.8.x is the last block of changes in that interdiff where $status is flipped for published inversely

joseph.olstad’s picture

ah ok so it failed because of deprecation, nice work! The interdiffs help us follow along, thanks!

Anybody’s picture

Thank you, we just ran into this problem in a project and #130 worked nice for us.

Anybody’s picture

Testing if #130 would also work for 8.7.x (optional).

Chris Matthews’s picture

I just ran into this problem as well and the 8.8.x patch in #130 works for my project.

joseph.olstad’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests, -Needs issue summary update

Has test included with the patch, RTBC based on tests and @Chris Matthews and @Anybody reported results.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Anybody’s picture

Sad to see that this will not be included in 8.8? Or is there any chance for this helpful fix?

william.jieh’s picture

After I applied this patch , do I need to run any command to update the Drupal database?

init90’s picture

Yes, you should run updates. For that go to SITE.COM/update.php or run drush updb command.

catch’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/modules/taxonomy/taxonomy.install
    @@ -248,3 +249,49 @@ function taxonomy_update_8702(&$sandbox) {
    +    $batch_size = 100;
    

    This should use entity_update_batch_size so that sites can tweak it.

  2. +++ b/core/modules/taxonomy/taxonomy.install
    @@ -248,3 +249,49 @@ function taxonomy_update_8702(&$sandbox) {
    +    // Build the taxonomy index for each node.
    +    foreach ($node_storage->loadMultiple($nids) as $node) {
    +      // Delete node index to avoid integrity constraint violation errors.
    +      taxonomy_delete_node_index($node);
    +      taxonomy_build_node_index($node);
    +      $sandbox['last'] = $node->id();
    

    Could this re-implement taxonomy_delete_node_index()/taxonomy_build_node_index() to avoid loading the nodes in the update?

Also the update could be a hook_post_update_NAME() instead of hook_update_N() since it's updating content rather than schema.

william.jieh’s picture

I'm using Lightning 8.x-4.003, it use composer, so If I want to apply the patch in #130 through cweagans/composer-patches , how should I do it? for example , in composer-patches document, there is an example:

"extra": {
    "patches": {
      "drupal/drupal": {
        "Add startup configuration for PHP server": "https://www.drupal.org/files/issues/add_a_startup-1543858-30.patch"
      }
    }
  }

Should I replace "drupal/drupal" to other text string? Like "acquia/lightning" because I'm using Lightning, or may be "drupal/core" ? because this patch is for taxonomy module which is a drupal core module.

sokru’s picture

This should take care of issues mentioned in #140

Status: Needs review » Needs work

The last submitted patch, 142: 962664-drupal-add-unpublished-nodes-to-taxonomy-index-142.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

sokru’s picture

Status: Needs review » Needs work
sokru’s picture

Reverting back the node loading because taxonomy_build_node_index is rather long function to re-implement.

simoneb’s picture

Patch 146 does not work anymore with Drupal 8.8.5.

Patch applies but UPDB command returns this error:

PHP Fatal error: Cannot use Drupal\Core\Site\Settings as Settings because the name is already in use in [...]/web/core/modules/taxonomy/taxonomy.post_update.php on line 18

mxr576’s picture

Assigned: mxr576 » Unassigned
sokru’s picture

maacl’s picture

Status: Needs review » Reviewed & tested by the community

Tested #149 on Drupal 8.8.5. successfully. Had to find out that unpublishing a translation removes the node from the taxonomy_index for all languages.

As far as I can tell the feedback from #140 is included in the current patch.

justinmello32’s picture

#130 works on Drupal 8.4.4, thanks!

Erik Frèrejean’s picture

Version: 8.9.x-dev » 9.1.x-dev
Status: Reviewed & tested by the community » Needs review

This needs to be targeted against 9.1.x.

Erik Frèrejean’s picture

Status: Needs review » Needs work
maacl’s picture

Here is a reroll of #149.

Erik Frèrejean’s picture

Status: Needs review » Needs work

@maacl you've introduced a couple of coding standards violations with your reroll.

  1. +++ b/core/modules/taxonomy/taxonomy.post_update.php
    @@ -18,3 +21,49 @@ function taxonomy_removed_post_updates() {
    +    $query      = $database->select('node_field_data', 'n');
    

    Incorrect indentation.

  2. +++ b/core/modules/taxonomy/tests/src/Functional/TermIndexTest.php
    @@ -120,6 +120,21 @@ public function testTaxonomyIndex() {
    +        ':nid' => $node->id(),
    +        ':tid' => $term_1->id(),
    +      ])->fetchField();
    

    Incorrect indentation.

  3. +++ b/core/modules/taxonomy/tests/src/Functional/TermIndexTest.php
    @@ -120,6 +120,21 @@ public function testTaxonomyIndex() {
    +        ':nid' => $node->id(),
    +        ':tid' => $term_1->id(),
    +      ])->fetchField();
    

    Incorrect indentation.

ravi.shankar’s picture

Status: Needs work » Needs review
FileSize
6.08 KB
2.56 KB

Here I have addressed comment #155.

Erik Frèrejean’s picture

Seems I missed one small cs change.

With the patch green on 9.1.x, this can go back to RTBC

xjm’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs subsystem maintainer review, +Needs framework manager review, +Security, +Needs change record

Oh wow, I remember this issue from back in the day when I was the Taxonomy Access Control maintainer and started working on the Taxonomy subsystem. (It was so exciting to be in MAINTAINERS.txt for the first time back then!)

I think we need to think carefully about the access implications of this change. TAC for example relied on that list as the basis of its access rules with the expectation that it was only published content, so adding new entries might have exposed access to content that wasn't available before.

We at least need a change record, and we need a careful review of the access implications. NW for the CR.

Erik Frèrejean’s picture

Bunty Badgujar’s picture

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

justinmello32’s picture

Is there a working patch for 8.9? I used to run #130 on 8.8 but after upgrading that fails and 157 doesn't seem to work either.

joseph.olstad’s picture

FileSize
56.6 KB

@mello23 , patch 157 applies correctly (with fuzz) to 8.9.x
Patch applies (with fuzz) to 8.9.x

Can you please repeat your steps ? What part fails, can you please be more specific?

Did you follow the installation steps after applying the patch?

From comment #139

you should run updates. For that go to SITE.COM/update.php or run drush updb command.

arawashdeh’s picture

There is a duplicate use of Drupal\Core\Site\Settings

potassium’s picture

Fix error in #164

line 54 changed from : @@ -5,6 +5,9 @@
to: @@ -5,6 +5,8 @@

Otherwise "composer install" was not working

Anybody’s picture

I can confirm that #157 does not work on Drupal 8.9.x:

PHP Fatal error: Cannot use Drupal\Core\Site\Settings as Settings because the name is already in use in /var/www/vhosts/dentaldirekt.de/dentaldirekt.de/web/core/modules/taxonomy/taxonomy.post_update.php on line 19

luca_loguercio’s picture

Seems there was a small typo in #157 with a missing `+`

sokru’s picture

Basically same as #167, but could not create interdiff (maybe for same reason the CI failed to apply the patch).

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

kreatIL’s picture

Drupal Version 8.9.16
In my case, applying patch #168 results in breaking the site. Calling admin/reports/status shows white screen of death. If I run drush updb the system throws the following error:

PHP Fatal error: Cannot use Drupal\Core\Site\Settings as Settings because the name is already in use in /path-to-my-local-installation/web/core/modules/taxonomy/taxonomy.post_update.php on line 19

msti’s picture

Version 9.1.10 - It applies and works.

msti’s picture

I have re-rolled the patch for 8.9.16
Tested and works for me.

@kreatIL

mcortes19’s picture

Tested and works for me on 8.9.16

kreatIL’s picture

Tested on 9.2.6. Works as expected. Thanks!

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

kreatIL’s picture

Status: Needs review » Reviewed & tested by the community

We have these functionalities of #168 patched into our production site since three months. No problems have occured and it works as expected. From my point of view it could be committed.

AurianaHg’s picture

I have tested the #172 on 9.2.4, and I had an issue during drush updb:

Error : Class 'Settings' not found in taxonomy_post_update_add_unpublished_nodes_to_taxonomy_index() ((line 43 of /var/www/html/htdocs/core/modules/taxonomy/taxonomy.post_update.php)

The line use Drupal\Core\Site\Settings; is missing, so the best patch is #168.

kreatIL’s picture

@AurianaHg:
#172 is a re-roll for Drupal 8.
#168 is for Drupal 9

catch’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs issue summary update

Unpublished nodes were originally excluded from the index so that it wouldn't be necessary to have a WHERE condition on the table, potentially making database queries more efficient.

We need to check the views query on taxonomy/term/{term} pages before and after the patch (with a lot of records in the index table), and compare the before/after EXPLAIN.

ranjith_kumar_k_u’s picture

Re-rolled #168 for 9.4

Status: Needs review » Needs work

The last submitted patch, 180: 962664-180.patch, failed testing. View results

smustgrave’s picture

Status: Needs work » Needs review
FileSize
6.08 KB

Same as #180 just fixed the test.

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community
quietone’s picture

Version: 9.4.x-dev » 10.0.x-dev
Issue summary: View changes
Status: Reviewed & tested by the community » Needs work
Issue tags: -Needs change record +Needs reroll

This is still in need of an Issue Summary update. It should, at least, include a description of the proposed resolution from the latest patch. Setting to NW.

This now needs to be on 10.0.x and the patch does not apply. Changing version and adding tag.

There is a change record, so removing tag.

ravi.shankar’s picture

Added reroll of patch #185 on Drupal 10.0.x, still needs work for Issue Summary update.

joseph.olstad’s picture

Issue summary: View changes
Anybody’s picture

Status: Needs work » Needs review

Back to Needs review as of #185 and #186 unclear if #186 is enough to fix the issue summary update, so leaving the tag for now.

nod_’s picture

Version: 10.0.x-dev » 10.1.x-dev

D10 version needed
At this time we would need a D10.1.x patch or MR for this issue.

Leaving NR as it seems to still apply to 10.0.x branch.

Bhanu951’s picture

Assigned: Unassigned » Bhanu951
NivethaSubramaniyan’s picture

Updated the patch for 10.1.x

Bhanu951’s picture

Assigned: Bhanu951 » Unassigned
Issue tags: -Needs reroll

Seems @NivethaSubramaniyan beat me to it. UnAssigning.

Akhil Yadav’s picture

FileSize
4.29 KB

Added Patch Against #186 in 10.1.x

Webbeh’s picture

Removing #192, as it is a duplicate of #190 as far as I can tell (same 10.1.x re-roll). Credit should not be assigned to the work in #192.

Webbeh’s picture

And took a first pass at getting the Issue Summary (IS) up to fluff on the history, background, and progress of the fix. Wading through the patch spiderweb to find the evolution of the fix is a pretty daunting task.

From Slack, an additional Needs Work remaining task to do per @xjm:

  • the taxonomy API, and Drupal overall, is very, very different than it was when this issue was proposed, so we really need people to think seriously about whether it makes sense anymore
owenbush’s picture

Re-roll of #182 for the latest 9.5.x changes, this removes a small change to the indentation in TermIndexTest.php which has already been fixed upstream.

@@ -62,7 +62,7 @@ protected function setUp(): void {
     $handler_settings = [
       'target_bundles' => [
         $this->vocabulary->id() => $this->vocabulary->id(),
-       ],
+      ],
       'auto_create' => TRUE,
     ];
     $this->createEntityReferenceField('node', 'article', $this->fieldName1, NULL, 'taxonomy_term', 'default', $handler_settings, FieldStorageDefinitionInterface::CARDINALITY_UNLIMITED);

smustgrave’s picture

Status: Needs review » Needs work

Moving to NW as @catch requested in #179 what still needs happen (per the IS).

Thanks!

xjm’s picture

In addition to profiling, we need to rethink whether this table even makes sense as-is anymore. Does it make sense to have this index table? Should it instead be implemented at the entity relationship level in Entity Reference's APIs? Should it use EFQ or an abstracted storage model instead of plain DB queries? Etc. We should answer those questions before rerolling this on and on. Thence the framework and subsystem maintainer review tags.

For now, I think the contrib module is sufficient for the immediate needs of taxonomy-related contrib or custom code. In the long term, IMO we need an entity-reference-level approach.

eugene.brit’s picture

FileSize
5.64 KB

The #185 patch has not applied for 10.0.8
re-roll this patch.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

herved’s picture

I also got bit by this issue today.
Similar to #150 when we unpublish a translation, the records in the taxonomy_index table get removed by taxonomy_node_update(), even though the original translation is unchanged.
This is an issue in my case when using the "Has taxonomy term ID" contextual filter in views which adds an inner join to the query.
The patch seems to resolve it.

joey-santiago’s picture

I also got here looking for the bug mentioned in #150: when an unpublished translation of a node is saved, the entry for that is removed from the taxonomy_index table.
In my case, the module Permissions by Term is using the taxonomy_index table to gather all the nodes a user is entitled to view. So suddenly nodes are removed from listings when unpublished translations are saved.

From what i have tested, the patch works well. I especially like the fact that the database is updated putting back in place all the entries from unpublished nodes. The patch applies #198 to 10.1 too.

Actually, this makes me think: shouldn't the taxonomy_index table also keep track of translations? a node can be related to different taxonomy terms in different translations, so i wonder how does core handle this atm?

RichardDavies’s picture

Can someone please reroll the patch for Drupal 10.2.0?

shweta__sharma’s picture

Re-rolling patch for 10.2.0 version.

RichardDavies’s picture

FileSize
86.51 KB

@shweta__sharma Thank you, but the patch won't apply for me with 10.2.0: screenshot of failed patch

kreatIL’s picture

The patch cannot be applied due to at least one issue: In Version 10.2.x, Line 228 (which corresponds to Line 208 in Version 10.1.x) has been modified. It now reads:
keys(['nid' => $node->id(), 'tid' => $tid, 'status' => $node->isPublished()])
It's important to note that the array is named "keys" in Version 10.2.x, as opposed to "key" which was used in Version 10.1.x.

RichardDavies’s picture

Updated @shweta__sharma's patch and changed key() to keys(). Patch now cleanly applies for me in 10.2.0.

tommyddp’s picture

Tried the patch for 10.2.x and it applies cleanly for me but does not fix the issue and I still only see published content in my view.

***edit after running drush updb the updates apply. Rookie mistake! Looks good on my end!