Is there a reason hook_field_update doesn't update the taxonomy index for unpublished entities? I have a use case where editors can manipulate the taxonomy terms of unpublished nodes but these updates aren't inserted into the taxonomy index table. This makes it a challenge to create a view that searches unpublished nodes by taxonomy term name (as opposed to tid, which can be search by taxonomy_field_name_instance)

This would be trivial to patch as it is just an if statement in line 501 of field.api.php.

CommentFileSizeAuthor
#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
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch add-unpublished-nodes-to-taxonomy-index-962664-30.patch. Unable to apply patch. See the log in the details link for more information. View
#28 add-unpublished-nodes-to-taxonomy-index-962664-28.patch8.29 KBklaasvw
FAILED: [[SimpleTest]]: [MySQL] 34,296 pass(es), 4 fail(s), and 2 exception(s). View
#20 taxonomy-index-unpublished-20.patch1.54 KBxjm
PASSED: [[SimpleTest]]: [MySQL] 33,544 pass(es). View
#18 taxonomy-index-unpublished-18.patch1.54 KBxjm
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in modules/taxonomy/taxonomy.module. View
#4 taxonomy.module.patch1.53 KBngmaloney
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch taxonomy.module_65.patch. View
Members fund testing for the Drupal project. Drupal Association Learn more

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
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch taxonomy.module_65.patch. View

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
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in modules/taxonomy/taxonomy.module. View

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
PASSED: [[SimpleTest]]: [MySQL] 33,544 pass(es). View

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
FAILED: [[SimpleTest]]: [MySQL] 34,296 pass(es), 4 fail(s), and 2 exception(s). View

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

FileSize
8.34 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch add-unpublished-nodes-to-taxonomy-index-962664-30.patch. Unable to apply patch. See the log in the details link for more information. View

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.

alisonjo2786’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.