Based on #1498634: [meta] Define revision/translation SQL schema for core entities (no patch) there should be a new schema for both taxonomy. See #1498674: Refactor node properties to multilingual for an ongoing conversion.

Files: 
CommentFileSizeAuthor
#92 1498660-term-ml-91.patch34.63 KBplach
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 74,990 pass(es).
[ View ]

Comments

dawehner’s picture

Issue tags:+D8MI, +language-content

Add tags

Gábor Hojtsy’s picture

Title:Define taxonomy translation schema» Refactor taxonomy entity properties to multilingual
Status:Active» Postponed
klonos’s picture

plach’s picture

Status:Active» Postponed
Gábor Hojtsy’s picture

One more D8Mi tag.

effulgentsia’s picture

Status:Postponed» Active

Issue from #4 is in, so unpostponing.

andypost’s picture

Anything left here?

Gábor Hojtsy’s picture

Yeah, everything. You cannot translate a taxonomy label ATM in Drupal 8. At least these are NG entities. Same are menu items. They are not even NG.

andypost’s picture

@Gabor, please update issue summary with proposed solution. I'll attack this on devDays

Gábor Hojtsy’s picture

This would apply the same property changes like #1498674: Refactor node properties to multilingual.

Gábor Hojtsy’s picture

Issue summary:View changes

Update. Vocabularies are not content entities anymore.

marco’s picture

Assigned:Unassigned» marco
Issue summary:View changes
plach’s picture

Issue tags:+beta target

We definitely need this to happen, and we definitely want it to happen before beta.

Berdir’s picture

Status:Active» Needs review
StatusFileSize
new1.23 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,847 pass(es), 0 fail(s), and 1 exception(s).
[ View ]

With the generated schema, this is now supposed to work...

.. and I can translate the name and description, but..

- While the description is displayed correctly, the title is not (this is why we need #2160965: Content entities are not upcast in the page language, inconsistent with config entities)
- Views integration is now incorrect (this is why we need #1740492: Implement a default entity views data handler)
- You need to re-install (This is why we need #1498720: [meta] Make the entity storage system handle changes in the entity and field schema definitions)

marco’s picture

StatusFileSize
new12.41 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,090 pass(es), 9 fail(s), and 9 exception(s).
[ View ]

I was working on this one too, but after seeing #13 I'm not so sure the approach I was following for views is correct, see the attached patch.

The last submitted patch, 13: term-translatable-1498660-1.patch, failed testing.

Status:Needs review» Needs work

The last submitted patch, 14: 1498660-14-term-translatable.patch, failed testing.

plach’s picture

@berdir:

I know, but none of them is a beta-blocker so we need the default table layout to change to allow for translatable base fields. Also there are some queries in the storage that need to be adjusted (and that in the long run will need to support all 4 table layouts).

Berdir’s picture

Priority:Normal» Major

I don't see how we can release with a static and hardcoded views integration when we claim to support different table layouts, for example for node. So IMHO, this just became a lot more important, setting to major for now.

Berdir’s picture

Priority:Major» Normal

Sorry, wrong issue :)

Berdir’s picture

@plach: Yeah I know, I was mostly trying to point out why we do need those issues :)

Gábor Hojtsy’s picture

Well, the question is how big of a schema change will this be and whether that is beta blocking in some way.

plach’s picture

This is not beta blocking AFAIK, but it definitely needs to be done before beta if we want taxonomy translatable by default: we are switching from base table only to base + data tables.

Gábor Hojtsy’s picture

Issue tags:+sprint

Put on D8MI sprint. This is an important missing piece for us.

marco’s picture

I'm going to work on this during this weekend, or early next week worst case.

marco’s picture

Status:Needs work» Needs review
StatusFileSize
new29.55 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,777 pass(es), 0 fail(s), and 1 exception(s).
[ View ]

Status:Needs review» Needs work

The last submitted patch, 25: 1498660-25-term-translatable.patch, failed testing.

plach’s picture

  1. +++ b/core/modules/taxonomy/src/Plugin/views/argument/IndexTid.php
    @@ -21,9 +21,10 @@ class IndexTid extends ManyToOne {
    +    $result = db_select('taxonomy_term_field_data', 'td')

    This should be loading the term entity instead of querying the db directly and use \Drupal::entityManager()->getTranslationFromContext($entity)->label() to render the term name in the proper language.

  2. +++ b/core/modules/taxonomy/src/Plugin/views/filter/TaxonomyIndexTid.php
    @@ -107,9 +107,10 @@ protected function valueForm(&$form, &$form_state) {
    +        $result = db_select('taxonomy_term_field_data', 'td')

    Same as above: here and below we should be using entity query + entity load to retrieve term entities and properly render them or at least output translated data.

  3. +++ b/core/modules/taxonomy/taxonomy.views.inc
    @@ -14,21 +14,21 @@
    + // taxonomy_term_field_data table

    Views integration does not look exactly proper to me: for nodes we are exposing both the base table and the data table. You may want to have a look to node.views.inc. That might fix also the last Views-related test failure.

plach’s picture

marco’s picture

StatusFileSize
new21.99 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,636 pass(es), 1 fail(s), and 2 exception(s).
[ View ]

Thanks for the patience, here's a new patch.

marco’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 29: 1498660-29-term-translatable.patch, failed testing.

marco’s picture

Status:Needs work» Needs review
StatusFileSize
new23.88 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 1498660-32-term-translatable.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Status:Needs review» Needs work

The last submitted patch, 32: 1498660-32-term-translatable.patch, failed testing.

marco’s picture

Status:Needs work» Needs review
StatusFileSize
new22.46 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,064 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

Status:Needs review» Needs work

The last submitted patch, 34: 1498660-34-term-translatable.patch, failed testing.

marco’s picture

Status:Needs work» Needs review
StatusFileSize
new24.94 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,890 pass(es).
[ View ]
plach’s picture

Status:Needs review» Needs work
Issue tags:+Needs manual testing

Thanks, I found a few more issues but we are getting close! Tagging as needs manual testing as I suspect some code we are touching is not test covered. I will test this myself before RTBCing it.

  1. +++ b/core/modules/taxonomy/src/Plugin/views/filter/TaxonomyIndexTid.php
    @@ -132,31 +131,30 @@ protected function valueForm(&$form, &$form_state) {
    +            $choice->option = array($term_record->id() => str_repeat('-', $term_record->getDepth()) . String::checkPlain(\Drupal::entityManager()->getTranslationFromContext($term_record)->label()));

    Minor, but can we rename the $term_record variable to $term? It's really confusing now.

  2. +++ b/core/modules/taxonomy/src/TermStorage.php
    @@ -251,4 +254,23 @@ public function getSchema() {
    +  public function nodesGetTerms($nids, $vocabs = array()) {

    This is not returning a value. I guess this code is not test-covered atm.

  3. +++ b/core/modules/taxonomy/src/TermStorage.php
    @@ -251,4 +254,23 @@ public function getSchema() {
    +      $result[$term_record->node_nid][$term_record->tid] = $this->load($term_record->tid);

    A load multiple here would be nice :)

  4. +++ b/core/modules/taxonomy/src/TermStorageInterface.php
    @@ -85,4 +85,16 @@ public function nodeCount($vid);
    +   *   An optional vocabularies array to restrict the term search.

    Should be:
    "(optional) A vocabularies array to restrict the terms search. Defaults to ..."

  5. +++ b/core/modules/taxonomy/src/TermStorageInterface.php
    @@ -85,4 +85,16 @@ public function nodeCount($vid);
    +  public function nodesGetTerms($nids, $vocabs = array());

    I'd call this ::getNodeTerms(). Also, we could add an optional langcode parameter and filter to the list with its value if specified.

  6. +++ b/core/modules/taxonomy/taxonomy.views.inc
    @@ -36,6 +34,15 @@ function taxonomy_views_data() {
    +  // taxonomy_term_field_data table

    Missing trailing dot.

andypost’s picture

+ $choice->option = array($term_record->id() => str_repeat('-', $term_record->getDepth()) . String::checkPlain(\Drupal::entityManager()->getTranslationFromContext($term_record)->label()));
Minor, but can we rename the $term_record variable to $term? It's really confusing now.

loading a whole tree of entities... incredible for filter
+1 it's no more a record

plach’s picture

+++ b/core/modules/taxonomy/src/Entity/Term.php
@@ -248,4 +251,11 @@ public function getVocabularyId() {
+    return $this->depth;

Not sure how this is working: it is not a field so how is it getting loaded?

@andypost:

loading a whole tree of entities... incredible for filter

It's just the filter configuration form, I think.

marco’s picture

Status:Needs work» Needs review
StatusFileSize
new28.82 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,069 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
new12.14 KB

Status:Needs review» Needs work

The last submitted patch, 40: 1498660-40-term-translatable.patch, failed testing.

marco’s picture

StatusFileSize
new28.83 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,072 pass(es).
[ View ]
new12.54 KB

@andypost:

loading a whole tree of entities... incredible for filter

I'm not a big fan as well: taxonomy_get_tree() accepts a bool $load_entities and I think it's either we don't support translations or we load the entities.

@plach

It's just the filter configuration form, I think.

Yes.

+ return $this->depth;
Not sure how this is working: it is not a field so how is it getting loaded?

Not sure how to do this. $depth is populated on the fly in taxonomy_get_tree() here:
https://github.com/drupal/drupal/blob/7.x/modules/taxonomy/taxonomy.modu...
but due to the __set() magic method, it's stored in $term->values, which is protected.
I've renamed the method getTreeDepth(), but it's confusing because someone might think he can get the depth of the term always, while this happens only when the term was returned by taxonomy_get_tree().

marco’s picture

Status:Needs work» Needs review
YesCT’s picture

StatusFileSize
new2.08 KB

for those following along. :)
tree depth changes that were in 42
[edit: and the fix for the fail was change getTranslatedTerm() to getTranslatedName()]

mon_franco’s picture

@YesCT I have reviewed the patch using codersniffer, all inline comments are ok.

andypost’s picture

Status:Needs review» Needs work

Is there any reason to change TermInterface in that conversion?

About loading all entities - suppose there should be other solution

YesCT’s picture

Status:Needs work» Needs review
StatusFileSize
new28.83 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,923 pass(es).
[ View ]
new1.59 KB

just cleaning up some nits.

"summary lines must start with a third person singular present tense verb, and they must explain what the function does. Example: "Calculates the maximum weight for a list.""
https://www.drupal.org/node/1354#functions

and some methods added, ended up removing the blank line at the end of the classes, which should be there.
"Leave an empty line between end of method and end of class definition:"
https://www.drupal.org/node/608152#indenting

YesCT’s picture

StatusFileSize
new27.03 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,062 pass(es).
[ View ]
new3.19 KB

took out some out of scope changes to comments. if those should be made, let's do it in #2298309: clean up comments in taxonomy.views.inc

plach’s picture

  1. +++ b/core/modules/taxonomy/src/Entity/Term.php
    @@ -224,6 +224,13 @@ public function getName() {
    +  public function getTranslatedName() {

    I am not entirely sure about the DX implications of this change: it might make sense but it might also open the way for the idea that's the entity responsibility to provide the proper translation data for the current context, which is not. Entities are dumb data containers. Anyway, if we do this we should do it for any entity type, that is ::getTranslatedLabel(). I'd open a separate issue to discuss this.

  2. +++ b/core/modules/taxonomy/src/Plugin/views/filter/TaxonomyIndexTid.php
    @@ -154,7 +154,7 @@ protected function valueForm(&$form, &$form_state) {
    +          $options[$term->id()] = String::checkPlain($term->getTranslatedTerm());

    @@ -314,7 +314,7 @@ function validate_term_strings(&$form, $values) {
    +      unset($missing[strtolower($term->getTranslatedTerm())]);

    @@ -352,7 +352,7 @@ public function adminSummary() {
    +        $this->value_options[$term->id()] = String::checkPlain($term->getTranslatedTerm());

    These are definitely not test covered, I think :)

  3. +++ b/core/modules/taxonomy/src/TermStorage.php
    @@ -269,8 +270,23 @@ public function nodesGetTerms($nids, $vocabs = array()) {
    +    $all_terms = Term::loadMultiple($all_tids);

    We can just do $this->loadMultiple() here.

  4. +++ b/core/modules/taxonomy/src/TermStorageInterface.php
    @@ -91,10 +91,12 @@ public function resetWeights($vid);
    +   *   (optional) A vocabularies array to restrict the term search.
    ...
    +   *   (optional) A language code to restrict the term search.

    We are missing the default description here: "Defaults to ...". In general, it can just repeat the default value of the function signature or, if needed, provide a more extensive description of the logic behind the default.

@marco:

Not sure how to do this. $depth is populated on the fly in taxonomy_get_tree() [...]
I've renamed the method getTreeDepth(), but it's confusing because someone might think he can get the depth of the term always, while this happens only when the term was returned by taxonomy_get_tree().

Mmh, this is a leftover of an approach that in D8 should no longer exist, that is storing non-field data on entity objects just to pass it around more easily. Probably the right way to do this in D8 is by using a computed field, but I think this deserves its own issue, where we can discuss the proper implementation. Would you mind reverting the related changes and just leave $term->depth for now?

@andypost:

About loading all entities - suppose there should be other solution

There is no other solution: the only way to get entity data is by loading entities. The entity API does not support partial entity data loading/displaying, it's an approach that's completely deprecated in D8. We can try to optimize data loading internally, put the public-facing API does not need to know or care about that.

plach’s picture

Status:Needs review» Needs work
marco’s picture

Mmh, this is a leftover of an approach that in D8 should no longer exist, that is storing non-field data on entity objects just to pass it around more easily. Probably the right way to do this in D8 is by using a computed field, but I think this deserves its own issue, where we can discuss the proper implementation. Would you mind reverting the related changes and just leave $term->depth for now?

If I revert to $term->depth, then this test will fail:
https://qa.drupal.org/pifr/test/818708
because $term->depth is not accessible, and it's used for the dashes in the tree hierarchy.

plach’s picture

Sorry, but I don't get how wrapping $term->depth with a ::getTreeDepth() method fixes that...

I misread your reply, sorry: do you mean we can write $term->depth but not read it?

Edit: If I am not missing something ContentEntityBase::__get() should return its value...

marco’s picture

I misread your reply, sorry: do you mean we can write $term->depth but not read it?
If I am not missing something ContentEntityBase::__get() should return its value...

Never mind, it works. I'll post a new patch today.

marco’s picture

Status:Needs work» Needs review
StatusFileSize
new6.97 KB
new25.99 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,368 pass(es).
[ View ]

* restored $term->depth
* removed getTranslatedName()
* other review fixes

YesCT’s picture

StatusFileSize
new25.94 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,388 pass(es).
[ View ]

rerolled for #2298309: clean up comments in taxonomy.views.inc.
conflict lines were around deleted comments, mostly just context lines in the patch.
nothing tricky.

plach’s picture

Changes look good to me, I didn't have the time to manually test the Views handlers we are touching in the patch. Anyone else feel free to :)

If manual testing is ok, this is RTBC for what I'm concerned.

Gábor Hojtsy’s picture

Issue summary:View changes

Woot, comments landed, so we are now down to this one and menus :) Menu is a beta blocker.

andypost’s picture

Issue tags:-Needs manual testing

Tested manually but vocabulary is not so big (500 terms) and found no visual regressions.
Glad to fire rtbc, but needs change record #1498662-55: Refactor comment entity properties to multilingual

PS: seems we need follow-up issue to optimize queries and fix views settings form for big vocabularies

plach’s picture

Thanks! Agreed on the follow-up, I will write the change record later if no one beats me to it.

plach’s picture

StatusFileSize
new1.25 KB
new27.19 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,171 pass(es).
[ View ]

I realized we did not have explicit test coverage for this change, someone please confirm the interdiff is good.

Working on the CR now.

jibran’s picture

plach’s picture

Draft change record at https://www.drupal.org/node/2301901

@jibran: not sure how that's related?

Anyone wishing to RTBC this? I'm ok with rest, we just need someone confirming my interdiff is good.

plach’s picture

Created follow-ups

Gábor Hojtsy’s picture

Status:Needs review» Reviewed & tested by the community

The interdiff looks good and the change record looks good. Per plach the prior stuff was already RTBC quality in #56 and andypost did manual tests also.

alexpott’s picture

Status:Reviewed & tested by the community» Needs work
    // We construct this filter using taxonomy_index.tid (which limits the
    // filtering to a specific vocabulary) rather than taxonomy_term_data.name

in Drupal\node\Plugin\views\wizard::buildFilters is now wrong.

* @param $load_entities
*   If TRUE, a full entity load will occur on the term objects. Otherwise they
*   are partial objects queried directly from the {taxonomy_term_data} table to

in taxonomy_get_tree() docs is now wrong.

      $query = db_select('taxonomy_term_data', 'td');
      $query->addJoin($def['type'], 'taxonomy_index', 'tn', 'tn.tid = td.tid');
      $query->condition('td.vid', array_filter($this->options['vids']));
      $query->condition('td.default_langcode', 1);

In Drupal\taxonomy\Plugin\views\relationship\NodeTermData::query()... How does this work? taxonomy_term_data does not have a default_langcode field? This must be untested.

plach’s picture

Status:Needs work» Needs review
StatusFileSize
new3.79 KB
new30.36 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,122 pass(es), 0 fail(s), and 2 exception(s).
[ View ]

This should address #65. I manually tested it with a node view and a term relationship to display term names and everything worked fine.

Status:Needs review» Needs work

The last submitted patch, 66: 1498660.term_translatable.66.patch, failed testing.

andypost’s picture

Last failures caused by taxonomy_index does not have language

andypost’s picture

Status:Needs work» Needs review
StatusFileSize
new2.94 KB
new29.52 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,233 pass(es).
[ View ]

Fix broken tests

PS: while debuggin filed patch to #2239227: Views GroupwiseMax class calls protected properties

plach’s picture

Status:Needs review» Reviewed & tested by the community

Looks good, thanks! I tested the the node relationship and it's working as expected.

alexpott’s picture

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs tests

Given that the patch in #60 did not fail we must be missing test coverage - let's add it.

plach’s picture

I think that would be pretty out of scope here: can't we do that in a separate, non beta-deadline-bound issue?

andypost’s picture

@alexpott Please elaborate what needs tests?
@plach if you know that, please file follow-ups

plach’s picture

Status:Needs work» Reviewed & tested by the community

@alexpott:

Perhaps there's a misunderstanding: in #60 I actually added explicit test coverage for the changes we are making here. If you were referring to providing additional test coverage for taxonomy views integration, I still think that's out of scope, especially for a beta-deadline issue, but please let us know :)

plach’s picture

Status:Reviewed & tested by the community» Needs review

After reading #71 again, it's clear Alex refers to views integration.

plach’s picture

Status:Needs review» Needs work

@andypost:

Spoke with @alexpott: we need to make sure we have a passing test for the node_term relationship you fixed in #69. Just replicating the view you used to perform manual testing should be fine.

andypost’s picture

Status:Needs work» Needs review
StatusFileSize
new2.89 KB
new32.4 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,241 pass(es).
[ View ]

Extended test case to make sure that relation, limited by vocabularies works.
Also fixed plugin and test view to use "vids" option as defined by plugin.
Filed follow-up to fix schema for that plugin #2312693: NodeTermData config schema is broken

plach’s picture

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

Thanks, but this is not ok yet: tests are not failing with patch #60 as the view is not re-executed. Please post also the failing patch so we can show @alexpott this actually covers the issue.

  1. +++ b/core/modules/taxonomy/src/Tests/Views/RelationshipNodeTermDataTest.php
    @@ -36,6 +36,20 @@ function testViewsHandlerRelationshipNodeTermData() {
    +    $this->executeView($view, array($this->term1->id(), $this->term2->id()));

    You need to add the following lines above, otherwise the view won't be executed again:

    $view->executed = FALSE;
    $view->built = FALSE;
  2. +++ b/core/modules/taxonomy/src/Tests/Views/RelationshipNodeTermDataTest.php
    @@ -36,6 +36,20 @@ function testViewsHandlerRelationshipNodeTermData() {
    +    $resultset = array(

    The expected result set is not correct: two items for each node are returned (also on HEAD).

andypost’s picture

Status:Needs work» Needs review
StatusFileSize
new2.54 KB
new749 bytes
new34.4 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,268 pass(es), 0 fail(s), and 1 exception(s).
[ View ]
new33.99 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,269 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Fixed test, @dawehner suggested to use initHandlers() and manual testing shows that

$view->executed = FALSE;
$view->built = FALSE;

does not work

#78.2 is wrong - view should return 2 rows, importing the view and running in view UI shouws that (Don't forget to provide arguments without them you will got 4 rows)

Here's fail patch to show that second part of condition now tested

PS: interdiff have a hunk to fix small bug after #2225353: Convert $form_state to an object and provide methods like setError() seems there's no test for that too, but this out of scope

Status:Needs review» Needs work

The last submitted patch, 79: 1498660-term-ml-79.patch, failed testing.

The last submitted patch, 79: 1498660-term-ml-79-fail.patch, failed testing.

andypost’s picture

Status:Needs work» Needs review
StatusFileSize
new34.65 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,354 pass(es).
[ View ]
new35.05 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,359 pass(es), 0 fail(s), and 1 exception(s).
[ View ]
new2.72 KB

Vocabulary is views_testing_tags so test is failed returning 0 results.

Fixed patch, the test is cleaned-up:
1) view should use proper 'node' table for sort, so order is actually descending
2) "magic" with view is removed in favour of direct config change

The last submitted patch, 82: 1498660-term-ml-82-fail.patch, failed testing.

plach’s picture

Status:Needs review» Reviewed & tested by the community

Looks good, thanks!

plach’s picture

Issue tags:-Needs tests
plach’s picture

Priority:Normal» Major

This is one of the most important issues for D8MI: at very least major.

alexpott’s picture

Status:Reviewed & tested by the community» Needs work
  1. +++ b/core/modules/taxonomy/src/TermStorage.php
    @@ -11,6 +11,7 @@
    +use Drupal\taxonomy\Entity\Term;

    Not used.

  2. +++ b/core/modules/taxonomy/src/TermStorage.php
    @@ -160,12 +164,12 @@ public function getSchema() {
         unset($schema['taxonomy_term_data']['indexes']['field__vid']);
         unset($schema['taxonomy_term_data']['indexes']['field__description__format']);

    Neither of these indexes exist anymore so there unsets are doing nothing. The description format field has moved to the taxonomy_term_field_data table - should we unset it on there?

plach’s picture

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new1.29 KB
new34.9 KB

should we unset it on there?

Definitely, those are needed to "match" the original (static) term schema.

plach’s picture

Displayed the wrong attachment, hopefully now the bot will pick it up for testing...

plach’s picture

StatusFileSize
new34.9 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,340 pass(es).
[ View ]

Nope...

alexpott’s picture

Status:Reviewed & tested by the community» Needs work
    unset($schema['taxonomy_term_data']['indexes']['field__vid']);
    unset($schema['taxonomy_term_data']['indexes']['field__description__format']);

So here is the funny thing on head neither of these unsets are doing anything! $schema['taxonomy_term_data']['indexes'] contains 2 keys which are taxonomy_term_field__vid__target_id and taxonomy_term_field__description__format.

CREATE TABLE `taxonomy_term_data` (
  `tid` int(10) unsigned NOT NULL AUTO_INCREMENT COMMENT 'The term ID.',
  `vid` varchar(32) NOT NULL COMMENT 'The vocabulary to which the term is assigned.',
  `uuid` varchar(128) NOT NULL COMMENT 'The term UUID.',
  `langcode` varchar(12) NOT NULL COMMENT 'The term language code.',
  `name` varchar(255) NOT NULL COMMENT 'The term name.',
  `description__value` longtext COMMENT 'A description of the term.',
  `description__format` varchar(255) DEFAULT NULL COMMENT 'A description of the term.',
  `weight` int(11) NOT NULL COMMENT 'The weight of this term in relation to other terms.',
  `changed` int(11) DEFAULT NULL COMMENT 'The time that the term was last edited.',
  PRIMARY KEY (`tid`),
  UNIQUE KEY `taxonomy_term_field__uuid__value` (`uuid`),
  KEY `taxonomy_term_field__vid__target_id` (`vid`),
  KEY `taxonomy_term_field__description__format` (`description__format`),
  KEY `taxonomy_term__tree` (`vid`,`weight`,`name`),
  KEY `taxonomy_term__vid_name` (`vid`,`name`),
  KEY `taxonomy_term__name` (`name`)
) ENGINE=InnoDB DEFAULT CHARSET=utf8 COMMENT='The base table for taxonomy_term entities.'

Above is the current result of show create table taxonomy_term_data;

With this patch we have two tables:

CREATE TABLE `taxonomy_term_data` (
  `tid` int(10) unsigned NOT NULL AUTO_INCREMENT COMMENT 'The term ID.',
  `vid` varchar(32) NOT NULL COMMENT 'The vocabulary to which the term is assigned.',
  `uuid` varchar(128) NOT NULL COMMENT 'The term UUID.',
  `langcode` varchar(12) NOT NULL COMMENT 'The term language code.',
  PRIMARY KEY (`tid`),
  UNIQUE KEY `taxonomy_term_field__uuid__value` (`uuid`),
  KEY `taxonomy_term_field__vid__target_id` (`vid`)
) ENGINE=InnoDB DEFAULT CHARSET=utf8 COMMENT='The base table for taxonomy_term entities.'
CREATE TABLE `taxonomy_term_field_data` (
  `tid` int(10) unsigned NOT NULL COMMENT 'The term ID.',
  `vid` varchar(32) NOT NULL COMMENT 'The vocabulary to which the term is assigned.',
  `langcode` varchar(12) NOT NULL COMMENT 'The term language code.',
  `name` varchar(255) NOT NULL COMMENT 'The term name.',
  `description__value` longtext COMMENT 'A description of the term.',
  `description__format` varchar(255) DEFAULT NULL COMMENT 'A description of the term.',
  `weight` int(11) NOT NULL COMMENT 'The weight of this term in relation to other terms.',
  `changed` int(11) DEFAULT NULL COMMENT 'The time that the term was last edited.',
  `default_langcode` tinyint(4) NOT NULL DEFAULT '1' COMMENT 'Boolean indicating whether field values are in the default entity language.',
  PRIMARY KEY (`tid`,`langcode`),
  KEY `taxonomy_term_field__vid__target_id` (`vid`),
  KEY `taxonomy_term_field__description__format` (`description__format`),
  KEY `taxonomy_term__tree` (`vid`,`weight`,`name`),
  KEY `taxonomy_term__vid_name` (`vid`,`name`),
  KEY `taxonomy_term__name` (`name`)
) ENGINE=InnoDB DEFAULT CHARSET=utf8 COMMENT='The data table for taxonomy_term entities.'

With this patch should we be unsetting $schema['taxonomy_term_data']['indexes']['taxonomy_term_field__vid__target_id'], $schema['taxonomy_term_field_data']['indexes']['taxonomy_term_field__vid__target_id'] and $schema['taxonomy_term_field_data']['indexes']['taxonomy_term_field__description__format']? Actually I think we should leave the $schema['taxonomy_term_data']['indexes']['taxonomy_term_field__vid__target_id'] alone since this index will be useful for functions like TermStorage::nodeCount().

+++ b/core/modules/taxonomy/taxonomy.views.inc
@@ -132,7 +140,7 @@ function taxonomy_views_data() {
-  $data['taxonomy_term_data']['vid'] = array(
+  $data['taxonomy_term_field_data']['vid'] = array(

I think this change might be unnecessary and lead to less performant views since might be possible to not include taxonomy_term_field_data to filter by node's by vid for example.

plach’s picture

Status:Needs work» Needs review
StatusFileSize
new1.49 KB
new34.63 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 74,990 pass(es).
[ View ]

Addressed #91.

plach’s picture

Status:Needs review» Reviewed & tested by the community

Green. Back to RTBC.

alexpott’s picture

Status:Reviewed & tested by the community» Fixed

Committed d2e7d62 and pushed to 8.x. Thanks!

  • alexpott committed d2e7d62 on 8.0.x
    Issue #1498660 by marco, plach, andypost, YesCT, Berdir | dawehner:...
Gábor Hojtsy’s picture

Issue tags:-sprint

Thanks all, removing from the sprint.

Status:Fixed» Closed (fixed)

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