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.
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
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. |
Comment | File | Size | Author |
---|---|---|---|
#232 | taxonomy-kill_term_desc-569434-232.txt | 1.73 KB | plach |
#219 | 569434-remove-term-description-219.patch | 39.85 KB | jibran |
#219 | interdiff.txt | 565 bytes | jibran |
Issue fork drupal-569434
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:
Comments
Comment #1
catchThis 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.
Comment #3
catchDrupal 8.
Comment #4
sun#553326: Provide consistency to the way field UI is presented adds a 'format' column to taxonomy_term_data...
Comment #6
xjmMarked #1152624: Add label for 'Description' field in view mode as duplicate of this issue.
See also: #1192070: Remove / Don't add a default "Description" field to Terms.
Comment #7
xjmMarked #1233032: Taxonomy Text Processing as duplicate of this issue.
Comment #8
Jody Lynn#1192070: Remove / Don't add a default "Description" field to Terms closed as a duplicate.
Comment #9
Jody LynnThis 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.
Comment #11
Jody LynnWorked through some failed tests.
Comment #13
Jody LynnTypo
Comment #15
Jody LynnI don't know what the deal is with the upgrade tests failing and could use some help.
Comment #18
Jody LynnReroll.
Comment #20
tim.plunkettTrailing whitespace makes the bot fail? Has it always done it?
Comment #21
xjmIt's not the trailing whitespace; it's that the patch does not apply, as far as I can see?
Comment #22
Jody LynnI think it was just the whitespace that made it not apply. Geez..
Comment #24
Jody LynnComment #26
tim.plunkettBroken by #1496462: Convert RSS publishing settings to configuration system.
Comment #28
tim.plunketttaxonomy_update_8001 was added by #1454538: Add langcode property to all entity types; for the user entity, distinguish entity language from user's language preference
Comment #30
Jody LynnComment #32
sunLovely 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.
You must not use API functions (which invoke module hooks) in module updates. Use a direct db query instead.
Ditto for these - however, field.install provides stub replacement functions for module updates which you can use instead.
Comment #33
Jody LynnChanges per sun's review.
Comment #35
Jody LynnComment #37
Jody LynnComment #39
Jody LynnHere's some progress to passing the upgrade tests. There's still a language/langcode problem that may need some hook_update_dependencies work.
Comment #40
DuaelFrThis 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.
Comment #41
plachOr use the Title module. Ammitedly the project name is a bit misleading...
Comment #42
Jody LynnHopefully 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.
Comment #44
Jody LynnI 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.
Comment #45
Jody LynnI 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.
Comment #49
BTMash CreditAttribution: BTMash commentedI'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?
Comment #50
Jody LynnOK, 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.
Comment #51
BTMash CreditAttribution: BTMash commentedWe should be looking at the generate-d7-script in that case:
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.
Comment #52
Jody LynnI 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.
Comment #53
Jody LynnThanks BTMAsh, yeah looks like a mistake in creating those terms.
Comment #54
tim.plunkettThis should also have upgrade tests, I think.
Here's a reroll for the PSR-0 taxonomy tests.
Comment #55
tim.plunkettEasy reroll after #1496542: Convert site information to config system.
Comment #56
tim.plunkettOkay, CNW for the upgrade tests.
Comment #57
BTMash CreditAttribution: BTMash commentedI'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.
Comment #58
BTMash CreditAttribution: BTMash commentedTalking 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.
Comment #59
dcam CreditAttribution: dcam commentedRerolled 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.
Comment #60
dcam CreditAttribution: dcam commentedRerolled #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.
Comment #62
klonos#52:
Do we have a todo/follow-up for this?
Comment #63
BTMash CreditAttribution: BTMash commentedBased 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.
Comment #67
sunRe-rolled against HEAD.
Also turned into a branch in the Platform sandbox.
Comment #69
webflo CreditAttribution: webflo commentedRemoved the {taxonomy_term_data}.description definition from
taxonomy_views_data
.Comment #71
tim.plunkettThere is an upgrade path, but no explicit tests for it.
Comment #72
BTMash CreditAttribution: BTMash commentedI'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).
Comment #73
BTMash CreditAttribution: BTMash commentedGoing 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.
Comment #75
BTMash CreditAttribution: BTMash commentedFor 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).
Comment #79
BTMash CreditAttribution: BTMash commentedUpgrade path tests have already been added to the patch - however, it does require a (not-so-easy) reroll.
Assigning to self.
Comment #80
BTMash CreditAttribution: BTMash commentedThe
field_sql_storage_update_8000()
changes seem to have made it in via another patch. So I removed that portion from this patch.Comment #83
jstollerI 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.
Sorry I can't help with the testbot errors.
Comment #86
tim.plunkettAttempted reroll.
Comment #88
tim.plunkettNeeds a major reroll.
Comment #89
dcam CreditAttribution: dcam commentedRerolled #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.
Comment #90
dcam CreditAttribution: dcam commentedAttaching the patch always helps.
Comment #92
dcam CreditAttribution: dcam commentedThe 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.
Comment #97
dcam CreditAttribution: dcam commentedRerolled #92.
Comment #99
dcam CreditAttribution: dcam commentedWell... 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.
Comment #100
tim.plunkettThis 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!
Comment #101
tim.plunkettOh hm, nvm, those are fields not tables!
Comment #102
alexpottNeeds a reroll...
Comment #103
alexpottBecause 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.
Comment #104
tim.plunkettConflicted 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
Comment #105
BerdirThis 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?
Comment #106
tim.plunkettBecause this was written before that. Didn't even notice. Not sure if it matters...
Comment #107
alexpottI 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.
Comment #108
tim.plunkettberdir pointed this out in IRC:
Who knows what else might become terms, why not convert the data we know is the correct data?
Comment #109
BerdirI 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.
Comment #110
tim.plunkettOh, okay.
Comment #111
tim.plunkettI'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.
Comment #112
BerdirThe 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.
Comment #114
BerdirOh, 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.
Comment #116
BerdirUh, 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 :)
Comment #117
BerdirWe (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.
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
Speaks for itself. Another thing that implicitly worked but had no explicit dependency but now needs it.
Comment #120
Berdir@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.
Comment #122
BerdirRe-roll.
Comment #127
larowlandrupal_strlen?
Comment #128
larowlanShould 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.
http://sad1969796e10317.s3.simplytest.me/ admin/power
Comment #129
BerdirRe-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.
Comment #131
BerdirFixing 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?
Comment #133
PanchoAbsolutely agree with #131.
Comment #134
andypostSuppose tags does not need description at all, but others could need. But once this is a conversion we need helper
Comment #137
effulgentsia CreditAttribution: effulgentsia commentedWith 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.
Comment #139
BerdirWe'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.
Comment #140
scor CreditAttribution: scor commentedThe RDF module also cannot annotate the term description in a consistent manner, that's why streamlining this would help avoid custom ugly workarounds.
Comment #141
effulgentsia CreditAttribution: effulgentsia commentedBoth 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.
Comment #142
BerdirNot 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.
Comment #143
jibranPatch needs reroll.
Comment #144
jesse.d CreditAttribution: jesse.d commentedDo 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.
Comment #145
Gaelan CreditAttribution: Gaelan commentedRerollin'
Comment #146
jesse.d CreditAttribution: jesse.d commentedHopefully 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.
Comment #148
swentel CreditAttribution: swentel commentedThis was a hard one to get right. I've had to change a lot of things to make it work nicely with current HEAD.
Comment #150
swentel CreditAttribution: swentel commentedSome redundant stuff
Comment #151
jesse.d CreditAttribution: jesse.d commentedThe 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.
Comment #152
BerdirWe 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.
Comment #153
BerdirAlso 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?
Comment #154
jesse.d CreditAttribution: jesse.d commentedOkay, 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?
Comment #155
alexpott@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.
Why is this not just default config in the standard profile?
Comment #156
jesse.d CreditAttribution: jesse.d commentedWhoops, looks like I might have moved too early on working on it. see #'s 152 and 153 before reviewing 154.
Comment #158
Berdir@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 :)
Comment #159
andypostI think better to have a public method on Vocabulary entity for that
This broken when no terms exists
this could be a config file as well
Comment #160
andypostForum vocabulary have description and uses the one
Comment #161
jibranComment #162
pcambraHere'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
Comment #164
pcambra162: term-description-569434-162.patch queued for re-testing.
Comment #166
jesse.d CreditAttribution: jesse.d commentedRemoving 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.
Comment #167
jesse.d CreditAttribution: jesse.d commentedI 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.
Comment #168
jesse.d CreditAttribution: jesse.d commentedComment #169
jesse.d CreditAttribution: jesse.d commentedSo 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.
Comment #170
BerdirOk, 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.
Comment #171
xjmAlrighty, I've been convinced. Let's do everything in #170.
P.S. -- Sorry for the prolonged wintry silence. :)
Comment #173
xjmRe-scoping. There's now some duplicates of this somewhere about that we should close as such.
Comment #174
xjmComment #175
pfrenssenGoing to work on this.
Comment #176
pfrenssenReviewing the patch while getting familiar with the issue:
What is this doing here? Is this needed in scope of this issue?
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.
Comment #177
pfrenssenUpdated 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.
Comment #179
BerdirI know you just reverted the changes, but here are some hints that you will need to consider when changing it to a field.
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.
This will need to be updated, as the field will be description[0][value] in the form.
This revert can be reverted, as the description will then be in a field table. Or load it and then verify.
See above, we should document/use processed here, not value.
If we remove it then we should open a separate issue for this.
More important than this is removing the definitions in baseFieldDefinitions()
Comment #180
pfrenssenAdded 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.
Comment #181
sunHm. 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:
node_add_body_field($bundle, $label)
, which allows modules to create a (single) term body field on selected bundles. + Make Forum use that function.[term_]body
internally. The label may or may not stay "Description".Thoughts?
Comment #183
andypost#179 is not addressed
term_body! could be shipped with taxonomy module
and forum will just add its instance
is not fixed, and template variable too
Comment #184
BerdirRe-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.
Comment #187
BTMash CreditAttribution: BTMash commentedTested 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.
Comment #189
BerdirRe-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().
Comment #191
swentel CreditAttribution: swentel commentedFixing tests
Comment #192
andypostRTBC except if the nitpick
suppose this should not be removed
Comment #195
jibranI'll try to reroll this.
Comment #196
jibranHere is reroll.
Comment #197
jibranWith real patch.
Comment #199
andypostThe 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
+ br
no need
Comment #200
jibranHere is the reroll. Fixed #199
Comment #202
jibranWith some test fixes.
Comment #204
jibrancreated inverted patch. Same interdiff applies. Taging VDC for views changes.
Comment #205
jibranComment #209
jibranRerolled
Comment #211
jibranFixed tests
Comment #213
jibran:/
Comment #214
andypostThere's a removal of some tests, not sure we have the same coverage for forum module. Also some nitpics
any reason to change weight?
probably we should persist it to re-use
what this changes for? no idea...
clean up namespace Xss no more usage
and a lot of others - any reason for this clean-up? seems out of scope
Term::load()
the removed test is not added to forum which have description field
Comment #215
jibran\Drupal\taxonomy\Entity\Term
object so I can't use access it using ['tid'] anymore.$this->assertRaw($body, 'Body was found');
inDrupal\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.Comment #216
jibranRegarding 3 in #214 yeah we can remove that field. Fixed rest of the points. Thank you for the review.
Comment #217
larowlanManually tested, looks good
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.
Comment #218
larowlanadded api changes and beta template to intro
Comment #219
jibranFixed #217. Thanks @larowlan for the review and IS update.
Comment #220
xjmSo 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:
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.
Comment #221
xjmComment #222
effulgentsia CreditAttribution: effulgentsia commentedTaxonomy 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)
Comment #223
effulgentsia CreditAttribution: effulgentsia commentedAlso, 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.
Comment #224
xjmThanks @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. :)
Comment #225
andypostis there any policy about?
The thought about dependency is interesting
Comment #226
catchHmm. 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.
Comment #227
andypost@catch so maybe migration tests needs adjustment and change record to be filed?
PS: I think it's good to have cleaner data-model
Comment #228
jibranI humbly disagree with these points. Please have a look at the patch again.
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.
Comment #229
xjmThanks @jibran!
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.
Comment #230
plachI 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.
Comment #231
jibran@plach if you can come up with a concert plan then we can make a proper case of moving it to 8.1.x.
Comment #232
plachSomething like this. Obviously we should keep a BC layer and just deprecate methods on
TermInterface
instead of removing them.Comment #233
catchYes let's see if this is possible in 8.x
Comment #238
kclarkson CreditAttribution: kclarkson commentedIs 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.
Comment #241
AnybodyI 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!
Comment #242
BerdirWell, #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".
Comment #243
AnybodyHi @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?
Comment #244
AnybodyComment #245
AnybodyComment #248
gabesulliceI like this plan, with one question: why not make
getDescription
fall back to$this->label()
if the description field has been deleted?Comment #249
AnybodyWell 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! :)
Comment #253
andypostComment #255
Bhanu951 CreditAttribution: Bhanu951 as a volunteer commentedComment #257
Bhanu951 CreditAttribution: Bhanu951 as a volunteer commentedComment #259
andypostLeft 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 changestid
/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