Taxonomy module currently maintains annoying custom storage and form handling for related terms.

Advantages of moving it to a field:

1. It'd be optional.
2. You could use different widgets for entering the terms (like autocomplete).
3. Related terms will be available in the term object for free via the field cache, making things like blocks of related terms easier/cheaper to provide
4. Would cut out a reasonable amount of crufty code no-one ever uses from taxonomy.module

CommentFileSizeAuthor
#18 before.png17.11 KBcatch
#18 after.png22.11 KBcatch
#18 related_terms.patch8.03 KBcatch
#17 related_terms.patch7.61 KBcatch
#8 related_terms.patch8.51 KBcatch
#6 related_terms.patch7.08 KBcatch
#4 related_terms.patch5.63 KBcatch
#3 related_terms.patch0 bytescatch
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

moshe weitzman’s picture

moshe weitzman CreditAttribution: moshe weitzman commented

Would be great to see a dev summary about our plan for all the reference modules like node_ref, user_ref, term_ref, block_ref, etc. Would be great to come up with a solution for all of them at once.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented

subscribe.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Active » Needs review
FileSize
related_terms.patch0 bytes

Initial patch.

This removes all related terms-specific code from taxonomy module, and adds an upgrade path converting it to a field.

Possible todos:

Does any site use related terms enough that we need to batch the update?

Do we need to keep the specific concept of 'related terms' in core other than providing an upgrade path for data, I think we probably don't - sites like the Encyclopedia of life need to be able to state the type of relationship between terms (synonym for example), not just that they're related in some unspecified way - fields and field instances are ideal for this.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
FileSize
related_terms.patch5.63 KB

Status: Needs review » Needs work

The last submitted patch failed testing.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Needs work » Needs review
FileSize
related_terms.patch7.08 KB

Forgot the one bit of code in Drupal which actually uses this, taxonomy.test

Status: Needs review » Needs work

The last submitted patch failed testing.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Needs work » Needs review
FileSize
related_terms.patch8.51 KB
yched’s picture

yched CreditAttribution: yched commented

I'm not really familiar with that feature, so I'll just feedback the 'Field' part.

- The related_terms field needs to have 'cardinality' => FIELD_CARDINALITY_UNLIMITED.

- We want to encourage a good practice where modules defining fields should prefix field name with 'MYMODULE_', to avoid name clashes in contrib.
It was decided (rightly so IMO) to leave node body as 'body' instead of 'node_body', but I'm wondering when is the time to start setting the good practice in core. Maybe here, maybe not, dunno for sure.

- Even if field_create_instance() fills in default widget and formatter as shorthands, it's probably best to explicitly provide a widget type and a formatter type for the 'full' build mode.

- As per the current state of 'taxo_term' field type, ['allowed_values']['vid'] is a field setting, not an instance setting - which possibly complicates the patch a bit ?

- Unfinished sentence ? // Fetch all

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented

My view of the patch.

I have no way of knowing how many people are using related terms. However, I do know that we'll definitely have to batch migration of taxonomy_term_node data to taxonomy term field instance values. ... someone's gonna have to batch it. Maybe not here and now, but we can't get by without doing it somewhere.

Yched's suggestion to use MODULE for the namespace dictates that this will be named taxonomy_FIELDNAME. Is that what you mean? I am getting slightly worried that someone somewhere will want vocabularies as a field, but I know what to tell them if they do.

The allowed_values for taxonomy term fields are field settings not instance settings. Furthermore, this patch is using it incorrectly. The snip of the settings for the field would be constructed like this:

'allowed_values' => array(
  array(
    'vid' => $vocabulary->vid,
    'parent' => '0',
  ),
);
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Needs review » Needs work

So if it's a field setting, that presumably means we'd need to create a separate field for every vocabulary to hold related terms? I'm not sure I like that as an option much in this case. bangpound, is that 'terms from any vocabulary' easter egg possible with the current term field - I'm thinking we could combine that with autocomplete and call this done - assuming my approach of removing the feature as such, and just letting fields replace it wholesale is agreed on more generall.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented

Catch:

You are right that it requires an additional field, but only if the cardinality of the "default" term field for a vocabulary is not multiple. Maybe instead we should make cardinality into an instance setting? Otherwise, it's just an additional field for those vocabularies which use related terms.

Terms from any vocabulary is not meant to be an easter egg. ;-) However, who is to say that "related" terms are only ever from the same vocabulary?

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

I meant more in relation to storage and allowed values, we currently have a single table for related terms, and an interface which only allows you to put terms from the vocabulary you're editing into it. A single field, with allowed values per-instance lets us do exactly the same thing - which means if there's a site using the existing feature (or a contrib module which actually uses it, not aware of any though), they can easily upgrade their code to use the upgraded data. I'd be much more concerned to keep the single table than the vocabulary restriction though.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented

catch

Why is a single table important for related terms but not for the terms themselves as they relate to nodes?

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

I think it's important for the terms themselves too - hence arguing for a term_node, term_user, term_$entity table in addition to default field storage on the taxonomy_node_* issue.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented

Catch and I discussed this in IRC. Because related terms means zilch in Drupal so far, we are doing a lot of work to migrate a core feature that really never existed.

The function Catch has begun to write to do migration is however entirely relevant to every migration of old-fashioned term_node data to new-fashioned field instance data. This function should (1) batch and (2) be abstract so that CORE can use it to migrate to fields and contrib modules that do care about related terms can rely on it to migrate as well.

Catch is proposing we quietly drop related terms from core, do not provide a hook_update_N to bring them into fields, and will follow up soon with details about what he's proposing we do.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Title: Convert related terms to use a term reference field » Remove related terms from core now that terms can have a term reference field
Status: Needs work » Needs review
FileSize
related_terms.patch7.61 KB

OK so my idea with the upgrade path was that field storage is more or less the same as related terms schema was, but turns out it won't be here, so we're better off leaving the table as-is on existing sites. That way a module using the table now which wants to keep it can adopt it as is, or migrate it to a field, or whatever they want.

But in core, we don't use this feature, and it's currently much, much better served by simply adding a term field to terms - you'll get views integration, related terms displayed on taxonomy/term/n pages, different form widget choices etc. for free, and takes zero code to do that.

So, let's just kill it.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
FileSize
related_terms.patch8.03 KB
after.png22.11 KB
before.png17.11 KB

Since we never, ever display that information or use it anywhere in any way whatsoever, here's the only UI impact of this patch in screenshots.

Also re-added dropping the relations column in taxonomy_vocabulary since it hasn't had accurate information in it since Drupal 5.

moshe weitzman’s picture

moshe weitzman CreditAttribution: moshe weitzman commented
Status: Needs review » Reviewed & tested by the community

well said. be gone.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Fixed

Makes sense here too.

Committed to HEAD.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

Nice, added to http://drupal.org/update/modules/6/7#related_terms

Status: Fixed » Closed (fixed)
Issue tags: -Fields in Core

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

Issue tags: +Fields in Core

Restoring issue tags, see #2125755: System messages removed all issue tags during D7 upgrade.