Updated: Comment #171, #242

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.
  • Most likely what we need to do is convert it to a configurable field with the same name and add it to all bundles on existing installations, but we have \Drupal\taxonomy\TermInterface::getDescription(), so terms having a description is part of the API. so likely we need to install this configurable field even for new D9 installations, deprecate it for D10 and then remove the default config. But to not break the API, we'd literally need to *disallow* deleting and always force-enable that field on all vocabularies.

    #242

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

Removed TermInterface methods as follows:
- public function getDescription();
- public function setDescription($description);
- public function getFormat();
- public function setFormat($format);

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because not broken as is
Issue priority Not critical because works as is
Prioritized changes This is not a prioritized change for the beta phase (it does not fix a bug, improve usability or security or performance, etc.).
Disruption Disruptive for core/contributed and custom modules/themes that use the removed methods, and for existing sites that have data in the field.
CommentFileSizeAuthor
#232 taxonomy-kill_term_desc-569434-232.txt1.73 KBplach
#219 569434-remove-term-description-219.patch39.85 KBjibran
#219 interdiff.txt565 bytesjibran
#217 Screenshot 2014-12-08 17.46.24.png72.52 KBlarowlan
#216 569434-remove-term-description-216.patch39.85 KBjibran
#216 interdiff.txt2.46 KBjibran
#213 569434-remove-term-description-213.patch39.6 KBjibran
#213 interdiff.txt525 bytesjibran
#211 569434-remove-term-description-211.patch39.6 KBjibran
#211 interdiff.txt2.89 KBjibran
#209 569434-remove-term-description-209.patch38.76 KBjibran
#204 569434-204.patch40.48 KBjibran
#202 569434-202.patch40.49 KBjibran
#202 interdiff.txt13.21 KBjibran
#200 569434-200.patch30.42 KBjibran
#200 interdiff.txt2.8 KBjibran
#197 569434-197.patch30.75 KBjibran
#197 interdiff.txt1.86 KBjibran
#196 569434-196.patch0 bytesjibran
#191 interdiff.txt2.51 KBswentel
#191 term-description-569434-191.patch31.92 KBswentel
#189 term-description-569434-189-interdiff.txt3.22 KBBerdir
#189 term-description-569434-189.patch31.64 KBBerdir
#187 term-description-569434-187.patch33.72 KBBTMash
#187 term-description-569434-interdiff.txt92 bytesBTMash
#184 term-description-569434-184-interdiff.txt2.88 KBBerdir
#184 term-description-569434-184.patch33.72 KBBerdir
#180 interdiff.txt14.84 KBpfrenssen
#180 term-description-569434-178.patch32.23 KBpfrenssen
#177 interdiff.txt10.94 KBpfrenssen
#177 term-description-569434-177.patch17.68 KBpfrenssen
#167 term-description-569434-167.patch31.22 KBjesse.d
#167 interdiff.patch714 bytesjesse.d
#162 term-description-569434-162.patch31.92 KBpcambra
#154 interdiff.txt2.55 KBjesse.d
#154 term-description-569434-152.patch35.08 KBjesse.d
#150 term-description-569434-150.patch32.73 KBswentel
#150 interdiff.txt2.66 KBswentel
#148 term-description-569434-148.patch34.84 KBswentel
#148 interdiff.txt12.84 KBswentel
#146 term-description-569434-144.patch50.54 KBjesse.d
#146 interdiff.txt542 bytesjesse.d
#146 reroll-notes.txt1.97 KBjesse.d
#131 term-description-569434-131.patch80.41 KBBerdir
#131 term-description-569434-131-interdiff.txt2.37 KBBerdir
#129 term-description-569434-129.patch80.28 KBBerdir
#129 term-description-569434-129-interdiff.txt1.28 KBBerdir
#128 Screen Shot 2013-05-24 at 8.03.00 AM.png25.58 KBlarowlan
#128 Screen Shot 2013-05-24 at 8.03.16 AM.png17.23 KBlarowlan
#122 term-description-569434-122.patch78.65 KBBerdir
#122 term-description-569434-122-interdiff.txt953 bytesBerdir
#116 term-description-569434-116.patch78.71 KBBerdir
#116 term-description-569434-116-interdiff.patch3.16 KBBerdir
#114 term-description-569434-114.patch76.45 KBBerdir
#114 term-description-569434-114-interdiff.txt6.6 KBBerdir
#112 term-description-569434-112-interdiff.txt7.14 KBBerdir
#112 term-description-569434-112.patch71.72 KBBerdir
#112 term-description-569434-112-upgrade-tests.patch69.97 KBBerdir
#111 term-description-569434-111.patch66.27 KBtim.plunkett
#111 interdiff.txt985 bytestim.plunkett
#104 taxonomy-description-569434-104.patch66.21 KBtim.plunkett
#103 changes-to-drupal-7.filled.standard_all.database.php-do-not-test.patch2.47 KBalexpott
#99 569434-99-taxonomy-description.patch66.21 KBdcam
#97 569434-97-taxonomy-description.patch64.82 KBdcam
#92 interdiff.txt2.31 KBdcam
#92 569434-92-taxonomy-description.patch64.5 KBdcam
#90 569434-90-taxonomy-description.patch64.93 KBdcam
#86 taxonomy-569434-86.patch67.73 KBtim.plunkett
#80 569434.interdiff.do-not-test.txt1.45 KBBTMash
#80 569434.patch68.51 KBBTMash
#75 569434.patch69.84 KBBTMash
#72 569434.patch26.33 KBBTMash
#72 569434.interdiff.do-not-test.patch2.62 KBBTMash
#69 taxonomy.desc-field.69.patch23.86 KBwebflo
#69 taxonomy.desc-field.69.interdiff.do-not-test.patch748 byteswebflo
#67 taxonomy.desc-field.67.patch23.13 KBsun
#60 taxonomy-description-field-569434-60.patch23.25 KBdcam
#59 taxonomy-description-field-569434-59.patch23.89 KBdcam
#55 drupal-569434-55.patch23.65 KBtim.plunkett
#54 drupal-569434-54.patch23.69 KBtim.plunkett
#52 569434-51.patch23.05 KBJody Lynn
#45 569434-44.patch23.04 KBJody Lynn
#42 569434-42.patch23.01 KBJody Lynn
#39 569434-39.patch21.47 KBJody Lynn
#37 569434-37.patch21.56 KBJody Lynn
#35 569434-35.patch21.55 KBJody Lynn
#33 569434-33.patch21.38 KBJody Lynn
#30 569434-30.patch19.38 KBJody Lynn
#28 drupal-569434-28.patch18.98 KBtim.plunkett
#26 drupal-569434-26.patch18.98 KBtim.plunkett
#24 569434-22.patch18.98 KBJody Lynn
#22 569434-22.patch18.97 KBJody Lynn
#18 569434-18.patch19 KBJody Lynn
#13 569434-13.patch19.54 KBJody Lynn
#11 569434-11.patch19.54 KBJody Lynn
#9 569434.patch14.79 KBJody Lynn

Issue fork drupal-569434

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

catch’s picture

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.

catch’s picture

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

Drupal 8.

sun’s picture

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

xjm’s picture

Title: Convert taxonomy term subject and description to fields » Convert taxonomy term description to a normal field
xjm’s picture

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

Jody Lynn’s picture

Jody Lynn’s picture

Status: Active » Needs review
FileSize
14.79 KB

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.

Jody Lynn’s picture

Status: Needs work » Needs review
FileSize
19.54 KB

Worked through some failed tests.

Jody Lynn’s picture

Status: Needs work » Needs review
FileSize
19.54 KB

Typo

Jody Lynn’s picture

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

Jody Lynn’s picture

Status: Needs work » Needs review
FileSize
19 KB

Reroll.

tim.plunkett’s picture

+++ 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?

xjm’s picture

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

Jody Lynn’s picture

Status: Needs work » Needs review
FileSize
18.97 KB

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

Jody Lynn’s picture

Status: Needs work » Needs review
FileSize
18.98 KB
tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
18.98 KB
tim.plunkett’s picture

Jody Lynn’s picture

Status: Needs work » Needs review
FileSize
19.38 KB
sun’s picture

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.

Jody Lynn’s picture

Status: Needs work » Needs review
FileSize
21.38 KB

Changes per sun's review.

Jody Lynn’s picture

Status: Needs work » Needs review
FileSize
21.55 KB
Jody Lynn’s picture

Status: Needs work » Needs review
FileSize
21.56 KB
Jody Lynn’s picture

FileSize
21.47 KB

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

DuaelFr’s picture

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.

/**
 * 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;
  }
}
plach’s picture

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

Jody Lynn’s picture

Status: Needs work » Needs review
FileSize
23.01 KB

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.

Jody Lynn’s picture

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.

Jody Lynn’s picture

FileSize
23.04 KB

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.

BTMash’s picture

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?

Jody Lynn’s picture

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.

BTMash’s picture

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.

Jody Lynn’s picture

Status: Needs work » Needs review
FileSize
23.05 KB

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.

Jody Lynn’s picture

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

tim.plunkett’s picture

Issue tags: +Needs tests
FileSize
23.69 KB

This should also have upgrade tests, I think.

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

tim.plunkett’s picture

tim.plunkett’s picture

Status: Needs review » Needs work

Okay, CNW for the upgrade tests.

BTMash’s picture

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.

BTMash’s picture

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.

dcam’s picture

Status: Needs work » Needs review
FileSize
23.89 KB

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.

dcam’s picture

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.

klonos’s picture

#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?

BTMash’s picture

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.

sun’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests, -Needs reroll +API clean-up, +Field system
FileSize
23.13 KB

Re-rolled against HEAD.

Also turned into a branch in the Platform sandbox.

webflo’s picture

Status: Needs work » Needs review
FileSize
748 bytes
23.86 KB

Removed the {taxonomy_term_data}.description definition from taxonomy_views_data.

tim.plunkett’s picture

Issue tags: +Needs tests

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

BTMash’s picture

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).

BTMash’s picture

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.

BTMash’s picture

Status: Needs work » Needs review
FileSize
69.84 KB

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).

BTMash’s picture

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.

BTMash’s picture

Status: Needs work » Needs review
FileSize
68.51 KB
1.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.

jstoller’s picture

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.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
67.73 KB

Attempted reroll.

tim.plunkett’s picture

Assigned: Unassigned » tim.plunkett

Needs a major reroll.

dcam’s picture

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.

dcam’s picture

Attaching the patch always helps.

dcam’s picture

Status: Needs work » Needs review
FileSize
64.5 KB
2.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.

dcam’s picture

Status: Needs work » Needs review
FileSize
64.82 KB

Rerolled #92.

dcam’s picture

Status: Needs work » Needs review
FileSize
66.21 KB

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.

tim.plunkett’s picture

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!

tim.plunkett’s picture

Status: Needs work » Reviewed & tested by the community

Oh hm, nvm, those are fields not tables!

alexpott’s picture

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
alexpott’s picture

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.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
66.21 KB

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

Berdir’s picture

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?

tim.plunkett’s picture

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

alexpott’s picture

+++ 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.

tim.plunkett’s picture

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?

Berdir’s picture

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.

tim.plunkett’s picture

Status: Needs review » Needs work

Oh, okay.

tim.plunkett’s picture

Assigned: tim.plunkett » Unassigned
FileSize
985 bytes
66.27 KB

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.

Berdir’s picture

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.

Berdir’s picture

Status: Needs work » Needs review
FileSize
6.6 KB
76.45 KB

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.

Berdir’s picture

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 :)

Berdir’s picture

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.

Berdir’s picture

@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.

Berdir’s picture

larowlan’s picture

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

drupal_strlen?

larowlan’s picture

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.png

Screen Shot 2013-05-24 at 8.03.16 AM.png

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

Berdir’s picture

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.

Berdir’s picture

Status: Needs work » Needs review
FileSize
2.37 KB
80.41 KB

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?

Pancho’s picture

Absolutely agree with #131.

andypost’s picture

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

effulgentsia’s picture

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.

Berdir’s picture

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.

scor’s picture

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

effulgentsia’s picture

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.

Berdir’s picture

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.

jibran’s picture

Issue tags: +Needs reroll

Patch needs reroll.

jesse.d’s picture

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.

Gaelan’s picture

Assigned: Unassigned » Gaelan

Rerollin'

jesse.d’s picture

Status: Needs work » Needs review
FileSize
1.97 KB
542 bytes
50.54 KB

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.

swentel’s picture

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.

swentel’s picture

Some redundant stuff

jesse.d’s picture

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.

Berdir’s picture

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.

Berdir’s picture

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?

jesse.d’s picture

Issue summary: View changes
FileSize
35.08 KB
2.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?

alexpott’s picture

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?

jesse.d’s picture

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.

Berdir’s picture

@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 :)

andypost’s picture

  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

andypost’s picture

+++ 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

pcambra’s picture

Status: Needs work » Needs review
FileSize
31.92 KB

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.

pcambra’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

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

jesse.d’s picture

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.

jesse.d’s picture

Related issues:
FileSize
714 bytes
31.22 KB

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.

jesse.d’s picture

Status: Needs work » Needs review
jesse.d’s picture

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.

Berdir’s picture

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.

xjm’s picture

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.

xjm’s picture

Title: Convert taxonomy term description to a configurable field » Remove 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.

xjm’s picture

Issue summary: View changes
pfrenssen’s picture

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

Going to work on this.

pfrenssen’s picture

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.

pfrenssen’s picture

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.

Berdir’s picture

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()

pfrenssen’s picture

Status: Needs work » Needs review
FileSize
32.23 KB
14.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.

sun’s picture

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.

andypost’s picture

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

Berdir’s picture

Status: Needs work » Needs review
FileSize
33.72 KB
2.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.

BTMash’s picture

Status: Needs work » Needs review
FileSize
92 bytes
33.72 KB

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.

Berdir’s picture

Assigned: pfrenssen » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
31.64 KB
3.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.

swentel’s picture

Status: Needs work » Needs review
FileSize
31.92 KB
2.51 KB

Fixing tests

andypost’s picture

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

Status: Needs review » Needs work

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

jibran’s picture

Assigned: Unassigned » jibran
Issue tags: +Needs reroll

I'll try to reroll this.

jibran’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
0 bytes

Here is reroll.

jibran’s picture

Assigned: jibran » Unassigned
FileSize
1.86 KB
30.75 KB

With real patch.

Status: Needs review » Needs work

The last submitted patch, 197: 569434-197.patch, failed testing.

andypost’s picture

The big change here that we does not ship default description field anymore.
The shipping of form_description field could help making forum content type dependency issue

  1. +++ b/core/modules/forum/config/install/field.field.taxonomy_term.forum_description.yml
    @@ -0,0 +1,12 @@
    +indexes: {  }
    \ No newline at end of file
    

    + br

  2. +++ b/core/modules/forum/config/install/field.instance.taxonomy_term.forums.forum_description.yml
    @@ -0,0 +1,15 @@
    +field_uuid: 19f95967-fe48-4f52-8e36-546303f4d589
    

    no need

jibran’s picture

Status: Needs review » Needs work

The last submitted patch, 200: 569434-200.patch, failed testing.

jibran’s picture

Status: Needs work » Needs review
FileSize
13.21 KB
40.49 KB

With some test fixes.

Status: Needs review » Needs work

The last submitted patch, 202: 569434-202.patch, failed testing.

jibran’s picture

Status: Needs work » Needs review
Issue tags: +VDC
FileSize
40.48 KB

created inverted patch. Same interdiff applies. Taging VDC for views changes.

jibran’s picture

Status: Needs review » Needs work

The last submitted patch, 204: 569434-204.patch, failed testing.

Status: Needs work » Needs review

jibran queued 204: 569434-204.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 204: 569434-204.patch, failed testing.

jibran’s picture

Status: Needs work » Needs review
FileSize
38.76 KB

Rerolled

Status: Needs review » Needs work

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

jibran’s picture

Status: Needs work » Needs review
FileSize
2.89 KB
39.6 KB

Fixed tests

Status: Needs review » Needs work

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

jibran’s picture

Status: Needs work » Needs review
FileSize
525 bytes
39.6 KB

:/

andypost’s picture

There's a removal of some tests, not sure we have the same coverage for forum module. Also some nitpics

  1. +++ b/core/modules/forum/config/install/core.entity_form_display.taxonomy_term.forums.default.yml
    @@ -17,10 +17,12 @@ content:
    -  description:
    ...
    -    weight: 0
    ...
    +  forum_description:
    ...
    +    weight: 1
    

    any reason to change weight?

  2. +++ b/core/modules/forum/config/install/field.storage.taxonomy_term.forum_description.yml
    @@ -0,0 +1,17 @@
    +persist_with_no_fields: false
    

    probably we should persist it to re-use

  3. +++ b/core/modules/field_ui/src/Tests/ManageDisplayTest.php
    @@ -365,7 +365,10 @@ function testNonInitializedFields() {
    -    $this->drupalGet('admin/structure/taxonomy/manage/' . $this->vocabulary . '/display');
    ...
    +    $this->fieldUIAddNewField('admin/structure/taxonomy/manage/' . $this->vocabulary . '/overview', 'test', 'test');
    ...
    +    $this->drupalGet('admin/structure/taxonomy/manage/' . $this->vocabulary . '/overview/display');
    

    what this changes for? no idea...

  4. +++ b/core/modules/forum/forum.module
    @@ -567,7 +567,7 @@ function template_preprocess_forum_list(&$variables) {
    -    $variables['forums'][$id]->description = Xss::filterAdmin($forum->description->value);
    

    clean up namespace Xss no more usage

  5. +++ b/core/modules/forum/src/Tests/ForumTest.php
    @@ -47,17 +48,17 @@ class ForumTest extends WebTestBase {
    -   * An array representing a forum container.
    +   * A taxonomy term representing a forum container.
    
    @@ -120,7 +121,7 @@ function testForum() {
    -    $this->drupalGet('forum/' . $this->forum['tid']);
    +    $this->drupalGet('forum/' . $this->forum->id());
    

    and a lot of others - any reason for this clean-up? seems out of scope

  6. +++ b/core/modules/forum/src/Tests/ForumTest.php
    @@ -387,11 +388,17 @@ function createForum($type, $parent = 0) {
    +    $term = entity_load('taxonomy_term', $tid);
    

    Term::load()

  7. +++ b/core/modules/taxonomy/src/Tests/TermTest.php
    @@ -396,21 +393,6 @@ function testTermInterface() {
    -    // Check that it does NOT show a description when description is blank.
    -    $term->setDescription(NULL);
    -    $term->save();
    ...
    -    $this->assertFalse($this->xpath('//div[contains(@class, "field-taxonomy-term--description")]'), 'Term page did not display the term description when description was blank.');
    

    the removed test is not added to forum which have description field

jibran’s picture

  1. I just created field, export the config and added to the default config. Will revert that.
  2. I just created field, export the config and added to the default config. Will ask @larowlan about it.
  3. Apparently they change the signature of the function in #2384357: Simplify Field UI testing
  4. Nice catch. Will remove it.
  5. $this->forum is \Drupal\taxonomy\Entity\Term object so I can't use access it using ['tid'] anymore.
  6. Will fix it.
  7. We have an assert $this->assertRaw($body, 'Body was found'); in Drupal\forum\Tests\ForumTest::createForumTopic() I think it is enough. And description is not a base field anymore so we can't move those asserts to forum.
jibran’s picture

FileSize
2.46 KB
39.85 KB

Regarding 3 in #214 yeah we can remove that field. Fixed rest of the points. Thank you for the review.

larowlan’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
72.52 KB

Manually tested, looks good

+++ b/core/modules/forum/config/install/field.storage.taxonomy_term.forum_description.yml
@@ -0,0 +1,17 @@
+locked: false

Should this be locked? Issue Summary says yes

Api changes section needs to be updated to include changes to TermInterface.
And we need a beta-period evaluation phase template.

larowlan’s picture

Issue summary: View changes

added api changes and beta template to intro

jibran’s picture

Fixed #217. Thanks @larowlan for the review and IS update.

xjm’s picture

Version: 8.0.x-dev » 9.x-dev
Status: Reviewed & tested by the community » Postponed

So the beta evaluation in the summary is missing the most important line for normal tasks, which is whether it's a prioritized change. And it's not. Following through the flowchart in https://www.drupal.org/core/beta-changes:

  1. It's not a feature.
  2. It's not unfrozen.
  3. It's not critical.
  4. It's not a prioritized change.
  5. It's not major.
  6. It doesn't really reduce fragility.

Therefore, the issue should be postponed (though it makes me sad to say so since this was a nice change), and I guess it wouldn't really be a minor version target either. I don't really think we can justify the change at this point in the release cycle, especially given the API and data model disruptions. Let me know if I've missed anything.

xjm’s picture

Issue summary: View changes
effulgentsia’s picture

Taxonomy module currently depends on text module. With this patch, could that dependency be removed? If so, would doing so change the priority to Major? (On its own, I don't think so, but not sure if there are other major issues in the queue that that would assist with)

effulgentsia’s picture

It's not a prioritized change.

Also, I think an argument could be made that it improves usability by not having a useless description field on Tags, but not sure if that's enough of a usability gain to justify the disruption.

xjm’s picture

Thanks @effulgentsia. I don't think that removing the dependency would be major either, and I think that the usability improvement is debatable or at least negligible (since most users probably never even notice that the tags field has a description anyway as the terms just get autocreated from the node form generally).

I also discussed this issue with @alexpott (should have mentioned that before, sorry). He and @berdir have both agreed with postponing the issue despite that it was sad. :)

andypost’s picture

I guess it wouldn't really be a minor version target either

is there any policy about?
The thought about dependency is interesting

catch’s picture

Hmm. All the changes are in taxonomy module and forum, so I would say it is not that disruptive.

However that's only because we're not supporting an 8-8 upgrade path yet, because the migrate tests aren't failing (although the actual term migration will need updating because there's nothing to migrate into).

So I'm neutral on 8.x vs. 9.x - if this had been committed I'd not have complained, but I can also see the reasoning for bumping the version.

@andypost I don't think we could get away with this in a minor version - the API changes slightly (and by the time we get to 8.1.x some modules may actually be using that API), and we should avoid data migrations in minor versions if at all possible.

andypost’s picture

@catch so maybe migration tests needs adjustment and change record to be filed?
PS: I think it's good to have cleaner data-model

jibran’s picture

It doesn't really reduce fragility.

I think that the usability improvement is debatable or at least negligible

I humbly disagree with these points. Please have a look at the patch again.

it is not that disruptive.

This is not at all disruptive imo cuz there is no critical in taxonomy issue queue right now.

That being said I am fine with the decision.

xjm’s picture

Thanks @jibran!

This is not at all disruptive imo cuz there is no critical in taxonomy issue queue right now.

That's not what makes something disruptive. See the definition of disruptive changes. Edit: I edited this to be more clear about the fact that something that affects stored data or requires an upgrade path is inherently at least somewhat disruptive.

Removing an entire field definitely affects existing stored data on existing sites and would need an upgrade path, so this definitely has disruption for that reason alone. The Taxonomy ER conversion had an even greater disruption, but also had so many benefits that we discussed it and decided it was worthwhile. However, that should probably be the end of changes in taxonomy that affect existing sites like that.

So let's keep this as 9.x.

plach’s picture

I didn't try but I think this could be implemented in a BC way: we could leave the field definition in place only for installations already having it, and remove it for all new ones. The Entity Manager has a list in state of the last installed field definitions.

If I'm correct this could be implemented in any minor release.

jibran’s picture

@plach if you can come up with a concert plan then we can make a proper case of moving it to 8.1.x.

plach’s picture

Something like this. Obviously we should keep a BC layer and just deprecate methods on TermInterface instead of removing them.

catch’s picture

Version: 9.x-dev » 8.3.x-dev
Status: Postponed » Needs work

Yes let's see if this is possible in 8.x

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now 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.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now 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.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

kclarkson’s picture

Is there a reason this has not been suggested as a change to core?

I am using Taxonomy for Institution, but I want to make the description field required.

Yes, I can remove the core field from the form display and add a custom field but that seems like major overkill.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Anybody’s picture

Version: 8.9.x-dev » 9.0.x-dev

I guess if this important change and standardization happens, then with the switch to Drupal 9! Changing the version accordingly due to 3 years inactivity. Meanwhile it still makes a lot of sense to solve this identically to other entities.

TL;DR: see #170 by @Berdir!

Berdir’s picture

Well, #170 was in 2014, 5 years later the (Drupal) world is very, very different.

For starters, no API changes that aren't removing already deprecated code are allowed in D9 compared to 8.8/8.9.

So we can do it only with 9.1 or later, and I'm not quite sure even then what the best approach is. Most likely what we need to do is convert it to a configurable field with the same name and add it to all bundles on existing installations, but we have \Drupal\taxonomy\TermInterface::getDescription(), so terms having a description is part of the API. so likely we need to install this configurable field even for new D9 installations, deprecate it for D10 and then remove the default config. But to not break the API, we'd literally need to *disallow* deleting and always force-enable that field on all vocabularies.

Yes, BC is hard. That's the price for "Upgrading to Dcurrent+1 is easy".

Anybody’s picture

Version: 9.0.x-dev » 9.1.x-dev

Hi @Berdir,
sorry for my delayed reply. From my perspective the BC conditions are absolutely okay in this case. This is an important change and further standardization in the entity sphere. I'll change the version accordingly and link your comment in the IS. Could you or someone else with the required core experience perhaps have a look at the IS and update it accordingly to your comment? I guess I'm not experienced enough to do that for such a central topic in core code. Sorry and thank you in advance.
PS: Who can decide, if this is still "wanted" before we invest more time?

Anybody’s picture

Issue summary: View changes
Anybody’s picture

Issue summary: View changes

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

gabesullice’s picture

So we can do it only with 9.1 or later, and I'm not quite sure even then what the best approach is. Most likely what we need to do is convert it to a configurable field with the same name and add it to all bundles on existing installations, but we have \Drupal\taxonomy\TermInterface::getDescription(), so terms having a description is part of the API. so likely we need to install this configurable field even for new D9 installations, deprecate it for D10 and then remove the default config. But to not break the API, we'd literally need to *disallow* deleting and always force-enable that field on all vocabularies.

I like this plan, with one question: why not make getDescription fall back to $this->label() if the description field has been deleted?

Anybody’s picture

I like this plan, with one question: why not make getDescription fall back to $this->label() if the description field has been deleted?

Well I think this isn't what you want if you removed the description field. If you want to get the label, get the label... if there is no description, return none...

But I agree it would be cool to have this solved. Really looking forward to that! :)

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

andypost’s picture

Issue tags: +Needs reroll

Bhanu951 made their first commit to this issue’s fork.

Bhanu951’s picture

Assigned: Unassigned » Bhanu951

Bhanu951’s picture

Assigned: Bhanu951 » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs reroll

Anchal_gupta made their first commit to this issue’s fork.

andypost’s picture

Status: Needs review » Needs work
Issue tags: +Needs upgrade path, +Needs upgrade path tests

Left a set of reviews on MR

Patch in #232 shows what should be done for proper deprecation of interface, migrations and probably some code should be moved to forum module from taxonomy.

Forum's field should be named description as having "field_" prefix will lead inability to create "description" field via field_ui module. It will prevent fixing lots of tests (also there's unrelated changes tid/vid)

Taxonomy's module tests could still provide a trait or helper method to create description field so it will prevent changes in many contrib/custom tests but will provide sane deprecation message if field used in tests

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.