Replace taxonomy_index with field_data_taxonomy_weblinks table

Thanks for this background info.

unpublished nodes are not kept in the taxonomy_index table. Therefore when Web Links Checker unpublishes a node it has to be removed from this table as well, and vice versa.

Hopefully we do not have to do this manually - if we use node_save() for the updating as discussed in #2415693: Weblinks Checker should update node_revision table then Taxonomy module should take care of this. We can check this after implementing node_save()

the taxonomy_index table can be disabled on a site through variable 'taxonomy_maintain_index_table'.

If if the variable is set to FALSE are you saying we should not use the table? I guess, then, that this requires each of the four uses of the table to be examined to see what alternatives there are for this scenario. I thought this issue was going to be simple and quick to close ...

The variable 'taxonomy_maintain_index_table' is only used in taxonomy.module and field.api.php. Reading the comments in the header for taxonomy_field_presave() the variable defaults to TRUE and there is no admin config for this, it can only be set to FALSE by a custom module or maybe some contrib modules which would be fairly niche and specific. If this is the case then the site builder would know what they are doing, and they have a site which is not common. With this in mind, looking at our usages of the table:

1. In weblinks_term_node_count() we can return 0 for the term count.
2. Within our hook_node_load weblinks_node_load() there would be another way to get the terms, but that would be a separate issue and not a 7.x blocker
3. In _weblinks_get_query() likewise we would have to get the node term data from the other taxonomy tables. This is more work and may not hardly be needed, so again it could be covered by that separate issue.
4. For weblinks_user_form() if the taxonomy_index table was not maintained the processing could follow the same route as when taxonomy module does not exist, and all the users weblinks would be unclassified.

Hence for Weblinks 7.x-1.0 we need to return sensible results when the taxonomy_index table is not being maintained, but we can leave the implementation of alternative methods of term retrieval until after our 1.0 release, and then only when requested by our users - there may be not a single site which is both using Weblinks and has taxonomy_index table disabled.

As expected, now that we are using node_save() when Weblinks Checker unpublishes a bad link the taxonomy_index table is automatically maintained (ie the rows are removed). When the url is good again and Checker republishes the node, the rows are added back into the taxonomy_index table. So that's good.

The four places where changes are required to check if we can use the the taxonomy_index table are:

1. In weblinks_term_node_count() we can use table field_data_taxonomy_weblinks as an alternative source of term to node data.
2. Function weblinks_node_load() is inefficient, and I don't know why we do it this way. It selects all fields from the node table for all nids being loaded. Then for each of them, checks taxonomy_index to get terms but joining back to node to filter on type 'weblinks'. We can use the existing ->taxonomy_weblinks property. This not only saves two database queries and avoids using taxonomy_index but means we still load the terms even when the node is unpublished.
3. In _weblinks_get_query() we can use table field_data_taxonomy_weblinks if taxonomy_index is not being maintained
4. In weblinks_user_form() likewise we can use table field_data_taxonomy_weblinks

Then this got me thinking ... If we can use table field_data_taxonomy_weblinks when taxonomy_index is not to be trusted, why can't we use it all the time? This would save the added complexity of checking, and avoid duplicating some code.

I have addresses each of the issues above, and made four separate patches - named accordingly. This is so you can test each change separately if you wish, or you can apply all four together as they do not clash. Once you've had time to look at the code, try it and have some thoughts then I will make a proper clean patch to test.

Jonathan

Thanks for the background. I hope you realise that when I make these questions there is no implied criticism or anything like that. I just ask the questions, and you always have good answers. :-)

I had a search but could not find the issue you mention, but anyway from what you say, that is good we can drop ->taxonomy. It will also be useful to make it consistent by using field_data_taxonomy_weblinks in all cases.

Would you like me to make a single clean patch based on this? The single debug patches are still useful if you want to see what I did and how the code developed.

Here is a clean patch for 1, 3 and 4.
For item 2 I have created a separate issue #2449127: Remove $node->taxonomy and replace all usages with $node->taxonomy_weblinks

Thanks for the review

a. Yes, maybe you are right. Those strings are ok to remain without a capital letter.

b. So you are saying that just using $query = db_select('node', 'n'); other contrib module can add extra fields to the node table? I'll take a look at Modr8 as that sounds interesting.

c. Yes you are are right we do need to check. In particular when a minimal installation is used without Taxonomy module. I had that on my list to check but forgot to mention it. There may even be some php5 warnings to fix.

Modr8 actually changes the core node table by adding a 'moderate' field. I didn't know that was allowed, or at least I thought it was against good practice. I don't see why we should give extra importance to this particular contrib module and display the ->moderate field when there could be any number of fields added to the node table which "might be useful". I propose we drop it, but I'm ready to be persuaded if you have a strong view on this.

With Taxonomy module not enabled the changes to weblinks.module and weblinks.user.inc are all OK, as they will only be called when we are doing group work, which in turn means that the Taxonomy and Field modules must be available. Before installing Web Links I could not find a way to disable the 'field' core module. Is it possible? It says that it is required by 'Drupal' and there also seems to be a reciprocal requirement between 'Field' and 'Field SQL storage' which means that neither can be disabled. Is this due to some deep config setting?

Here is a patch with these things tidied up.

OK we'll continue to add the 'moderate' field, although the Modr8 module does not have a full 7.x version yet. They have 500 installs of 7.x-dev and 1,700 at 6.x I think there are more widely used moderation modules such as Workbench Moderation which has 19,000 installs at 7.x. What would be really nice is if we could implement a hook or alter function allowing other modules to add their own entries to this table. Then Modr8 could add text and/or links, or any custom module could include extras. We could also add links on behalf of other modules if they are installed.

One question on coding style and your preference: query->fields('n', array('nid', ...)) fails if it specifies a field which does not exist. So to include the optional 'moderate' field we either load all fields from the node table (which is against best practice as we only need six out of the fourteen fields, therefore doing more db work that necessary), or we do a conditional if module_exists('modr8') which adds complexity and an extra bit of processing. I think in this situation the difference in page serve time is going to be negligible, but I'd like to hear your view on this.

Thanks for the background Nancy. I'd like to drop it too, now that it is not in Core. It was only a text value after all, not an active link for the user to click. We could look at making the table properly customisable at a later date, after out 7.x release.

Thanks for the tip about db_query in preference to db_select, that's a useful reminder (I think I had read it, but forgotten)

Thanks Gerhard for finding those past issues - one was even started by me, I'd forgotten that.
I'm pleased we all agree. Here is an updated patch.

Thank you both for all your input and help.