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.
Comment | File | Size | Author |
---|---|---|---|
#92 | 1498660-term-ml-91.patch | 34.63 KB | plach |
Comments
Comment #1
dawehnerAdd tags
Comment #2
Gábor HojtsyRetitle based on #1658712: Refactor test_entity schema to be multilingual. Marking postponed on #1691952: Make EntityFieldQuery work with multilingual properties.
Comment #3
klonos#1691952: Make EntityFieldQuery work with multilingual properties is in.
Comment #4
plachActually this is postponed to #1818560: Convert taxonomy entities to the new Entity Field API.
Comment #5
Gábor HojtsyOne more D8Mi tag.
Comment #6
effulgentsia CreditAttribution: effulgentsia commentedIssue from #4 is in, so unpostponing.
Comment #7
andypostAnything left here?
Comment #8
Gábor HojtsyYeah, 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.
Comment #9
andypost@Gabor, please update issue summary with proposed solution. I'll attack this on devDays
Comment #10
Gábor HojtsyThis would apply the same property changes like #1498674: Refactor node properties to multilingual.
Comment #10.0
Gábor HojtsyUpdate. Vocabularies are not content entities anymore.
Comment #11
marco CreditAttribution: marco commentedComment #12
plachWe definitely need this to happen, and we definitely want it to happen before beta.
Comment #13
BerdirWith 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)
Comment #14
marco CreditAttribution: marco commentedI 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.
Comment #17
plach@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).
Comment #18
BerdirI 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.
Comment #19
BerdirSorry, wrong issue :)
Comment #20
Berdir@plach: Yeah I know, I was mostly trying to point out why we do need those issues :)
Comment #21
Gábor HojtsyWell, the question is how big of a schema change will this be and whether that is beta blocking in some way.
Comment #22
plachThis 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.
Comment #23
Gábor HojtsyPut on D8MI sprint. This is an important missing piece for us.
Comment #24
marco CreditAttribution: marco commentedI'm going to work on this during this weekend, or early next week worst case.
Comment #25
marco CreditAttribution: marco commentedComment #27
plachThis 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.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.
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.Comment #28
plachComment #29
marco CreditAttribution: marco commentedThanks for the patience, here's a new patch.
Comment #30
marco CreditAttribution: marco commentedComment #32
marco CreditAttribution: marco commentedComment #34
marco CreditAttribution: marco commentedComment #36
marco CreditAttribution: marco commentedComment #37
plachThanks, 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.
Minor, but can we rename the $term_record variable to $term? It's really confusing now.
This is not returning a value. I guess this code is not test-covered atm.
A load multiple here would be nice :)
Should be:
"(optional) A vocabularies array to restrict the terms search. Defaults to ..."
I'd call this
::getNodeTerms()
. Also, we could add an optionallangcode
parameter and filter to the list with its value if specified.Missing trailing dot.
Comment #38
andypostloading a whole tree of entities... incredible for filter
+1 it's no more a record
Comment #39
plachNot sure how this is working: it is not a field so how is it getting loaded?
@andypost:
It's just the filter configuration form, I think.
Comment #40
marco CreditAttribution: marco commentedComment #42
marco CreditAttribution: marco commented@andypost:
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
Yes.
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().
Comment #43
marco CreditAttribution: marco commentedComment #44
YesCT CreditAttribution: YesCT commentedfor those following along. :)
tree depth changes that were in 42
[edit: and the fix for the fail was change getTranslatedTerm() to getTranslatedName()]
Comment #45
mon_franco CreditAttribution: mon_franco commented@YesCT I have reviewed the patch using codersniffer, all inline comments are ok.
Comment #46
andypostIs there any reason to change
TermInterface
in that conversion?About loading all entities - suppose there should be other solution
Comment #47
YesCT CreditAttribution: YesCT commentedjust 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
Comment #48
YesCT CreditAttribution: YesCT commentedtook 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
Comment #49
plachI 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.These are definitely not test covered, I think :)
We can just do
$this->loadMultiple()
here.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:
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:
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.
Comment #50
plachComment #51
marco CreditAttribution: marco commentedIf 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.
Comment #52
plachSorry, 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...Comment #53
marco CreditAttribution: marco commentedNever mind, it works. I'll post a new patch today.
Comment #54
marco CreditAttribution: marco commented* restored $term->depth
* removed getTranslatedName()
* other review fixes
Comment #55
YesCT CreditAttribution: YesCT commentedrerolled for #2298309: clean up comments in taxonomy.views.inc.
conflict lines were around deleted comments, mostly just context lines in the patch.
nothing tricky.
Comment #56
plachChanges 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.
Comment #57
Gábor HojtsyWoot, comments landed, so we are now down to this one and menus :) Menu is a beta blocker.
Comment #58
andypostTested 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
Comment #59
plachThanks! Agreed on the follow-up, I will write the change record later if no one beats me to it.
Comment #60
plachI realized we did not have explicit test coverage for this change, someone please confirm the interdiff is good.
Working on the CR now.
Comment #61
jibranIs #569434: Remove taxonomy term description field; provide description field for forum taxonomy needed anymore?
Comment #62
plachDraft 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.
Comment #63
plachCreated follow-ups
Comment #64
Gábor HojtsyThe 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.
Comment #65
alexpottin Drupal\node\Plugin\views\wizard::buildFilters is now wrong.
in taxonomy_get_tree() docs is now wrong.
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.
Comment #66
plachThis should address #65. I manually tested it with a node view and a term relationship to display term names and everything worked fine.
Comment #68
andypostLast failures caused by
taxonomy_index
does not have languageComment #69
andypostFix broken tests
PS: while debuggin filed patch to #2239227: Views GroupwiseMax class calls protected properties
Comment #70
plachLooks good, thanks! I tested the the node relationship and it's working as expected.
Comment #71
alexpottGiven that the patch in #60 did not fail we must be missing test coverage - let's add it.
Comment #72
plachI think that would be pretty out of scope here: can't we do that in a separate, non beta-deadline-bound issue?
Comment #73
andypost@alexpott Please elaborate what needs tests?
@plach if you know that, please file follow-ups
Comment #74
plach@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 :)
Comment #75
plachAfter reading #71 again, it's clear Alex refers to views integration.
Comment #76
plach@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.
Comment #77
andypostExtended 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
Comment #78
plachThanks, 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.
You need to add the following lines above, otherwise the view won't be executed again:
The expected result set is not correct: two items for each node are returned (also on HEAD).
Comment #79
andypostFixed test, @dawehner suggested to use
initHandlers()
and manual testing shows thatdoes 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
Comment #82
andypostVocabulary 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
Comment #84
plachLooks good, thanks!
Comment #85
plachComment #86
plachThis is one of the most important issues for D8MI: at very least major.
Comment #87
alexpottNot used.
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?
Comment #88
plachDefinitely, those are needed to "match" the original (static) term schema.
Comment #89
plachDisplayed the wrong attachment, hopefully now the bot will pick it up for testing...
Comment #90
plachNope...
Comment #91
alexpottSo here is the funny thing on head neither of these unsets are doing anything!
$schema['taxonomy_term_data']['indexes']
contains 2 keys which aretaxonomy_term_field__vid__target_id
andtaxonomy_term_field__description__format
.Above is the current result of
show create table taxonomy_term_data;
With this patch we have two tables:
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 likeTermStorage::nodeCount()
.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.
Comment #92
plachAddressed #91.
Comment #93
plachGreen. Back to RTBC.
Comment #94
alexpottCommitted d2e7d62 and pushed to 8.x. Thanks!
Comment #96
Gábor HojtsyThanks all, removing from the sprint.