Updated: Comment #171

Problem/Motivation

  • Following #413192 make taxonomy terms fieldable, the taxonomy term description base field is unnecessary.
  • For some vocabularies, e.g. the forum vocabulary, a taxonomy term description is a reasonable part of the taxonomy data model.
  • The tags vocabulary supplied for article nodes is intended for freetagging, so a description field doesn't really fit the usecase for that vocabulary.

Proposed resolution

  • Remove the description base field from taxonomy.
  • Do not add a description field to term bundles by default.
  • Do not add it for tags.
  • Do add a fixed, locked description for forum terms, in forum.module, so that the templates and code there can rely on it.

Remaining tasks

  • Update the patch for this issue to implement the proposed resolution, without any upgrade path.
  • TBD, potentially a followup discussion: For D7 -> D8 migrations, any term bundle having a populated description field for any terms in the vocabulary should be migrated into a new configurable description field on that term bundle on the destination site. How do we make this easy for people?

User interface changes

There is no longer a description field available on taxonomy terms by default.

API changes

TBD

Files: 
CommentFileSizeAuthor
#191 interdiff.txt2.51 KBswentel
#191 term-description-569434-191.patch31.92 KBswentel
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,960 pass(es).
[ View ]
#189 term-description-569434-189-interdiff.txt3.22 KBBerdir
#189 term-description-569434-189.patch31.64 KBBerdir
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,896 pass(es), 3 fail(s), and 5 exception(s).
[ View ]
#187 term-description-569434-187.patch33.72 KBBTMash
FAILED: [[SimpleTest]]: [MySQL] 64,401 pass(es), 16 fail(s), and 0 exception(s).
[ View ]
#187 term-description-569434-interdiff.txt92 bytesBTMash
#184 term-description-569434-184-interdiff.txt2.88 KBBerdir
#184 term-description-569434-184.patch33.72 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 64,402 pass(es), 20 fail(s), and 1 exception(s).
[ View ]
#180 interdiff.txt14.84 KBpfrenssen
#180 term-description-569434-178.patch32.23 KBpfrenssen
FAILED: [[SimpleTest]]: [MySQL] 63,087 pass(es), 1 fail(s), and 2 exception(s).
[ View ]
#177 interdiff.txt10.94 KBpfrenssen
#177 term-description-569434-177.patch17.68 KBpfrenssen

Comments

This would allow for translation of terms in core when translatable fields fully lands - which ought to let us deprecate some or all of i18n_taxonomy.module in contrib.

See also #557292: TF #3: Convert node title to fields for node title.

Version:7.x-dev» 8.x-dev

Drupal 8.

#553326: Provide consistency to the way field UI is presented adds a 'format' column to taxonomy_term_data...

Title:Convert taxonomy term subject and description to fieldsConvert taxonomy term description to a normal field

Marked #1233032: Taxonomy Text Processing as duplicate of this issue.

Status:Active» Needs review
StatusFileSize
new14.79 KB
FAILED: [[SimpleTest]]: [MySQL] 33,891 pass(es), 19 fail(s), and 5 exception(s).
[ View ]

This patch adds a taxonomy_term_description field and instances to existing vocabularies. It removes all the code relating to term description being a custom field and removes the columns from {taxonomy_term_data}.

It doesn't add description field to new vocabularies when they are created. I'd argue that most vocabularies don't need the description field.

Status:Needs work» Needs review
StatusFileSize
new19.54 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 569434-11.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Worked through some failed tests.

Status:Needs work» Needs review
StatusFileSize
new19.54 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 569434-13.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Typo

I don't know what the deal is with the upgrade tests failing and could use some help.

Status:Needs work» Needs review
StatusFileSize
new19 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 569434-18.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Reroll.

+++ b/core/modules/taxonomy/taxonomy-term.tpl.phpundefined
@@ -6,11 +6,10 @@
+ * - $content: An array of items for the content of the term (fields). Use ¶
+ *   render($content) to print them all, or print a subset such as ¶
+ *   render($content['field_example']). Use hide($content['field_example']) ¶

Trailing whitespace makes the bot fail? Has it always done it?

It's not the trailing whitespace; it's that the patch does not apply, as far as I can see?

Status:Needs work» Needs review
StatusFileSize
new18.97 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/path/path.test.
[ View ]

I think it was just the whitespace that made it not apply. Geez..

Status:Needs work» Needs review
StatusFileSize
new18.98 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 569434-22_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Status:Needs work» Needs review
StatusFileSize
new18.98 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/taxonomy/taxonomy.install.
[ View ]

Status:Needs work» Needs review
StatusFileSize
new18.98 KB
FAILED: [[SimpleTest]]: [MySQL] 36,461 pass(es), 63 fail(s), and 33 exception(s).
[ View ]

Status:Needs work» Needs review
StatusFileSize
new19.38 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PDO Exception detected during run_tests.sh. See review log for details..
[ View ]

Lovely patch.

It looks like we're missing some code for the Standard profile that sets up a Description field for taxo terms by default?

If so, #1292470: Convert user pictures to Image Field contains some similar code that could possibly be used as template.

+++ b/core/modules/taxonomy/taxonomy.install
@@ -300,3 +287,78 @@ function taxonomy_update_8001() {
+  $vocabularies = taxonomy_vocabulary_load_multiple(FALSE);
...
+    $term = taxonomy_term_load($term->tid);
...
+    taxonomy_term_save($term);

You must not use API functions (which invoke module hooks) in module updates. Use a direct db query instead.

+++ b/core/modules/taxonomy/taxonomy.install
@@ -300,3 +287,78 @@ function taxonomy_update_8001() {
+    field_create_field($field);
...
+      field_create_instance($instance);

Ditto for these - however, field.install provides stub replacement functions for module updates which you can use instead.

Status:Needs work» Needs review
StatusFileSize
new21.38 KB
FAILED: [[SimpleTest]]: [MySQL] 36,558 pass(es), 11 fail(s), and 12 exception(s).
[ View ]

Changes per sun's review.

Status:Needs work» Needs review
StatusFileSize
new21.55 KB
FAILED: [[SimpleTest]]: [MySQL] 36,492 pass(es), 62 fail(s), and 41 exception(s).
[ View ]

Status:Needs work» Needs review
StatusFileSize
new21.56 KB
FAILED: [[SimpleTest]]: [MySQL] 36,466 pass(es), 62 fail(s), and 32 exception(s).
[ View ]

StatusFileSize
new21.47 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 569434-39.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Here's some progress to passing the upgrade tests. There's still a language/langcode problem that may need some hook_update_dependencies work.

This is a tip for users who would need this field right now :

Awaiting for a fix in 7.x or 8.x this is an easy workaround for this issue.
You can just create a custom long_text description field (field_description if you want to use the code below) and use a tiny module to hide the built-in one.

<?php
/**
* Implements hook_form_[form-name]_alter().
*/
function MY_MODULE_form_taxonomy_form_term_alter(&$form, &$form_state, $form_id) {
  if (!empty(
$form['field_description'])) {
   
$form['description']['#access'] = FALSE;
  }
}
?>

Or use the Title module. Ammitedly the project name is a bit misleading...

Status:Needs work» Needs review
StatusFileSize
new23.01 KB
FAILED: [[SimpleTest]]: [MySQL] 42,576 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

Hopefully this one passes. I added the same change to the langcode update in field_sql_storage.install as being used in #1292470: Convert user pictures to Image Field to get my tests to pass.

Status:Needs work» Needs review

I rerolled for move of the path test file.

I couldn't reproduce the Database test case failure and am still waiting for the language upgrade test to complete locally.

StatusFileSize
new23.04 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

I rerolled for move of the path test file.

I couldn't reproduce the Database test case failure and am still waiting for the language upgrade test to complete locally.

I'm trying to debug what is going on (might need to pick up on it again tomorrow), but the behavior I see is that it is stuck in the sandbox when taxonomy_term_data is empty. But the strange part is that taxonomy_term_data does have data as far as I can see from the generated dumps. So there is some very strange stuff going on.

Also...I see the field is explicity in field_sql_storage. I know the vast majority of the sites are in a relational DB...but is this a safe assumption to make?

OK, I think the problem is that in the D7 testing db all but one of the taxonomy terms have a vid of 0 (which I find odd) and as a result taxonomy_update_8003 never completes as the max and the terms are using different queries.

We need to have bundles for the terms. We need to find out if this is a data problem in the testing db or if it's actually valid to have 0 for vids. We also need to make the queries in 8003 match regardless in case other people do have orphaned terms.

We should be looking at the generate-d7-script in that case:

  $voc_id = 0;
  $term_id = 0;
  for ($i = 0; $i < 24; $i++) {
    $vocabulary = new stdClass;
    ++$voc_id;
    $vocabulary->name = "vocabulary $voc_id (i=$i)";
    $vocabulary->machine_name = 'vocabulary_' . $voc_id . '_' . $i;
    $vocabulary->description = "description of ". $vocabulary->name;
    $vocabulary->multiple = $multiple[$i % 12];
    $vocabulary->required = $required[$i % 12];
    $vocabulary->relations = 1;
    $vocabulary->hierarchy = $hierarchy[$i % 12];
    $vocabulary->weight = $i;
    taxonomy_vocabulary_save($vocabulary);
    ...
  }

It *seems* like when the vocab gets saved, it would automatically also be saving a higher vid. The vocab gets created properly. It seems then that the problem is with the terms being created. I see a line $term->vocabulary_machine_name = $vocabulary->machine_name;. But I do not see a vid get set. And looking at http://api.drupal.org/api/drupal/modules!taxonomy!taxonomy.module/functi..., we need to set a vid (the vocab name is optional and gets set by vid, not the other way around). So we need to rebuild the dump...argh :(

I'm going to check if there are other issues that have regenerated dumps or issues are going to get stuck in some nasty holes.

Status:Needs work» Needs review
StatusFileSize
new23.05 KB
PASSED: [[SimpleTest]]: [MySQL] 36,810 pass(es).
[ View ]

I fixed taxonomy_update_8003 so it won't get stuck. Why the d7 database has all those 0's for vid I don't know though.

Thanks BTMAsh, yeah looks like a mistake in creating those terms.

Issue tags:+Needs tests
StatusFileSize
new23.69 KB
PASSED: [[SimpleTest]]: [MySQL] 36,835 pass(es).
[ View ]

This should also have upgrade tests, I think.

Here's a reroll for the PSR-0 taxonomy tests.

StatusFileSize
new23.65 KB
PASSED: [[SimpleTest]]: [MySQL] 37,284 pass(es).
[ View ]

Status:Needs review» Needs work

Okay, CNW for the upgrade tests.

I'll take a look into what needs to be done re: the upgrade tests. I *believe* a new dump was created from another issue so this is hopefully no longer a problem. With that said, I am concerned about the description utilizing field_sql_storage for d8. Talking with @tim.plunkett about this, I have created #1696802: Upgrade concerns for fields from 7.x -> 8.x that utilize non-sql storage. to figure out how creating new fields for entities should be handled.

Talking with @chx, my issues surrounding field_sql_storage is not an issue. So don't have to worry about that and just need to look into tests.

Status:Needs work» Needs review
StatusFileSize
new23.89 KB
PASSED: [[SimpleTest]]: [MySQL] 40,556 pass(es).
[ View ]

Rerolled due to some config changes and moving the add term form code to TermFormController.php. This was the most complicated reroll I've done so far. I'm not certain I got it right.

StatusFileSize
new23.25 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch taxonomy-description-field-569434-60.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Rerolled #59.

I also added the 'description' element to the field instance array in taxonomy_update_8003(). Without it, there's an error caused by field_multiple_value_form() when the add term page is accessed. field_multiple_value_form() assumes the description is set, probably because field_create_instance() sets a default value 'description' => ''. _update_7000_field_create_instance() does not set a default.

#52:

...Why the d7 database has all those 0's for vid I don't know though.

Do we have a todo/follow-up for this?

Assigned:Unassigned» BTMash

Based on #57, the 0s for vid should have been taken care of. But this patch still needs tests. I've been out of the loop on a number of things Drupal related (and entirely my fault) but should be able to dedicate more and more time again soon. I'm assigning it to myself for now (hopefully this gets me to commit to figuring out tests) and if I don't do so myself, someone can unassign me if I don't answer back before September 14.

Status:Needs work» Needs review
Issue tags:-Needs tests, -Needs reroll+API clean-up, +Field system
StatusFileSize
new23.13 KB
FAILED: [[SimpleTest]]: [MySQL] 46,182 pass(es), 0 fail(s), and 1 exception(s).
[ View ]

Re-rolled against HEAD.

Also turned into a branch in the Platform sandbox.

Status:Needs work» Needs review
StatusFileSize
new748 bytes
new23.86 KB
PASSED: [[SimpleTest]]: [MySQL] 47,553 pass(es).
[ View ]

Removed the {taxonomy_term_data}.description definition from taxonomy_views_data.

Issue tags:+Needs tests

There is an upgrade path, but no explicit tests for it.

StatusFileSize
new2.62 KB
new26.33 KB
FAILED: [[SimpleTest]]: [MySQL] 47,579 pass(es), 48 fail(s), and 0 exception(s).
[ View ]

I'm still trying to work out the test and hit a snag with taxonomy_load_term($tid) and not yet sure why. I'm attaching the combined test and interdiff that contains just the test so hopefully we can debug it (the test fails though not it is not for what it should be getting tested on).

Going through this...the terms somehow remain but the vocabulary ids are set to 0 so those terms are not loaded (ie, taxonomy term upgrade is effectivey not being tested). So really...we need a new filled db dump from D7 to test this.

Status:Needs work» Needs review
StatusFileSize
new69.84 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 569434_1.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

For this patch, I generated a new standard_all.filled.php.gz file and added in my tests if someone want to review it. I have also updated the script that would be used for generating the filled dump (though for this, I unzipped the gzip file, edited the taxonomy term data table so the vids would be appropriate to get it working correctly).

Assigned:Unassigned» BTMash

Upgrade path tests have already been added to the patch - however, it does require a (not-so-easy) reroll.

Assigning to self.

Status:Needs work» Needs review
StatusFileSize
new68.51 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 569434_2.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new1.45 KB

The field_sql_storage_update_8000() changes seem to have made it in via another patch. So I removed that portion from this patch.

I reviewed the patch and the code looks clean to me, for what it's worth. The only thing I noticed is the comment below, which has two spaces after the period, when there should only be one.

--- a/core/profiles/standard/standard.install
+++ b/core/profiles/standard/standard.install
@@ -253,7 +253,7 @@ function standard_install() {
   $user_settings = config('user.settings');
   $user_settings->set('register', USER_REGISTER_VISITORS_ADMINISTRATIVE_APPROVAL)->save();
-  // Create a default vocabulary named "Tags", enabled for the 'article' content type.
+  // Create a default vocabulary named "Tags", enabled for the 'article' content type.  Include a description field on terms for "Tags".
   $description = st('Use tags to group articles on similar topics into categories.');
   $help = st('Enter a comma-separated list of words to describe your content.');
   $vocabulary = entity_create('taxonomy_vocabulary', array(

Sorry I can't help with the testbot errors.

Status:Needs work» Needs review
StatusFileSize
new67.73 KB
FAILED: [[SimpleTest]]: [MySQL] 52,272 pass(es), 6 fail(s), and 4 exception(s).
[ View ]

Attempted reroll.

Assigned:Unassigned» tim.plunkett

Needs a major reroll.

Status:Needs work» Needs review

Rerolled #86. This was the first time I've tried to convert something to CMI, the description field on the tags vocabulary in the standard profile, so someone needs to check it. I ran a test installation and the description field was added with no problem. Hopefully it will check out.

StatusFileSize
new64.93 KB
FAILED: [[SimpleTest]]: [MySQL] 55,411 pass(es), 3 fail(s), and 3 exception(s).
[ View ]

Attaching the patch always helps.

Status:Needs work» Needs review
StatusFileSize
new64.5 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 569434-92-taxonomy-description.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new2.31 KB

The errors in the field_ui test are the result of the drupalPost() request assuming that the field ui form and its Save button existing, but they don't without any field. It must have previously just assumed that the description would be there. Since it doesn't now, a simple test field can be added to make sure the test passes.

It looks like the errors in the path test were just the result of an incorrect variable name.

Status:Needs work» Needs review
StatusFileSize
new64.82 KB
FAILED: [[SimpleTest]]: [MySQL] 55,873 pass(es), 9 fail(s), and 0 exception(s).
[ View ]

Rerolled #92.

Status:Needs work» Needs review
StatusFileSize
new66.21 KB
PASSED: [[SimpleTest]]: [MySQL] 55,611 pass(es).
[ View ]

Well... I managed to screw up that reroll in #97, not to mention #92 where I forgot to make sure the tags description field and instance configurations were added. Let's try this again.

Status:Needs review» Needs work

+++ b/core/modules/taxonomy/taxonomy.installundefined
@@ -335,3 +323,93 @@ function taxonomy_update_8006() {
+/**
+ * Remove {term_data}.description and {term_data}.format.
+ */
+function taxonomy_update_8009() {
+  db_drop_field('taxonomy_term_data', 'description');
+  db_drop_field('taxonomy_term_data', 'format');

This update hook should be removed, and the tables added to the list here: #1860986: Drop left-over tables from 8.x

Otherwise, I think this patch is done!

Status:Needs work» Reviewed & tested by the community

Oh hm, nvm, those are fields not tables!

Status:Reviewed & tested by the community» Needs work

Needs a reroll...

curl https://drupal.org/files/569434-99-taxonomy-description.patch | git apply
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 67801  100 67801    0     0  35053      0  0:00:01  0:00:01 --:--:-- 38457
<stdin>:229: trailing whitespace.
*f*A�e���3HS�y�\��=�}1/�JS"�.
                             �t��L$?d8�wɜN��a� �S��1�(���ޙ����%ߘm�\�[�A5
                                                                         ue�0ڢ�c�
error: patch failed: core/modules/field_ui/lib/Drupal/field_ui/Tests/ManageDisplayTest.php:192
error: core/modules/field_ui/lib/Drupal/field_ui/Tests/ManageDisplayTest.php: patch does not apply
error: patch failed: core/modules/path/lib/Drupal/path/Tests/PathTaxonomyTermTest.php:48
error: core/modules/path/lib/Drupal/path/Tests/PathTaxonomyTermTest.php: patch does not apply

Because reviewing core/modules/system/tests/upgrade/drupal-7.filled.standard_all.database.php.gz is difficult due to the fact it's compressed I'm uploading a diff of the uncompressed file.

Status:Needs work» Needs review
StatusFileSize
new66.21 KB
PASSED: [[SimpleTest]]: [MySQL] 55,607 pass(es).
[ View ]

Conflicted with #1978112: Convert taxonomy admin path to follow other core entity patterns

And yes, the database dump was changed because the VIDs were wrong, see #73 and #75

This looks great, the only thing that I find a bit strange is that we're using the deprecated taxonomy_vocabulary table for the upgrade path and not the config files, is there a reason for that?

Because this was written before that. Didn't even notice. Not sure if it matters...

+++ b/core/modules/taxonomy/taxonomy.installundefined
@@ -335,3 +323,93 @@ function taxonomy_update_8006() {
+  $vocabularies = db_query("SELECT machine_name FROM {taxonomy_vocabulary}")->fetchCol();

I agree with @berdir... this should be get this from the config entities. Whilst logically it should make no difference... once taxonomy_update_8005() has run we shouldn't be using the taxonomy_vocabulary anymore.

berdir pointed this out in IRC:

just sounds like that could result in interesting inconsistencies if e.g. aggregator.module categories do get converted to taxonomy and happen to run first or so

Who knows what else might become terms, why not convert the data we know is the correct data?

I actually meant this to be a reason for using the config files and not the database :)

Once 7005 did run, the config files are the right data, the database tables are just some left-overs in case someone still needs something from them.

Status:Needs review» Needs work

Oh, okay.

Assigned:tim.plunkett» Unassigned
StatusFileSize
new985 bytes
new66.27 KB
PASSED: [[SimpleTest]]: [MySQL] 55,552 pass(es).
[ View ]

I'm not sure how to rewrite this query:
$terms = db_query_range('SELECT t.tid, t.description, t.format, v.machine_name FROM {taxonomy_term_data} t INNER JOIN {taxonomy_vocabulary} v ON t.vid = v.vid WHERE tid > :tid ORDER BY tid ASC', 0, 100, array(':tid' => $sandbox['current_tid']));

I thought we weren't supposed to read/write CMI directly in update hooks, but I'm not sure.

Status:Needs work» Needs review
StatusFileSize
new69.97 KB
FAILED: [[SimpleTest]]: [MySQL] 55,874 pass(es), 5 fail(s), and 1 exception(s).
[ View ]
new71.72 KB
FAILED: [[SimpleTest]]: [MySQL] 55,989 pass(es), 0 fail(s), and 38 exception(s).
[ View ]
new7.14 KB

The query is actually easy to convert, we can just get rid of the join :)

The added tests proved nicely that the current query is in fact broken because t.vid is now the machine name, so that query never returned anything :) Yay tests.

Status:Needs work» Needs review
StatusFileSize
new6.6 KB
new76.45 KB
FAILED: [[SimpleTest]]: [MySQL] 55,665 pass(es), 1 fail(s), and 1 exception(s).
[ View ]

Oh, fun. Removing the properties shows that we still have quite some references left. Noticed that forum edit forms are not integrated with the field system at all, so it doesn't show any fields. But I don't think we can possibly be made responsible for that here, they are huge mess anyway.

Also added an explicit update function dependency so that this is run before field cmi conversion like e.g. the user picture. There's an existing field upgrade issue about when it should happen which will probably have to re-shuffle this.

StatusFileSize
new3.16 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
new78.71 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch term-description-569434-116.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Uh, that was a ride through update function dependencies hell, always fun ;) And it would be so many times worse without upgrade path tests...

Quite a list of changes were necessary, will explain in the next comment :)

Status:Needs work» Needs review

+++ b/core/modules/contact/contact.installundefined
@@ -29,7 +29,7 @@ function contact_install() {
   $dependencies['contact'][8003] = array(
-    'user' => 8011,
+    'user' => 8015,
+++ b/core/modules/overlay/overlay.installundefined
@@ -24,7 +24,7 @@ function overlay_enable() {
   $dependencies['overlay'][8000] = array(
-    'user' => 8011,
+    'user' => 8015,

We (was me I think), forgot to update the dependencies for the user_data update function as the user picture issue got in first. 8011 is that, 8015 is the users_data stuff. Happened to work before, the other added dependencies then caused this to to be moved around and only follow the explicit dependency.

+++ b/core/modules/field/field.installundefined
@@ -232,7 +232,7 @@ function _update_7000_field_create_instance($field, &$instance) {
-  $dependencies['field'][8003] = array(
+  $dependencies['field'][8002] = array(

The user picture upgrade path also changes the member_for extra field setting, which is converted to entity display in 8002. Same story as above...

Yay for my test :p

+++ b/core/modules/user/user.installundefined
@@ -373,6 +373,18 @@ function user_install_picture_field() {
+  // Make sure that the uuid column is added to {file_managed} before the
+  // user picture default image is added.
+  $dependencies['user'][8011] = array(
+    'system' => 8015,

Speaks for itself. Another thing that implicitly worked but had no explicit dependency but now needs it.

@swentel mentioned that overlay/contact should probably depend on user 8016 as that is the last update function that does something with users_data and that makes logically more sense.

StatusFileSize
new953 bytes
new78.65 KB
PASSED: [[SimpleTest]]: [MySQL] 55,920 pass(es).
[ View ]

Re-roll.

+++ b/core/modules/taxonomy/taxonomy.installundefined
@@ -335,3 +334,109 @@ function taxonomy_update_8006() {
+      $vocabulary = substr($vocabulary, strlen('taxonomy.vocabulary.'));

drupal_strlen?

Should the field use the hidden widget by default?
Fresh install with this patch gives me no description field in the add term form.
Looking at manage fields, I see it is hidden.
Must mean we're missing tests for standard.profile.
Screen Shot 2013-05-24 at 8.03.00 AM.pngScreen Shot 2013-05-24 at 8.03.16 AM.png

http://sad1969796e10317.s3.simplytest.me/ admin/power

StatusFileSize
new1.28 KB
new80.28 KB
FAILED: [[SimpleTest]]: [MySQL] 56,561 pass(es), 25 fail(s), and 25 exception(s).
[ View ]

Re-roll, used drupal_strlen() (although it doesn't really matter on a machine name), and added the widget configuration.

Lots of changes in upgrade path, let's see what happens.

Status:Needs work» Needs review
StatusFileSize
new2.37 KB
new80.41 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch term-description-569434-131.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Fixing the upgrade path.

What I don't understand is why only the tags vocabulary gets a hardcoded description field, provided by standard.profile. Why not do it the same way as nodes with the body field and add it by default for new vocabularies with a helper function?

Absolutely agree with #131.

Suppose tags does not need description at all, but others could need. But once this is a conversion we need helper

Status:Needs work» Needs review
Issue tags:-Fields in Core, -translatable fields, -Field system

With D8's new Entity and Field APIs, base/nonconfigurable fields are not any less "normal" than configurable fields. Nor are they less translatable. So, retitling and removing irrelevant tags. I still think this issue makes sense though, since I see no reason for term description to be any less configurable than node body or comment body.

Status:Needs review» Needs work
Issue tags:+Fields in Core, +translatable fields, +Field system

We'd need an API change approval for this but I agree that it would be still nice to have. For example, many term vocabularies don't need it and it would mean that for example the editor.module image upload/file usage stuff would also work for it, which it right now doesn't.

The RDF module also cannot annotate the term description in a consistent manner, that's why streamlining this would help avoid custom ugly workarounds.

the editor.module image upload/file usage stuff would also work for it, which it right now doesn't

The RDF module also cannot annotate the term description in a consistent manner

Both of those problems will be fixed when #2010930: [META] Apply formatters and widgets to rendered entity base fields (all besides node.title) is done. But you're correct in those being limitations of nonconfigurable fields until that issue is done.

Not really, because right now, there are two separate fields (description and format) and widgets/formatters work on a single field. And editor.module looks for a field with a format (hook_entity_*() based, not as a widget). Not exactly sure what rdf.module does and how, but it's probably similar.

So to make widgets/formatters and those modules work for this, we'd have to combine it into a single, non-configurable text with format field, which does not exist right now, nor is the current storage implementation capable of handling that (it has been discussed to support this but not implemented).

And if we would make all that work, we'd end up with the same API change ($term->description->value/$term->format->value => $term->description->value/$term->description->format), the only difference being that such a description would be non-configurable and always exist.

Issue tags:+Needs reroll

Patch needs reroll.

Do we still need to worry about upgrade paths in this patch? It seems like the import api in core might mean that we don't need to focus on that anymore.

Assigned:Unassigned» Gaelan

Rerollin'

Status:Needs work» Needs review
StatusFileSize
new1.97 KB
new542 bytes
new50.54 KB
FAILED: [[SimpleTest]]: [MySQL] 59,114 pass(es), 66 fail(s), and 199 exception(s).
[ View ]

Hopefully I'm not stepping on toes. It looked like it had been a little while since this had been assigned.

Reroll is attached.

I took some notes about what I was doing during the reroll. I'm attaching those as well in case that's useful.

Issue tags:-Needs tests, -Field system, -Needs reroll+Field API
StatusFileSize
new12.84 KB
new34.84 KB
PASSED: [[SimpleTest]]: [MySQL] 60,005 pass(es).
[ View ]

This was a hard one to get right. I've had to change a lot of things to make it work nicely with current HEAD.

StatusFileSize
new2.66 KB
new32.73 KB
PASSED: [[SimpleTest]]: [MySQL] 59,949 pass(es).
[ View ]

Some redundant stuff

The description field is still only hardcoded for the tags vocabulary, as mentioned by @berdir in #131. I'll investigate the helper function based on what node is doing for the body field and post back.

We discussed this a while ago in IRC with @catch. He said that we can a) still do this and b) don't need a default description on all vocabularies.

What he couldn't say is whether tags should have a description or not.

I personally think that if we don't add it by default, we shouldn't add it to tags, they're mostly only used for auto-complete/the term name.

So we need to find someone who can decide this and then we should be ready to move on here.

Also discussed this a bit with @alexpott, he agrees that there's not much point in having a default description for tags.

I'd suggest to provide a patch that doesn't contain the default field configuration so that it can be tried out and then possibly confirm with an UX person or maybe webchick?

Issue summary:View changes
StatusFileSize
new35.08 KB
FAILED: [[SimpleTest]]: [MySQL] 59,427 pass(es), 0 fail(s), and 1 exception(s).
[ View ]
new2.55 KB

Okay, this adds a helper function to add the description field instance to new taxonomy vocabularies, the same way node adds the body field with node_add_body_field.

I noticed that the tags field instance created on install doesn't display the description field. I didn't have time to include that in this patch. Then again, I noticed that there is no configuration file for the node body instance in the standard profile so maybe if we use the helper function then we don't need the description field configuration files?

Assigned:Gaelan» xjm

@Berdir asked me to make a decision on IRC about tags terms and whether or not they need to have a description field.

I'm assigning the issue to xjm as the component maintainer.

+++ b/core/profiles/standard/standard.install
@@ -86,4 +86,11 @@ function standard_install() {
+  // Configure the widget for the taxonomy term description field.
+  entity_get_form_display('taxonomy_term', 'tags', 'default')
+    ->setComponent('taxonomy_term_description', array(
+      'type' => 'text_textarea',
+    ))
+    ->save();

Why is this not just default config in the standard profile?

Whoops, looks like I might have moved too early on working on it. see #'s 152 and 153 before reviewing 154.

Status:Needs review» Needs work

The last submitted patch, 154: term-description-569434-152.patch, failed testing.

@xjm:

We have basically three options:
1. Default configuration for description for tags but not for new vocabulary like body field (green patch in #150).
2. Auto-create description field similar to node body (Almost passing patch in #154, would just need a dependency on text.module in that test).
3. No default fields, create yourself if you need it. (No patch yet, but basically #150 minus the default configuration).

As mentioned before, I don't think we need 2 as using a term description is way less common than a node body, 1 seems to be inconsistent and tags usually don't need descriptions, so I'd prefer 3 :)

  1. +++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Entity/Vocabulary.php
    @@ -106,6 +106,7 @@ public function postSave(EntityStorageControllerInterface $storage_controller, $
    +      taxonomy_add_description_field($this, 'Description');
    +++ b/core/modules/taxonomy/taxonomy.module
    @@ -1199,3 +1187,54 @@ function taxonomy_library_info() {
    + * Adds the default description field to a taxonomy term type.
    ...
    +function taxonomy_add_description_field(VocabularyInterface $type, $label = 'Description') {

    I think better to have a public method on Vocabulary entity for that

  2. +++ b/core/modules/taxonomy/taxonomy.install
    @@ -396,3 +398,112 @@ function taxonomy_update_8008() {
    +  $terms = db_query_range('SELECT t.tid, t.description, t.format, t.vid FROM {taxonomy_term_data} t WHERE tid > :tid ORDER BY tid ASC', 0, 100, array(':tid' => $sandbox['current_tid']));
    ...
    +  $data = db_insert('taxonomy_term__description')
    ...
    +  $revision = db_insert('taxonomy_term_revision__description')
    ...
    +  $data->execute();
    +  $revision->execute();

    This broken when no terms exists

  3. +++ b/core/profiles/standard/standard.install
    @@ -86,4 +86,11 @@ function standard_install() {
    +  // Configure the widget for the taxonomy term description field.
    +  entity_get_form_display('taxonomy_term', 'tags', 'default')

    this could be a config file as well

+++ b/core/modules/forum/forum.module
@@ -677,7 +677,6 @@ function template_preprocess_forum_list(&$variables) {
-    $variables['forums'][$id]->description = filter_xss_admin($forum->description->value);
+++ b/core/modules/forum/templates/forum-list.html.twig
@@ -15,8 +15,6 @@
- *   - description: The description field for the forum, containing:
- *     - value: The descriptive text for the forum.
@@ -59,9 +57,6 @@
-          {% if forum.description.value %}
-            <div class="description">{{ forum.description.value }}</div>
-          {% endif %}

Forum vocabulary have description and uses the one

Status:Needs work» Needs review
StatusFileSize
new31.92 KB
FAILED: [[SimpleTest]]: [MySQL] 59,658 pass(es), 0 fail(s), and 1 exception(s).
[ View ]

Here's a reroll that also takes into account 159.2, wasn't sure if the function should be a method of a vocabulary. For what I understand, the rest of things are still under discussion.
Removed the upgradepathtest from the patch as for #2134951: Remove upgrade path tests

Status:Needs review» Needs work

The last submitted patch, 162: term-description-569434-162.patch, failed testing.

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 162: term-description-569434-162.patch, failed testing.

Removing the call to taxonomy_add_description_field() that the patch adds to line 107 of core/modules/taxonomy/lib/Drupal/taxonomy/Entity/Vocabulary.php will get rid of this exception in the testViewModeCustom() test. That will also cause 10 other failures when running this test.

Related issues:
StatusFileSize
new714 bytes
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
new31.22 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

I realized that I had made some other changes in the test that were causing the failures. Attached is the patch without the call to taxonomy_add_description_field(). I haven't done a visual test yet, but I figured I'd post the updated patch.

Status:Needs work» Needs review

So the fix from #166 and #167 is probably not the right way to fix this, since that function actually adds an instance of the description field to a vocabulary's taxonomy terms and adds the field to the form and display.

Ok, here is what I would suggest to do.

- Drop description
- Do not add a description field by default
- Do not add it for tags
- Do add a fixed, locked description for forum terms, in forum.module, so that the templates and code there can rely on it.

Additionally, what we can do anyway is:
- Drop all upgrade path related changes, including tests and test data.

Assigned:xjm» Unassigned

Alrighty, I've been convinced. Let's do everything in #170.

P.S. -- Sorry for the prolonged wintry silence. :)

The last submitted patch, 167: interdiff.patch, failed testing.

Title:Convert taxonomy term description to a configurable fieldRemove taxonomy term description field; provide description field for forum taxonomy
Issue summary:View changes

Re-scoping. There's now some duplicates of this somewhere about that we should close as such.

Issue summary:View changes

Assigned:Unassigned» pfrenssen
Status:Needs review» Needs work

Going to work on this.

Reviewing the patch while getting familiar with the issue:

--- a/core/modules/taxonomy/taxonomy.install
+++ b/core/modules/taxonomy/taxonomy.install
@@ -7,6 +7,20 @@
use Drupal\Core\Entity\FieldableDatabaseStorageController;
use Drupal\field\Entity\Field;
+use Drupal\Component\Uuid\Uuid;
+use Drupal\Core\Language\Language;
+
+/**
+ * Implements hook_uninstall().
+ */
+function taxonomy_uninstall() {
+  // Remove taxonomy_term bundles.
+  $config_names = config_get_storage_names_with_prefix('taxonomy.vocabulary.');
+  foreach ($config_names as $config_name) {
+    $vid = substr($config_name, strlen('taxonomy.vocabulary.'));
+    entity_invoke_bundle_hook('delete', 'taxonomy_term', $vid);
+  }
+}

What is this doing here? Is this needed in scope of this issue?

--- a/core/scripts/generate-d7-content.sh
+++ b/core/scripts/generate-d7-content.sh
@@ -141,6 +141,7 @@
   for ($j = 0; $j < $vocabulary->hierarchy + 1; $j++) {
     ++$term_id;
     $term = entity_create('taxonomy_term', array(
+      'vid' => $voc_id,
       'vocabulary_machine_name' => $vocabulary->machine_name,
       // For multiple parent vocabularies, omit the t0-t1 relation, otherwise
       // every parent in the vocabulary is a parent.

This also seems to be out of scope. If this script fails without this line then it can be addressed in a separate issue.

My next step is to roll a new patch that drops all the changes to the forum module.

StatusFileSize
new17.68 KB
new10.94 KB

Updated patch. This drops all the changes to the forum module, the update hooks and everything else that seems to be unnecessary for the new scope.

Next step is to add the description field to the forum vocabulary when the forum module is installed.

The last submitted patch, 167: term-description-569434-167.patch, failed testing.

I know you just reverted the changes, but here are some hints that you will need to consider when changing it to a field.

  1. +++ b/core/modules/forum/forum.module
    @@ -676,6 +676,7 @@ function template_preprocess_forum_list(&$variables) {
       $row = 0;
       // Sanitize each forum so that the template can safely print the data.
       foreach ($variables['forums'] as $id => $forum) {
    +    $variables['forums'][$id]->description = filter_xss_admin($forum->description->value);
         $variables['forums'][$id]->link = url("forum/" . $forum->id());
         $variables['forums'][$id]->name = check_plain($forum->label());

    This is strange, what it does is updating the original value of the description with the filtered one.

    This should be dropped and $forum->description->processed be used instead in the template, or the rendered field output.

  2. +++ b/core/modules/forum/lib/Drupal/forum/Tests/ForumTest.php
    @@ -363,11 +363,13 @@ function editForumVocabulary() {
        */
       function createForum($type, $parent = 0) {
    -    // Generate a random name.
    +    // Generate a random name/description.
         $name = $this->randomName(10);
    +    $description = $this->randomName(100);
         $edit = array(
           'name' => $name,
    +      'description[value]' => $description,
           'parent[0]' => $parent,

    This will need to be updated, as the field will be description[0][value] in the form.

  3. +++ b/core/modules/forum/lib/Drupal/forum/Tests/ForumTest.php
    @@ -385,7 +387,7 @@ function createForum($type, $parent = 0) {
         // Verify forum.
    -    $term = db_query("SELECT * FROM {taxonomy_term_data} t WHERE t.vid = :vid AND t.name = :name", array(':vid' => \Drupal::config('forum.settings')->get('vocabulary'), ':name' => $name))->fetchAssoc();
    +    $term = db_query("SELECT * FROM {taxonomy_term_data} t WHERE t.vid = :vid AND t.name = :name AND t.description__value = :desc", array(':vid' => \Drupal::config('forum.settings')->get('vocabulary'), ':name' => $name, ':desc' => $description))->fetchAssoc();

    This revert can be reverted, as the description will then be in a field table. Or load it and then verify.

  4. +++ b/core/modules/forum/templates/forum-list.html.twig
    @@ -15,6 +15,8 @@
      *   - link: The URL to link to this forum.
    + *   - description: The description field for the forum, containing:
    + *     - value: The descriptive text for the forum.
      *   - new_topics: A flag indicating if the forum contains unread posts.
    @@ -57,6 +59,9 @@
               <div class="name"><a href="{{ forum.link }}">{{ forum.label }}</a></div>
    +          {% if forum.description.value %}
    +            <div class="description">{{ forum.description.value }}</div>
    +          {% endif %}
             {% for i in 1..forum.depth if forum.depth > 0 %}</div>{% endfor %}

    See above, we should document/use processed here, not value.

  5. +++ b/core/modules/taxonomy/taxonomy.install
    @@ -7,20 +7,6 @@
    -/**
    - * Implements hook_uninstall().
    - */
    -function taxonomy_uninstall() {
    -  // Remove taxonomy_term bundles.
    -  $config_names = config_get_storage_names_with_prefix('taxonomy.vocabulary.');
    -  foreach ($config_names as $config_name) {
    -    $vid = substr($config_name, strlen('taxonomy.vocabulary.'));
    -    entity_invoke_bundle_hook('delete', 'taxonomy_term', $vid);
    -  }

    If we remove it then we should open a separate issue for this.

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Entity/Term.php
@@ -81,20 +81,6 @@ class Term extends ContentEntityBase implements TermInterface {
   /**
-   * Description of the term.
-   *
-   * @var \Drupal\Core\Field\FieldItemListInterface
-   */
-  public $description;
-
-  /**
-   * The text format name for the term's description.
-   *
-   * @var \Drupal\Core\Field\FieldItemListInterface
-   */
-  public $format;
-
-  /**

More important than this is removing the definitions in baseFieldDefinitions()

Status:Needs work» Needs review
StatusFileSize
new32.23 KB
FAILED: [[SimpleTest]]: [MySQL] 63,087 pass(es), 1 fail(s), and 2 exception(s).
[ View ]
new14.84 KB

Added the field and updated ForumTest.

I didn't see @berdir's comment above until now, those points still need to be looked at. Alas I'm done for the day. I can probably pick this back up a week from now when I'm doing my core dev week.

Setting to needs review for the bot.

Hm. Reviewing the options in #158 and the suggested plan in #170, I'm a bit worried about alternative use-cases for the term body/description field:

There are a range of contributed modules that were/are able to leverage the existence of a term body field in other ways. For example, the Glossary and similar modules — which essentially use taxonomy terms in their true lexical meaning and as first-class content citizens, so as to output them on their own, instead of using them as a reference and relationship endpoint only.

The problem I see is the lack of a shared dependency. Forum may not be installed, and if it is, it would likely create forum_description field. → Even though that is a typical term body field, (all) other modules will have to re-invent their own term body field.

The same applies to two or more contrib/custom modules; even though they just need a body field, they can't depend on a shared one and need to add their own. As a result, terms will possibly have multiple body fields, because no one can rely on anyone else.

So out of the options in #158, I'd actually recommend to do a mixture of (2) and (3) instead:

  1. No term body field by default.
  2. Introduce a helper function similar to the existing node_add_body_field($bundle, $label), which allows modules to create a (single) term body field on selected bundles. + Make Forum use that function.
  3. For consistency, actually name this field [term_]body internally. The label may or may not stay "Description".

Thoughts?

Status:Needs review» Needs work

The last submitted patch, 180: term-description-569434-178.patch, failed testing.

Issue tags:-Field API+Entity Field API, +Needs reroll

#179 is not addressed

  1. +++ b/core/modules/forum/config/field.field.taxonomy_term.forum_description.yml
    @@ -0,0 +1,13 @@
    +id: taxonomy_term.forum_description
    ...
    +name: forum_description

    term_body! could be shipped with taxonomy module
    and forum will just add its instance

  2. +++ b/core/modules/forum/forum.module
    @@ -676,7 +676,7 @@ function template_preprocess_forum_list(&$variables) {
    -    $variables['forums'][$id]->description = filter_xss_admin($forum->description->value);
    +    $variables['forums'][$id]->description = filter_xss_admin($forum->forum_description->value);

    is not fixed, and template variable too

Status:Needs work» Needs review
StatusFileSize
new33.72 KB
FAILED: [[SimpleTest]]: [MySQL] 64,402 pass(es), 20 fail(s), and 1 exception(s).
[ View ]
new2.88 KB

Re-roll, git apply -3 actually worked.

Looking at my own review.

1. The name is now different as the field is named forum_description, still weird but cleaning that up would require more changes, so leaving for now. Just switching to use ->processed, so that it respects the field settings.
4. The forum description stuff in the forum-list.twig.html could not have worked, I guess we don't have test coverage there.

Everything else looks fine.

#183
1. : I don't think we should ship the field in taxonomy.module, and there's no reason to use body, this is a description, not a body. I like forum_description, it's prefixed with the module name and describes what it is.

2. Yep, that should be fixed now. I think we need some basic test coverage here.

#181: No detailed answer for now, but solving that problem can be as simple as allowing users to select the field that should be used by those modules, then it can be a separate one, the same one, whatever works for the specific use case.

Status:Needs review» Needs work

The last submitted patch, 184: term-description-569434-184.patch, failed testing.

The last submitted patch, 184: term-description-569434-184.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new92 bytes
new33.72 KB
FAILED: [[SimpleTest]]: [MySQL] 64,401 pass(es), 16 fail(s), and 0 exception(s).
[ View ]

Tested out image - that seems to be resolved from the latest git pull. Don't really understand the changes for field ui that are causing the test issues. But I changed the weight and rows from string ('5') to int (5) and that resolved the issues for defaultconfigtest.

Status:Needs review» Needs work

The last submitted patch, 187: term-description-569434-187.patch, failed testing.

Assigned:pfrenssen» Unassigned
Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new31.64 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,896 pass(es), 3 fail(s), and 5 exception(s).
[ View ]
new3.22 KB

Re-roll and fixing that test.

The problem was that the test below was already wrong and pointed to a 404 page but nobody noticed that as the only assertion was an assertNoText().

Status:Needs review» Needs work

The last submitted patch, 189: term-description-569434-189.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new31.92 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,960 pass(es).
[ View ]
new2.51 KB

Fixing tests

RTBC except if the nitpick

+++ b/core/modules/forum/config/field.field.taxonomy_term.forum_description.yml
@@ -0,0 +1,13 @@
+uuid: 19f95967-fe48-4f52-8e36-546303f4d589
+++ b/core/modules/forum/config/field.instance.taxonomy_term.forums.forum_description.yml
@@ -0,0 +1,16 @@
+uuid: 4b8963a1-63f5-48d3-9600-a718cca6ea8f
...
+field_uuid: 19f95967-fe48-4f52-8e36-546303f4d589

suppose this should not be removed