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
Comment | File | Size | Author |
---|---|---|---|
#206 | 962664-206.patch | 5.64 KB | RichardDavies |
#204 | patch-failed.jpeg | 86.51 KB | RichardDavies |
#203 | 962664-203.patch | 5.64 KB | shweta__sharma |
#198 | 962664-198.patch | 5.64 KB | eugene.brit |
#195 | 962664-195.patch | 5.68 KB | owenbush |
Comments
Comment #1
yched CreditAttribution: yched commentedrecategorizing
Comment #2
ngmaloney CreditAttribution: ngmaloney commentedThe 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)
to something like...
Comment #3
yched CreditAttribution: yched commentedfield.api.php is just documentation code that is never executed.
The behavior happens in taxonomy_field_update() in taxonomy.module.
Comment #4
ngmaloney CreditAttribution: ngmaloney commentedThanks 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".
Comment #5
yched CreditAttribution: yched commentedcatch and bangpound designed the behaviour of the taxonomy index table.
Changes around this would need their seal of approval.
Comment #7
yched CreditAttribution: yched commentedpatch needs to be rolled with git diff --no-prefix :-)
Comment #8
Anonymous (not verified) CreditAttribution: Anonymous commentedI'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.
Comment #9
ngmaloney CreditAttribution: ngmaloney commentedbangpound, 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.
Comment #10
Dave ReidThis 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.
Comment #11
Martin Alm CreditAttribution: Martin Alm commentedThe 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?
Comment #12
elachlan CreditAttribution: elachlan commentedSubscribed.
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.
Comment #13
Shadlington CreditAttribution: Shadlington commentedSubbing
Comment #14
elachlan CreditAttribution: elachlan commentedHas 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?
Comment #15
Mark TrappSubscribing, re-versioning, tagging, and linking to relevant Views issue: #1182924: Unpublished entities do not appear in views when using a taxonomy relationship
Comment #16
Dave ReidI 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.
Comment #17
RdeBoerThis 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.
Comment #18
xjmRerolled against 8.x, with the following changes:
taxonomy_index_unpublished
, which is more standard terminologyFALSE
so the default core behavior is maintained when the variable is not set.Comment #20
xjmErm. Let's try a valid patch.
Comment #21
xjmtaxonomy_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.
Comment #22
xjmFrom catch on IRC:
Comment #23
xjmSo, to add a status column to the table, the following changes would need to be made:
WHERE status = 1
condition to existing selects against this table where relevant.if ($entity->status)
checks intaxonomy_field_insert()
andtaxonomy_field_update()
.db_insert()
queries intaxonomy_field_insert()
andtaxonomy_field_update()
.Comment #24
swentel CreditAttribution: swentel commentedShameless subscribe - been bitten hard by this one for creating adminisatration pages a client today.
Comment #25
Taxoman CreditAttribution: Taxoman commentedSubscribing
Comment #26
arski CreditAttribution: arski commentedsubscribing, 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 :/
Comment #27
xjmWell, 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... :)
Comment #28
klaasvw CreditAttribution: klaasvw commentedHere's a patch that does the following:
Comment #30
klaasvw CreditAttribution: klaasvw commentedSecond 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?
Comment #31
klaasvw CreditAttribution: klaasvw commentedSetting status.
Comment #33
arcane CreditAttribution: arcane commentedHas anyone tried #30, really needing it for a project.
Comment #34
xjm@arcane: In the interim, you might consider:
http://drupal.org/project/taxonomy_entity_index
Comment #35
arcane CreditAttribution: arcane commentedThanks, I did get taxonomy_entity_index working
Comment #36
agentrickard@xjm - re: taxonomy_select_nodes() update: the current patch includes that change.
Comment #37
klonos...friendly bump ;)
Kick to D9? :/
Comment #38
Mołot CreditAttribution: Mołot commentedAny 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.
Comment #39
agentrickardIt's a performance optimization for a common query.
Comment #40
Mołot CreditAttribution: Mołot commentedI 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?
Comment #41
Mołot CreditAttribution: Mołot commentedAdding "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).
Comment #42
agentrickardWith 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.
Comment #43
Mołot CreditAttribution: Mołot commentedIt'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?
Comment #44
coolestdude1 CreditAttribution: coolestdude1 commentedJust 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.
Comment #45
alisoncoolestdude1 -- 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...
Comment #46
coolestdude1 CreditAttribution: coolestdude1 commentedYou 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.
Comment #47
Dave Cohen CreditAttribution: Dave Cohen commented+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.
Comment #50
tr-drupal CreditAttribution: tr-drupal commentedSubscribing. 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:
Comment #51
ron_s CreditAttribution: ron_s commented@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...
Comment #53
coleman.sean.c CreditAttribution: coleman.sean.c as a volunteer commentedIf 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.
Comment #54
dkre CreditAttribution: dkre commented@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
Comment #56
recrit CreditAttribution: recrit commentedIn 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.
Comment #57
joseph.olstadok, can we please get a D7 backport of patch #56 ? I'll maybe take a stab at it myself
Comment #58
joseph.olstadok, here's an untested D7 backport of the D8 patch from comment #56, feel free to try it , I haven't yet tried it.
Comment #59
mellowtothemax CreditAttribution: mellowtothemax commented#56 worked for me on 8.2.3
Comment #60
LittleRedHen CreditAttribution: LittleRedHen commented@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:array_flip()
error.Here's a replacement patch with these issues resolved. This worked for me on 7.53
Comment #61
LittleRedHen CreditAttribution: LittleRedHen commentedAs 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.
Comment #62
BoobaaThe patch in #56 looks proper to me, however, I got some of these messages during a
drush updb
:So it seems there is something lurking around the
hook_update_N()
. Additionally, this will need some tests before getting committed.Comment #63
recrit CreditAttribution: recrit commented@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.
Comment #64
chrisgross CreditAttribution: chrisgross commented#60 works for me on D7. Do we need a separate issue to get it committed?
Comment #66
joseph.olstad@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
Comment #67
joseph.olstadHave 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.
Comment #68
joseph.olstadpatch 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
Comment #69
joseph.olstadI 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.
Comment #70
joseph.olstadFor 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
Comment #72
lokapujyaIs the D7 code dependent on Entity API?
Comment #73
joseph.olstadPatch 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.
Comment #74
joseph.olstad@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.
Comment #75
lokapujyaSome minor review. In addition to fixing the test fails:
The comment should describe the nodes, not use a variable name to describe the nodes.
is the cast needed? maybe it is, but it seems funny to me.
need a space after the if.
Comment #76
joseph.olstad@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
that might please the test bot, although not sure yet.
Comment #78
joseph.olstadFor 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.
Comment #79
lathanThis 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
Comment #80
joseph.olstadPatch 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.
Comment #81
akalata CreditAttribution: akalata commentedHere'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.
Comment #82
joseph.olstad@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.
Comment #83
cilefen CreditAttribution: cilefen commentedFrom https://www.drupal.org/node/2715157:
Comment #84
DamienMcKenna@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.
Comment #85
akalata CreditAttribution: akalata commentedRe #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.
Comment #86
joseph.olstad@cilefen, yes , here is the D7 issue, thanks!
#2878046: D7 Taxonomy Index for unpublished entities
Comment #87
joseph.olstadIs there anything preventing this (the D8 version) from being RTBC ?
Comment #88
Boobaa@joseph.olstad: lack of tests is a showstopper, for example.
Comment #90
mgalalm CreditAttribution: mgalalm as a volunteer and at Johnson & Johnson commentedThis is 2-part patch to who need to leverage the change of the taxonomy parent storage #2543726: Make $term->parent behave like any other entity reference field, to fix REST and Migrate support and de-customize its Views integration along with this #962664: Taxonomy Index for unpublished entities
Comment #92
joseph.olstadif you use this core patch, make sure to :
Hide Taxonomy Term Pages not intended for Display
Comment #94
PanchoThis 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
Comment #95
BoobaaThe patch in #90 seems to be targeted for a different issue.
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).
Comment #96
pmchristensen CreditAttribution: pmchristensen commentedMade a newer version of the patch from #76. Thanks for the great work - this is working with later version of Drupal 7.
Comment #97
joseph.olstadHi @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
Comment #99
brooke_heaton CreditAttribution: brooke_heaton commentedPatch #90 fails on drupal/core 8.6.
Comment #100
Erik FrèrejeanReroll the patch from #81 on 8.6.x.
Comment #101
Erik FrèrejeanUpdated the patch with a testcase.
Comment #102
deepdru CreditAttribution: deepdru commentedUpdated patch with db_query to truncate taxonomy_index which will eliminate 'SQLSTATE[23000]: Integrity constraint violation error'.
Comment #104
deepdru CreditAttribution: deepdru commentedComment #105
deepdru CreditAttribution: deepdru commentedComment #106
stefanos.petrakis@gmail.comSince this is a feature request, I am rerolling #101 against 8.7.x
Comment #107
stefanos.petrakis@gmail.comRenaming the update function to be 8.7.x specific.
Comment #108
stefanos.petrakis@gmail.comAdding the table truncation (described in #102)
Comment #110
deepdru CreditAttribution: deepdru commentedComment #111
deepdru CreditAttribution: deepdru commentedComment #113
stefanos.petrakis@gmail.comI 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?
Comment #115
justcaldwellI 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.
Comment #116
mxr576Indeed, the problem exists. I bumped the update hook number.
Comment #117
dksdev01 CreditAttribution: dksdev01 commentedattached updated #101 for d8.6
Comment #118
sam.spinoy@gmail.com CreditAttribution: sam.spinoy@gmail.com at Adapt commentedTested patch in #116 against D 8.7.5, works fine. Thanks to all for the time invested in this.
Comment #119
dksdev01 CreditAttribution: dksdev01 commenteddb_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.
Comment #120
dksdev01 CreditAttribution: dksdev01 commentedInstead of using db_truncate(),it's better to use taxonomy_delete_node_index() that will avoid integrity constraint error.
Comment #121
joseph.olstadThanks 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
Comment #122
mxr576I'll work on those rerolls.
Comment #123
mxr576Comment #124
mxr576Reroll of #120, although I think the update hook can be further optimized so I'll work on that.
Comment #125
mxr576This should apply on 8.8.x.
Comment #126
mxr576Improved the update hook inherited from #120 and earlier patches.
Comment #129
joseph.olstadHi 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
Comment #130
mxr576Isn't it failed because of the deprecation testing in the 8.8.x builds?
Replaced
db_query()
calls in the test, let's see.Comment #131
joseph.olstadah ok so it failed because of deprecation, nice work! The interdiffs help us follow along, thanks!
Comment #132
AnybodyThank you, we just ran into this problem in a project and #130 worked nice for us.
Comment #133
AnybodyTesting if #130 would also work for 8.7.x (optional).
Comment #134
Chris Matthews CreditAttribution: Chris Matthews commentedI just ran into this problem as well and the 8.8.x patch in #130 works for my project.
Comment #135
joseph.olstadHas test included with the patch, RTBC based on tests and @Chris Matthews and @Anybody reported results.
Comment #137
AnybodySad to see that this will not be included in 8.8? Or is there any chance for this helpful fix?
Comment #138
william.jieh CreditAttribution: william.jieh commentedAfter I applied this patch , do I need to run any command to update the Drupal database?
Comment #139
init90Yes, you should run updates. For that go to
SITE.COM/update.php
or rundrush updb
command.Comment #140
catchThis should use entity_update_batch_size so that sites can tweak it.
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.
Comment #141
william.jieh CreditAttribution: william.jieh commentedI'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:
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.Comment #142
sokru CreditAttribution: sokru as a volunteer commentedThis should take care of issues mentioned in #140
Comment #144
sokru CreditAttribution: sokru as a volunteer commentedComment #146
sokru CreditAttribution: sokru as a volunteer commentedReverting back the node loading because
taxonomy_build_node_index
is rather long function to re-implement.Comment #147
simoneb CreditAttribution: simoneb commentedPatch 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
Comment #148
mxr576Comment #149
sokru CreditAttribution: sokru as a volunteer commentedReroll from #146 to fix the issue on #147.
Comment #150
maacl CreditAttribution: maacl commentedTested #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.
Comment #151
justinmello32 CreditAttribution: justinmello32 as a volunteer commented#130 works on Drupal 8.4.4, thanks!
Comment #152
Erik FrèrejeanThis needs to be targeted against 9.1.x.
Comment #153
Erik FrèrejeanComment #154
maacl CreditAttribution: maacl commentedHere is a reroll of #149.
Comment #155
Erik Frèrejean@maacl you've introduced a couple of coding standards violations with your reroll.
Incorrect indentation.
Incorrect indentation.
Incorrect indentation.
Comment #156
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedHere I have addressed comment #155.
Comment #157
Erik FrèrejeanSeems I missed one small cs change.
With the patch green on 9.1.x, this can go back to RTBC
Comment #158
xjmOh 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.
Comment #159
Erik FrèrejeanQuick CR draft, https://www.drupal.org/node/3133343
Comment #160
Bunty Badgujar CreditAttribution: Bunty Badgujar at Srijan | A Material+ Company for Drupal India Association commentedAdding related issue.
Comment #162
justinmello32 CreditAttribution: justinmello32 as a volunteer commentedIs 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.
Comment #163
joseph.olstad@mello23 , patch 157 applies correctly (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
Comment #164
arawashdeh CreditAttribution: arawashdeh at Vardot commentedThere is a duplicate use of Drupal\Core\Site\Settings
Comment #165
potassium CreditAttribution: potassium as a volunteer commentedFix error in #164
line 54 changed from : @@ -5,6 +5,9 @@
to: @@ -5,6 +5,8 @@
Otherwise "composer install" was not working
Comment #166
AnybodyI can confirm that #157 does not work on Drupal 8.9.x:
Comment #167
luca_loguercio CreditAttribution: luca_loguercio commentedSeems there was a small typo in #157 with a missing `+`
Comment #168
sokru CreditAttribution: sokru as a volunteer commentedBasically same as #167, but could not create interdiff (maybe for same reason the CI failed to apply the patch).
Comment #170
kreatIL CreditAttribution: kreatIL as a volunteer commentedDrupal 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
Comment #171
mstiVersion 9.1.10 - It applies and works.
Comment #172
mstiI have re-rolled the patch for 8.9.16
Tested and works for me.
@kreatIL
Comment #173
mcortes19 CreditAttribution: mcortes19 as a volunteer commentedTested and works for me on 8.9.16
Comment #174
kreatIL CreditAttribution: kreatIL as a volunteer commentedTested on 9.2.6. Works as expected. Thanks!
Comment #176
kreatIL CreditAttribution: kreatIL as a volunteer commentedWe 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.
Comment #177
AurianaHg CreditAttribution: AurianaHg at State of Geneva commentedI 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.Comment #178
kreatIL CreditAttribution: kreatIL as a volunteer commented@AurianaHg:
#172 is a re-roll for Drupal 8.
#168 is for Drupal 9
Comment #179
catchUnpublished 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.
Comment #180
ranjith_kumar_k_u CreditAttribution: ranjith_kumar_k_u at Zyxware Technologies commentedRe-rolled #168 for 9.4
Comment #182
smustgrave CreditAttribution: smustgrave at Mobomo commentedSame as #180 just fixed the test.
Comment #183
smustgrave CreditAttribution: smustgrave at Mobomo commentedComment #184
quietone CreditAttribution: quietone at PreviousNext commentedThis 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.
Comment #185
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedAdded reroll of patch #185 on Drupal 10.0.x, still needs work for Issue Summary update.
Comment #186
joseph.olstadComment #187
AnybodyBack 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.
Comment #188
nod_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.
Comment #189
Bhanu951 CreditAttribution: Bhanu951 as a volunteer commentedComment #190
NivethaSubramaniyan CreditAttribution: NivethaSubramaniyan as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedUpdated the patch for 10.1.x
Comment #191
Bhanu951 CreditAttribution: Bhanu951 as a volunteer commentedSeems @NivethaSubramaniyan beat me to it. UnAssigning.
Comment #192
Akhil Yadav CreditAttribution: Akhil Yadav commentedAdded Patch Against #186 in 10.1.x
Comment #193
WebbehRemoving #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.
Comment #194
WebbehAnd 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:
Comment #195
owenbush CreditAttribution: owenbush at Lullabot commentedRe-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.
Comment #196
smustgrave CreditAttribution: smustgrave at Mobomo commentedMoving to NW as @catch requested in #179 what still needs happen (per the IS).
Thanks!
Comment #197
xjmIn 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.
Comment #198
eugene.britThe #185 patch has not applied for 10.0.8
re-roll this patch.
Comment #200
herved CreditAttribution: herved commentedI 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.
Comment #201
joey-santiago CreditAttribution: joey-santiago at Trimble Solutions Corporation commentedI 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?
Comment #202
RichardDavies CreditAttribution: RichardDavies at City of Portland commentedCan someone please reroll the patch for Drupal 10.2.0?
Comment #203
shweta__sharma CreditAttribution: shweta__sharma at OpenSense Labs commentedRe-rolling patch for 10.2.0 version.
Comment #204
RichardDavies CreditAttribution: RichardDavies at City of Portland commented@shweta__sharma Thank you, but the patch won't apply for me with 10.2.0:
Comment #205
kreatIL CreditAttribution: kreatIL commentedThe 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.
Comment #206
RichardDavies CreditAttribution: RichardDavies at City of Portland commentedUpdated @shweta__sharma's patch and changed key() to keys(). Patch now cleanly applies for me in 10.2.0.
Comment #207
tommyddp CreditAttribution: tommyddp commentedTried 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!