#367 | 2543726-367-skip-update-if-already-ran.patch | 817 bytes | effulgentsia |
|
#355 | 2543726-355.patch | 68.52 KB | bander2 |
|
#353 | 2543726-353.patch | 3.57 KB | bander2 |
|
#311 | Screenshot-2018-1-15 Multiple parent filters Site-Install.png | 13.2 KB | larowlan |
#311 | Screenshot-2018-1-15 Multiple parent filters (Content) Site-Install.png | 37.5 KB | larowlan |
#309 | interdiff-2543726-309.txt | 7.47 KB | damiankloip |
#309 | 2543726-309.patch | 68.8 KB | damiankloip |
|
#300 | Screenshot-2018-1-12 Multiple parent filters (Content) Site-Install.png | 19.46 KB | larowlan |
#299 | after.png | 15.85 KB | larowlan |
#299 | before.png | 15.09 KB | larowlan |
#296 | Screenshot-2018-1-12 Tags drupal 8 5 x.png | 30.48 KB | larowlan |
#296 | Screenshot-2018-1-12 Tags drupal 8 5 x(1).png | 13.16 KB | larowlan |
#296 | Screenshot-2018-1-12 content drupal 8 5 x.png | 11.97 KB | larowlan |
#296 | Screenshot-2018-1-12 content drupal 8 5 x(1).png | 9.92 KB | larowlan |
#290 | interdiff-290.txt | 2.16 KB | amateescu |
#290 | 2543726-290.patch | 65.95 KB | amateescu |
|
#288 | 2543726-288.patch | 70.36 KB | Wim Leers |
|
#288 | interdiff-280-283.txt | 2.75 KB | Wim Leers |
#285 | interdiff-285.txt | 7.05 KB | amateescu |
#285 | 2543726-285.patch | 63.17 KB | amateescu |
|
#283 | 2543726-283.patch | 69.1 KB | Wim Leers |
|
#283 | interdiff-280-283.txt | 2.75 KB | Wim Leers |
#280 | interdiff-2543726-280.txt | 3.4 KB | damiankloip |
#280 | 2543726-280.patch | 63.77 KB | damiankloip |
|
#278 | interdiff-2543726-278.txt | 12.03 KB | damiankloip |
#278 | 2543726-278.patch | 62.14 KB | damiankloip |
|
#274 | interdiff-2543726-274.txt | 9.51 KB | damiankloip |
#274 | 2543726-274.patch | 54.29 KB | damiankloip |
|
#271 | interdiff-2543726-271.txt | 3.53 KB | damiankloip |
#271 | 2543726-271.patch | 57.97 KB | damiankloip |
|
#268 | interdiff-2543726-268.txt | 611 bytes | damiankloip |
#268 | 2543726-268.patch | 57.8 KB | damiankloip |
|
#260 | interdiff-2543726-260.txt | 6.43 KB | damiankloip |
#260 | 2543726-260.patch | 57.66 KB | damiankloip |
- 8.5.x:
- PHP 7 & MySQL 5.5 22,980 pass, 15 fail
- PHP 7.1 & MySQL 5.5 22,952 pass, 15 fail
- PHP 5.5 & MySQL 5.5 21,073 pass, 15 fail
- PHP 5.6 & MySQL 5.5 21,075 pass, 15 fail
- PHP 7 & PostgreSQL 9.1 22,978 pass, 15 fail
- PHP 7 & SQLite 3.14 22,977 pass, 15 fail
|
#257 | interdiff-2543726-257.txt | 1.12 KB | damiankloip |
#257 | 2543726-257.patch | 60.67 KB | damiankloip |
|
#254 | 2543726-254.patch | 63.58 KB | Wim Leers |
|
#254 | interdiff.txt | 1.93 KB | Wim Leers |
#249 | 2543726-249.patch | 64.04 KB | Wim Leers |
|
#249 | interdiff.txt | 2.34 KB | Wim Leers |
#245 | 2543726-245.patch | 63.52 KB | Wim Leers |
|
#245 | interdiff.txt | 1.08 KB | Wim Leers |
#238 | interdiff-216-237.txt | 12.8 KB | Wim Leers |
#237 | 2543726-237.patch | 63.71 KB | Wim Leers |
|
#237 | interdiff.txt | 856 bytes | Wim Leers |
#236 | 2543726-236.patch | 63 KB | Wim Leers |
|
#236 | interdiff-232-236.txt | 755 bytes | Wim Leers |
#234 | interdiff-2543726-232-234.patch | 4.07 KB | lawxen |
|
#234 | 2543726-234.patch | 63.06 KB | lawxen |
|
#232 | interdiff-2543726.txt | 610 bytes | dawehner |
#232 | 2543726-232.patch | 60.29 KB | dawehner |
|
#230 | 2543726-230.patch | 62.99 KB | Wim Leers |
|
#230 | interdiff.txt | 701 bytes | Wim Leers |
#228 | interdiff-2543726.txt | 671 bytes | dawehner |
#228 | 2543726-228.patch | 60.28 KB | dawehner |
|
#223 | 2543726-223.patch | 62.98 KB | Wim Leers |
|
#223 | interdiff.txt | 10.4 KB | Wim Leers |
#222 | 2543726-222.patch | 65.22 KB | Wim Leers |
|
#222 | interdiff.txt | 1.08 KB | Wim Leers |
#216 | 2543726-216.patch | 65.04 KB | Wim Leers |
|
#216 | interdiff.txt | 10.18 KB | Wim Leers |
#205 | interdiff-2543726-205.txt | 611 bytes | damiankloip |
#205 | 2543726-205.patch | 57.81 KB | damiankloip |
|
#203 | interdiff-2543726-203.txt | 3.89 KB | damiankloip |
#203 | 2543726-203.patch | 57.66 KB | damiankloip |
|
#200 | interdiff-2543726-199.txt | 2 KB | damiankloip |
#200 | 2543726-199.patch | 58.12 KB | damiankloip |
|
#195 | interdiff-2543726-195.txt | 8.96 KB | damiankloip |
#195 | 2543726-195.patch | 56.13 KB | damiankloip |
|
#192 | interdiff-2543726-192.txt | 1.98 KB | damiankloip |
#192 | 2543726-192.patch | 54.25 KB | damiankloip |
|
#190 | interdiff-2543726-190.txt | 1.29 KB | damiankloip |
#190 | 2543726-190.patch | 54.17 KB | damiankloip |
|
#188 | interdiff-2543726-188.txt | 730 bytes | damiankloip |
#188 | 2543726-188.patch | 54.32 KB | damiankloip |
|
#186 | interdiff-2543726-186.txt | 14.35 KB | damiankloip |
#186 | 2543726-186.patch | 54.31 KB | damiankloip |
|
#179 | make_term_parent-2543726-179.patch | 54.69 KB | Wim Leers |
|
#174 | make_term_parent-2543726-174.patch | 54.8 KB | jibran |
|
#174 | interdiff-ad8d93.txt | 8.12 KB | jibran |
#171 | 2543726-171.patch | 55.71 KB | Wim Leers |
|
#171 | interdiff.txt | 930 bytes | Wim Leers |
#169 | 2543726-168.patch | 55.7 KB | Wim Leers |
|
#169 | interdiff.txt | 8.17 KB | Wim Leers |
#167 | Screen Shot 2017-10-12 at 11.19.41.png | 9.4 KB | Wim Leers |
#163 | interdiff-2543726-163.txt | 941 bytes | damiankloip |
#163 | 2543726-163.patch | 47.98 KB | damiankloip |
|
#161 | interdiff-2543726-161.txt | 5.36 KB | damiankloip |
#161 | 2543726-161.patch | 47.98 KB | damiankloip |
|
#159 | interdiff-2543726-159.txt | 733 bytes | damiankloip |
#159 | 2543726-159.patch | 47.18 KB | damiankloip |
|
#151 | interdiff-2543726-151.txt | 4.08 KB | damiankloip |
#151 | 2543726-151.patch | 47.12 KB | damiankloip |
|
#146 | 2543726-146-for-8.3.x-do-not-test.patch | 46.78 KB | idebr |
|
#143 | 2543726-143.patch | 46.66 KB | tedbow |
|
#143 | interdiff-141-143.txt | 27.56 KB | tedbow |
#141 | drupal-term_parent_entity_reference-2543726-141.patch | 74.51 KB | mgalalm |
|
#139 | interdiff-2543726-128-139.txt | 17.77 KB | mgalalm |
#139 | drupal-term_parent_entity_reference-2543726-139.patch | 46.63 KB | mgalalm |
|
#130 | interdiff-25437326-108-130.txt | 4.75 KB | voleger |
#130 | drupal-term_parent_entity_reference-25437326-130.patch | 46.11 KB | voleger |
|
#128 | interdiff-25437326-108-128.txt | 5.74 KB | voleger |
#128 | drupal-term_parent_entity_reference-25437326-128.patch | 46.52 KB | voleger |
|
#127 | interdiff-25437326-108-125.txt | 3.37 KB | voleger |
#125 | drupal-term_parent_entity_reference-25437326-125.patch | 46.26 KB | voleger |
|
#118 | drupal-term_parent_entity_reference-25437326-118.patch | 45.41 KB | voleger |
|
#115 | drupal-term_parent_entity_reference-25437326-115.patch | 45.4 KB | voleger |
|
#114 | drupal-term_parent_entity_reference-25437326-114.patch | 45.23 KB | voleger |
|
#110 | drupal-term_parent_entity_reference-25437326-110.patch | 45.34 KB | voleger |
|
#108 | drupal-term_parent_entity_reference-25437326-108.patch | 46.89 KB | Grimreaper |
|
#100 | interdiff_joint_91_93_95.txt | 2.44 KB | Wim Leers |
#96 | interdiff-2543726-95.txt | 506 bytes | damiankloip |
#96 | 2543726-95.patch | 46.25 KB | damiankloip |
|
#93 | interdiff-2543726-93.txt | 909 bytes | damiankloip |
#93 | 2543726-93.patch | 46.24 KB | damiankloip |
|
#91 | interdiff-2543726-91.txt | 2.15 KB | damiankloip |
#91 | 2543726-91.patch | 46.15 KB | damiankloip |
|
#89 | interdiff-2543726-89.txt | 8.7 KB | damiankloip |
#89 | 2543726-89.patch | 44.43 KB | damiankloip |
|
#82 | interdiff-2543726-82.txt | 6.02 KB | damiankloip |
#82 | 2543726-82.patch | 50.22 KB | damiankloip |
|
#80 | interdiff-2543726-2543726-80.txt | 3.67 KB | damiankloip |
#80 | 2543726-80.patch | 46.26 KB | damiankloip |
|
#78 | term_parent-2543726-78-term-do-not-test.patch | 43.49 KB | pcambra |
|
#74 | Selection_294.png | 30.57 KB | dagmar |
#71 | interdiff-2015149-70-71.txt | 1.59 KB | dagmar |
#71 | dblog-2015149-71.patch | 43.87 KB | dagmar |
|
#70 | interdiff-66-70.txt | 3.08 KB | tedbow |
#70 | 2543726-70.patch | 45.45 KB | tedbow |
|
#69 | term_parent-2543726-69-term-do-not-test.patch | 42.41 KB | dmouse |
|
#66 | interdiff-2543726-63-66.txt | 6.41 KB | dagmar |
#66 | term_parent-2543726-66.patch | 42.38 KB | dagmar |
|
#63 | interdiff-2543726-54-62.txt | 589 bytes | FredCorreia |
#63 | term_parent-2543726-62.patch | 42.3 KB | FredCorreia |
|
| interdiff-2543726-54-62.txt | 589 bytes | FredCorreia |
| term_parent-2543726-62.patch | 42.3 KB | FredCorreia |
|
#54 | interdiff-2543726-45-54.txt | 3.07 KB | dagmar |
#54 | term_parent-2543726-54.patch | 42.29 KB | dagmar |
|
#45 | interdiff.txt | 5.26 KB | claudiu.cristea |
#45 | term_parent-2543726-45.patch | 42.03 KB | claudiu.cristea |
|
#44 | 2627678-42.patch | 22.93 KB | claudiu.cristea |
|
#44 | interdiff.txt | 5.26 KB | claudiu.cristea |
#41 | interdiff.txt | 7 KB | claudiu.cristea |
#41 | term_parent-2543726-41.patch | 41.87 KB | claudiu.cristea |
|
#39 | term_parent-2543726-38.patch | 42.05 KB | claudiu.cristea |
|
#38 | interdiff.txt | 19.05 KB | claudiu.cristea |
#38 | interdiff.txt | 19.05 KB | claudiu.cristea |
#34 | issue-2543726-comment-34.patch | 43.19 KB | dbjpanda |
|
#11 | interdiff.txt | 1.97 KB | claudiu.cristea |
#11 | 2543726-11.patch | 42.69 KB | claudiu.cristea |
|
#9 | 2543726-9.patch | 42.72 KB | claudiu.cristea |
|
#9 | interdiff.txt | 9.64 KB | claudiu.cristea |
#5 | 2543726-5.patch | 42.09 KB | claudiu.cristea |
|
#1 | 2543726-1.patch | 2.05 KB | claudiu.cristea |
|
Comments
Comment #1
claudiu.cristeaPatch.
Comment #3
dawehnerCan't we lazyload that somehow? I'm curious about that as additional queries to load an entity are probably problematic.
Comment #4
claudiu.cristea@dawehner, is there a reason why we are not keeping the hierarchy (parents) in the default storage? I mean like users are referring their roles.
Comment #5
claudiu.cristea@dawehner,
It took me some time but this patch is removing the custom storage in favor of default storage for term hierarchy. I agree that this makes the transformation more complex. Some benefits:
Methods that can be deprecated from storages and moved into entity classes because they are losing the SQL specific code:
TermStorageInterface::loadParents()
TermInterface::getParents()
TermStorageInterface::loadAllParents()
TermInterface::getAncestors()
TermStorageInterface::loadChildren()
TermInterface::getChildren()
VocabularyStorageInterface::getToplevelTids()
VocabularyInterface:: getTopLevelTids()
Comment #6
dawehnerYou also need to update existing views ...
That looks a bit odd :)
Is there a reason we need a leftJoin?
Comment #9
claudiu.cristea{taxonomy_term__parent}
. Fixed the join.Comment #11
claudiu.cristeaFixed the failure.
Comment #12
claudiu.cristeaComment #18
chx CreditAttribution: chx commentedin
preSave
Is this valid? For example, what would various widgets do when referring a nonexisting entity?
Comment #19
dawehnerIf I understand
correctly then we should set NULL ...
Comment #20
claudiu.cristea@chx, @dawehner,
#19 tells us that 0 (strict) is a non-empty value. That is good because we need to represent somehow the <root> parent.
NULL
would be the empty value and that is not stored in the backend. So, 0 (strict), is stored and we want that in order to represent the <root> pseudo-reference.Off-topic, it seems that
is never triggered.
Comment #25
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedThis falls a lot more into how taxonomy module works rather than entity reference..
Comment #27
g089h515r806 CreditAttribution: g089h515r806 commentedFor taxonomy term:
why not remove the "taxonomy_term_hierarchy" and add parent column to table "taxonomy_term_field_data".
For menu system, the parent is store at one table "menu_tree", but for taxonomy term, it is in a seperate table.
Which benifit we could get by using a seperate table to store parent info for taxonomy term?
Comment #28
chx CreditAttribution: chx commentedmultiple parents
Comment #29
g089h515r806 CreditAttribution: g089h515r806 commented1, The Drupal Core taxonomy UI does not support "multiple parents" feature
2, I have build 100+ Drupal websites, never use "multiple parents" feature
3, If is there any third party module that support "multiple parents" feature? I have never used one of them
If "multiple parents" feature is never used, we can remove this feature from Drupal core in the future.
Comment #30
gcardinal CreditAttribution: gcardinal commentedI dont agreed it is a good idea to remove this feature from Drupal - we are using it in a project with RESTful import right now.
Comment #31
chx CreditAttribution: chx commented> The Drupal Core taxonomy UI does not support "multiple parents" feature
It does.
Comment #32
jibranWe need a change notice for this change as well.
Then this is not a bug imo this is a task because this is an incomplete feature.
This can go to post update function now.
Comment #34
dbjpanda CreditAttribution: dbjpanda commentedRerolled the patch on 8.2.x
Comment #36
dbjpanda CreditAttribution: dbjpanda commentedCan any one please point me what is my mistake in rerolling the patch.
Comment #37
andypostI compared patches 11 vs 34 and there's few differences, maybe that caused test failures.
in 8.2.x removed 9.0.0
also first 2 ones needs to point what to use instead
Comment #38
claudiu.cristeaComment #39
claudiu.cristeaOuch! Forgot the main patch.
Comment #41
claudiu.cristeaFixed the update path thanks to @alexpott, here at Drupal DevDays Milan :)
Comment #43
chx CreditAttribution: chx commentedI am sorry and I know I should not but I will butt in: the getParents method returning a field item list is a lousy API. It should return either the target IDs or the entities but not the field item list.
Comment #44
claudiu.cristea@chx, yes, that perfectly make sense. Initially that was only a getter for
$term->get('parent')
but in order to be aligned with getAncestors() and getChildren() is more logical to be an array of entities keyed by entity id.Comment #45
claudiu.cristeaSorry, The last patch was totally wrong, from other issue.
Comment #47
chx CreditAttribution: chx commentedSo #43 was already pushing the new boundaries. I do not know what to say or how to say these days, really. Am I even supposed to give architectural advice any more? I really do not know. So let's make this very clear: this is just an opinion. You can take or leave it. Unlike #43 I have no problems if this goes in as is. With that said, I feel it might be a bit better if the new methods would live in a TermTreeStorage class and service. As far as I can see there's nothing that stops getParents, getAncestors and getChildren from living elsewhere. They are only calling public methods on a Term like id() and get() and the only protected property they are manipulating is added by and for getAncestors. Moving them out would, of course, open doors to a more efficient way to store a tree. Also I am thinking, although I really badly I don't know about these things that if you can separate a class into separate classes then you should. Single responsibility and all that.
Comment #48
andypostLooks that needs follow-up and link here
why not use at and array_unshift() root?
why only ancestors are statically cached? but other method results not?
this need issue too
otoh minor versions allows to extend interfaces
also needs issue and what chx said
this changes violates BC
Comment #49
dagmarIt seems there are several improvements to this patch. Moving to Needs work.
Comment #51
joachim CreditAttribution: joachim at Torchbox commentedThis seems to me like a fairly clear use case for a calculated entity reference field, which AFAIK isn't yet possible in core. I think I saw an issue for this, but not managing to find it right now.
Comment #52
claudiu.cristeaWhy would this be a calculated entity reference? It's a classical entity reference field in the most common way.
Comment #53
joachim CreditAttribution: joachim at Torchbox commentedYeah, sorry, ignore last comment. Not sure what I was thinking there!
Comment #54
dagmarI created 6 follow ups and linked them in the code.
I didn't change 2, 3 and 6.
Comment #55
mikeryanWill the current patch fix the fact that an entity query on 'parent' doesn't work (#2599450: Drupal\Core\Entity\Query\QueryException: 'parent' not found in Drupal\Core\Entity\Query\Sql\Tables->ensureEntityTable)? If so, that could be closed as a dupe of this. If not, that's another follow-up...
In the meantime, is there a way through the API to query on parent (or, in my case, the lack of one), apart from a database query on taxonomy_term_hierarchy?
Comment #56
mikeryanI see this patch does at least one entity query on 'parent' in the tests, so I've closed #2599450: Drupal\Core\Entity\Query\QueryException: 'parent' not found in Drupal\Core\Entity\Query\Sql\Tables->ensureEntityTable as a duplicate.
Comment #57
mikeryanI think this should be "Needs work" until the feedback in #47 and #48 is addressed.
Comment #58
Wim LeersAs https://www.previousnext.com.au/blog/we-could-add-default-content-drupal... mentions, this also affects the REST experience.
Comment #60
fgmAlso, when using this patch, adding a parent term as a view argument causes an incorrect query generation, using
taxonomy_term__parent.parent
instead oftaxonomy_term__parent.parent_target_id
, probably because that column was previouslytaxonomy_term_hierarchy.parent
.Comment #61
Wim Leers@fgm Thanks for reporting that! That indicates that needs test coverage too then. And since it affects Views, tagging
too.Comment #63
FredCorreia CreditAttribution: FredCorreia at Eurelis commentedI fixed the issue reported by @fgm in #60 by replacing a missed index value in TermViewsData.
Comment #66
dagmarRe-rolled.
Comment #69
dmouseI use this path in my current project, works weel on 8.2.x branch if anyone needs it.
Comment #70
tedbow@dagmar thanks for the re-roll!
Fixed \Drupal\Tests\rest\Functional\EntityResource\Term\TermResourceTestBase::getExpectedNormalizedEntity
because parent should have target_id => 0. See \Drupal\taxonomy\Entity\Term::preSave
Also added this to \Drupal\Tests\hal\Functional\EntityResource\Term\TermHalJsonAnonTest::getExpectedNormalizedEntity
So it is. but then our actually entity has it. thoughts?I am less sure about this fix because \Drupal\Tests\hal\Functional\EntityResource\HalEntityNormalizationTrait says that
Comment #71
dagmarIs there any reason to disable these tests?
Comment #72
tedbow@dagmar no sorry I meant to undo that before I made the patch.
I just disable them locally so I can easily just run the 1 method.
Comment #73
dagmarFrom #46:
.
From #48:
Items 2, 3 and 6.
From #61:
(This is for the views integration).
Comment #74
dagmarWhile trying to make a patch for test coverage from #61 I saw this:
The error displayed by the view is:
SQLSTATE[42S22]: Column not found: 1054 Unknown column 'taxonomy_term__parent.parent' in 'where clause': SELECT taxonomy_term_field_data.tid AS tid, taxonomy_term_field_data_taxonomy_term__parent.tid AS taxonomy_term_field_data_taxonomy_term__parent_tid FROM {taxonomy_term_field_data} taxonomy_term_field_data LEFT JOIN {taxonomy_term__parent} taxonomy_term__parent ON taxonomy_term_field_data.tid = taxonomy_term__parent.entity_id LEFT JOIN {taxonomy_term_field_data} taxonomy_term_field_data_taxonomy_term__parent ON taxonomy_term__parent.parent_target_id = taxonomy_term_field_data_taxonomy_term__parent.tid WHERE (taxonomy_term_field_data.vid IN (:db_condition_placeholder_0)) AND (taxonomy_term__parent.parent = :db_condition_placeholder_1) LIMIT 11 OFFSET 0; Array ( [:db_condition_placeholder_0] => letters [:db_condition_placeholder_1] => A )
Comment #75
Grayside CreditAttribution: Grayside at Phase2 for Norwegian Cruise Line commentedUsing this patch on a project and ran into a problem that seems possibly related to its use. Sorry for the size of this comment given it may not be relevant.
On migrating taxonomy terms, if a term is stubbed it will fail to be properly created when the migration process revisits the stubbed term. This is true whether the stub is via parent "reference" or via any other kind of reference (node -> term).
This results in an error message such as:
which I have seen in other data model/data import issue queues such as Multiversion so it is probably an issue more general than this.
Specifically, it appears that the ChangedItem code fails to find an "original" entity (despite there being a stubbed one in the database). The entity the field loads as the current one being updated is a new entity, but not marked as new, leading to the complexity of being an entity with an apparently non-existent tid that is also marked as not new. The intersection of empty
original
property with not-new status causes the error.I considered opening a new issue, but wanted to put it up for consideration here in case this issue is the cause. Unfortunately my use case requires this patch, so reproducing the problem without it would be very time consuming.
Comment #76
hchonov@Grayside, the problem you are describing is not related to the patches here but to the issue #2841311: Initialized fields of an entity clone have a reference to the original entity object instead to the cloned entity object where exactly the same problem with the ChangedItem is described.
Comment #77
Wim Leers@hchnonov++
Comment #78
pcambraAdding a patch compatible with the latest 8.2.x version
Comment #79
damiankloip CreditAttribution: damiankloip at Acquia commentedI am looking into the views issues outlined in #74
Comment #80
damiankloip CreditAttribution: damiankloip at Acquia commentedLet's see how this does, this fixes the problem for me - but still needs explicit test coverage for the fix I found.
It seems to be a generic problem with views data generated for multi-value based fields. The code is assuming that if it only have one property/column it should just use the field name, which in this case was 'parent' but this field is multi-value, so it has a dedicated table, which means the actual schema column name should be used. I think it's correct to always use this as regular base fields will have the same result as the actual $field_name we were using before. So we basically just remove the $multiple logic. So it kind of worked by chance before, because field names and storage names were always the same. As a side note, configurable fields are not affected as they are handled slightly differently in
views_views_data()
We also shouldn't need to keep the previous views data definition for the column in
\Drupal\taxonomy\TermViewsData::getViewsData()
now, it should be generated for us. This is why we have one working version and one broken version, as outlined in #71.Comment #82
damiankloip CreditAttribution: damiankloip at Acquia commentedThis should fix some(/most?) of the test failures. Not sure we actually need more tests here, this should be caught by the EntityViewsDataTest, but the expectations were tailored to return what was currently being returned, not what we expect to be returned. Also removed a bit more of the table data from TermViewsData as the generic EntityViewsData should be providing most of this now.
Comment #85
dawehnerIs there a reason we cache the ancestors but not the parents?
This check is a little bit weird, should be maybe check for
!== NULL
or so?can we typehint the array?
I'm wondering whether we could add a follow up for siblings as well
It would be nice to document the order of operations.
Is there no way to reimplement this method on the new storage?
Nitpick: Let's mention
8.3.x
This really should better have its own issue ... this reminds me of #2838040: Multi-value multi-column base fields are queried with incorrect column names in Views
Comment #86
claudiu.cristea#85.6:
@dawehner, There's nothing to implement there after parent is a normal entity reference field. The hierarchy is automatically updated in entity/field API. We have to keep the empty methods just to honour the TermStorageInterface contract.
Comment #87
dawehnerMh okay fair.
Comment #88
damiankloip CreditAttribution: damiankloip at Acquia commentedMade a new issue for the views fix to make things easier to review etc.. #2846614: Incorrect field name is used in views integration for multi-value base fields
Comment #89
damiankloip CreditAttribution: damiankloip at Acquia commentedRemoved the stuff from #2846614: Incorrect field name is used in views integration for multi-value base fields. Not sure what we should do about some of the query access related fails. This patch is not using a direct query anymore.
Comment #91
damiankloip CreditAttribution: damiankloip at Acquia commentedThis is not the prettiest, but it does ensure the same query access hooks are run when getParents() is called, just like before... and thus should fix the Drupal\taxonomy\Tests\TaxonomyQueryAlterTest There might be a better way to do this? Not sure. Either way, we need some kind of code we're not completely happy with there to keep BC I think.
Comment #93
damiankloip CreditAttribution: damiankloip at Acquia commentedThis should work... (still not saying this is the best fix).
Comment #96
damiankloip CreditAttribution: damiankloip at Acquia commentedSilly mistake. In theory this should bring it back to 2 fails, that would be fixed by #2846614: Incorrect field name is used in views integration for multi-value base fields
Comment #98
BerdirI can't see any indication why $this->list would ever be NULL or not unset? Yes, it can be an empty array, but then the foreach will just loop over it.
I know we say entity tables aren't part of the API, but this technically wasn't really an entity table before and still, just renaming and changing that might make some people very unhappy if they where querying this table.
I guess not many do, the only reference I could find outside of core is our D8 migration project.
Not sure what to do, what we could think about is to keep this as a read-only mirror of the new data structure, that we update when terms are saved.
On the other side, based on our BC rules, we are allowed to add methods to TermInterface and we've done that on a few other entity types as well in minor updates, for example contact form.
Previously, no value meant "do not update".
We partially lose that optimization now, on the other side we have a built-in optimization to not write a dedicated field table when the values didn't change, so this might actually be faster in the end?
Still, I'm wondering if we really still need that 0 special case now. We also shouldn't have any API's relying on it as it was write-only anyway, and we could have a BC for that easily and drop the item if the value is 0?
Hm.. continue indicates that it could have parent 0 *and* additional parents? Is that really something we support?
yeah, this is interesting..
I suppose we could also switch to using $parent->access('view') on the loaded terms. Not sure what's better.
There have been lots of discussions already around those tags, see #2073663: Taxonomy load functions filter for access control, #1947270: Add test coverage for the term_access query tag and #2073663: Taxonomy load functions filter for access control.
Maybe @xjm has some thoughts on this.
Ah, so here is one example where we need the explicit 0 to be stored. an isEmpty() might also work, not sure..
Do we lose the static cache here? Is it common that this is called multiple times per request? The terms themself are statically cached, as are the values, so the only overhead would be the repeated entity queries for access checks.
And the static cache indicates that the access checking could actually be flawed anyway, as it doesn't check the current user, so if you load terms with another current user, you'd get the wrong ones...
why not addField(), this isn't really an expression?
That load_entities stuff is a mess :-/
Tried to go through the code and calls a bit, to better understand this and I think it makes sense, as unsetting on the loaded term and then saving would remove the parents.
Just one interesting side-note, it obviously
doesn't work with translations as it hardcodes the default_langcode file, so anyplaces that uses this is broken on multilingual sites. Looks like there are only 3 non-test calls, in the term form for the parents and some helper function, which doesn't care about it, but at least the parent selection should...
is vocabulary really the right place to move this? Why not just switch to an entity query here and deprecate the method?
Comment #99
Wim LeersSo #89 has failures in
TaxonomyQueryAlterTest
.The joint interdiff of #91 + #93 + #95 is what needs to be reviewed.
Review of #91+#93+#95 in particular:
\Drupal\taxonomy\Entity\Term::getParents()
+\Drupal\taxonomy\Entity\Term::getChildren()
+\Drupal\taxonomy\Entity\Vocabulary::getTopLevelTids()
TermStorage::loadParents()
now callsTerm::getParents()
, it doesn't execute an entity query anymore. Nor does it need, nor did it ever need to: those always could and should have been loaded via$term->parent
. However, this indeed breaks BC forTermStorage::loadParents()
. So what about this: we keepTermStorage::loadParents()
and don't change its logic, but deprecate it. That means no BC break, no crazy work-arounds for tests.Term::getChildren()
andTerm::getAncestors()
. Here I have to agree with #47: it's not clear why this is being moved onto theTerm
class other than convenience.\Drupal\taxonomy\TermHierarchyInterface
.In short: I think having
Term::getParents()
might make sense because\Drupal\taxonomy\Entity\Term::baseFieldDefinitions
has aparent
entity reference field (with unlimited cardinality). I don't understand why this justifies a new method though. I think it makes sense to remove thesetCustomStorage(TRUE)
for theparent
base field so that you can do$term->parent->target_id
(if a term has a single parent). But I don't see how that causes the need for all these other method additions and API changes.Overall patch review:
Why does it strip out '0' references?
Let's use strict equality.
I'd like to see explicit test coverage for this in for vocabularies that have "multiple parents" enabled.
Because AFAICT the current logic will return
A, B, C, D, E
in case you have these two lineages:in other words: there is no way to restore the lineage.
I guess that perhaps that's the point?
Should we then also add
\Drupal\taxonomy\TermHierarchyInterface::getLineages()
?Why the term "top-level" and not "root"? Is ther e a precedent?
Nice!
AFAIK we have one of these tests per update path.
So this should be renamed to something like
TaxonomyTermParentUpdatePathTest
or something like that.I can confirm that in manual testing this also worked as expected! :)
Impressive!
This is just returning all children, in no particular order.
This used to return children in a particular order, but not anymore. AFAICT that's a BC break.
This change is not actually necessary.
Comment #100
Wim LeersHere is that joint interdiff to make life easier for other reviewers.
Comment #101
Wim LeersSo what I'm proposing in #99 is:
$term->parent
work, by removing->setCustomStorage(TRUE);
TermStorage
— it should have zero changesTermHierarchyInterface
and thus also remove its changes fromTerm
.Term
should only have the necessary changes for point 1.All the things in points 2 and 3 are huge scope creep. They may have made sense in Q3 2015, when D8 was not yet out. But now that D8 is out, this would all cause a huge BC break. With very little gain. If we want to to do those things, they should happen in a separate issue, as a feature request.
Comment #102
dawehnerAt least for me using the actual APIs instead of calling out to the DB directly is a prove that things are actually working as expected. If we still call out to the DB, the DB could be still wrong.
Comment #103
Wim LeersI think we should not call the DB for
$term->parent
, but we should forTermStorage::loadParents()
. BecauseTermStorage::loadParents()
calls the DB in HEAD, and changing that would break BC. Plus, changing that is not necessary to solve the problem described in the issue summary.Comment #104
claudiu.cristea@Wim Leers, yes, the patch was designed before 8 was out. I agree, that in this stage, we should restrict the changes to #101. Let's see if I understood well:
EDIT:
Term::getParents()
,Term::getAncestors()
andTerm::getChildren()
were created to replace the corresponding storage methods because, by making $term->parent a regular entity reference field, these methods become agnostic of the storage being used.Comment #105
claudiu.cristeaRelated #2850691: Deprecate TermInterface::getVocabularyId().
Comment #106
Wim Leers#104: glad to hear you agree :)
Comment #107
Wim LeersSadly, #2846614: Incorrect field name is used in views integration for multi-value base fields has been blocking this for exactly a month now :( (See #96.) Updating status field to reflect the actual current state.
Comment #108
GrimreaperHello,
Rebasing the patch from comment #2543726-96: Make $term->parent behave like any other entity reference field, to fix REST and Migrate support and de-customize its Views integration against the last 8.3.x branch as it does not apply anymore.
Comment #109
khiminrm CreditAttribution: khiminrm commented@Grimreaper, thanks!
I've successfully applied your patch to 8.3.2.
Comment #110
volegerTried to update patch from #108 to fix broken handlers for relationships in views.
Updated taxonomy_update_8203().
Updated TermViewsData.
Comment #111
mikl CreditAttribution: mikl at Högh Digital commented#107: Sorry to be a nag, but how long are we going to wait for #2846614: Incorrect field name is used in views integration for multi-value base fields? Views integration is nice, but this is a significant bug in Drupal’s core entities.
I’m currently forced to choose between having to add custom SQL queries to load this data for a template, or having to go live with a patch that alters the database structure. Either way is likely to break my site on upgrade.
Not getting this fixed for 8.4 would be a tragedy, in my opinion.
Comment #112
Wim Leers#111: I could not agree more.
Comment #113
khiminrm CreditAttribution: khiminrm commentedHi!
Please, could anyone say, will we have possibility to upgrade core in future after applying this patch on the project?
Or a way for fixing this issue could be dramatically changed and we will not have any chance to upgrade core without necessity to make a lot of work for moving back all changes in a database made by the patch?
And maybe someone can give advice how we can properly fix relations by parent in existing views for this moment using patch from https://www.drupal.org/node/2543726#comment-12031743
Thanks!
Comment #114
volegerThat patch fixes broken handlers.
And I want notice that ./core/modules/forum/tests/src/Functional/ForumTest.php:436 used query where taxonomy_term_hierarchy table is used. Is it correct to update that query using this patch?
Comment #115
volegerFix incorrect fields name for argument and sort handlers
Comment #116
volegerIf someone can review the latest patch?
I just checked that with this patch can successfully pass test Drupal\taxonomy\Tests\Views\TaxonomyRelationshipTest.
Patch works perfectly for me during the last week.
Comment #117
edaa CreditAttribution: edaa commentedworks for me!
just one code indention issue:
$ids[] needs to indent
Comment #118
volegerAdded indention
Comment #119
ericjgruber CreditAttribution: ericjgruber as a volunteer commentedWorks for me as well. Thanks for the fix!
Comment #125
volegerUpdated ForumTest
https://www.drupal.org/node/2543726#comment-12092088
Comment #126
damiankloip CreditAttribution: damiankloip at Acquia commented@volegar there are no interdiffs between patches so we're not sure what you changed each time without manually diffing each patch :/
Comment #127
volegerComment #128
volegerUpdated mention 8.3.x
Updated install file. Dropping the legacy table moved to separate hook_update to decrease possible error messages while update database process.
Comment #129
damiankloip CreditAttribution: damiankloip at Acquia commentedI am good with modifying the views data for now, but the views issue we were depending on should be mentioned where you have added those changes IMO, we can then update the code when it's actually fixed. I am not sure how this works with BC as this is now just working around the bug from that issue by forcing the field, but still with the incorrect field name ('parent').
Comment #130
volegerSo as I understand when https://www.drupal.org/node/2846614 will be fixed, this patch after reroll should pass all the tests.
Comment #132
volegerComment #133
Wim LeersSee #2882717-12: EntityResourceTestBase: assert that PATCHed and POSTed entities contain the intended values —
$term->parent
always being NULL is causing problems there too. That's adding a new@todo
that we'll be able to fix in this issue :)Comment #134
Wim LeersAFAICT #1915056: Use entity reference for taxonomy parents is the root cause of this. It's not that issue's fault though; it the sensible thing for its scope. This is something that only a Serialization/REST API would uncover. So the problem is that Serialization/REST's test coverage was too thin.
Comment #135
dawehnerBack to normal mode
Comment #136
jibran#2846614: Incorrect field name is used in views integration for multi-value base fields still needs some tag clearance.
Comment #138
FredCorreia CreditAttribution: FredCorreia at Eurelis for Carrefour commentedCan we add conditions in different taxonomy_update_83** to not make some changes if they have already been done by installing the previous patch (like the 108) ?
Because these changes will return errors and fail the update process. For example changes in database done in taxonomy_update_8302.
Comment #139
mgalalm CreditAttribution: mgalalm as a volunteer and at Johnson & Johnson commentedLocation of tests files have been changed, updating the patch upon.
Comment #141
mgalalm CreditAttribution: mgalalm as a volunteer and at Johnson & Johnson commentedAttached is a new patch created with GIT rather than the patch command.
Comment #142
tedbowThere are a number of *.orig files that are probably just left over from using the patch command. They need to be removed.
$display here is never used so definitely doesn't need to be by reference. Also could just use
array_keys($view->('display'))
in the foreach instead.2 extra blank links. Only need 1 between paragraphs.
Comment should appear before statement. Never after.
Comment #143
tedbowFixed items in my review in #142.
Also fixed test failure in #141 which was problems in core/modules/taxonomy/tests/src/Functional/Update/TaxonomyUpdateTest.php
Incorrect namespace, and incorrect directories for requiring files.
Comment #144
Wim Leers#130 and earlier said this was blocked on #2846614: Incorrect field name is used in views integration for multi-value base fields. @dawehner removed the
[PP-1]
prefix in #135. Perhaps that was accidental?I'm wondering now why #143 is passing without #2846614: Incorrect field name is used in views integration for multi-value base fields?
Comment #145
FredCorreia CreditAttribution: FredCorreia at Eurelis for Carrefour commentedIn file taxonomy.install (with last patch #143) :
We could test if the table 'taxonomy_term_hierarchy' exists to prevent any process on it and throw errors on some Drupal installation.
With a
if ($database->schema()->tableExists('taxonomy_term_hierarchy'))
before the line 32Comment #146
idebr CreditAttribution: idebr at ezCompany commentedAttached patch is rerolled against Drupal 8.3.x for those who wish to test the upgrade path against the current stable release. The 'do-not-test' suffix should tell the testbot to leave it alone.
Comment #147
tedbow@FredCorreia re #145 I am not sure in what case
taxonomy_term_hierarchy
would not exist at that point.The table will not be dropped until
taxonomy_update_8304
re @Wim Leers' question in #144
Looked into this and I think it is because in
In #96 @damiankloip said
It did the only failing test was
\Drupal\Tests\taxonomy\Functional\Views\TaxonomyRelationshipTest::testTaxonomyRelationships
This test failed again in #108
But then in #114
testTaxonomyRelationships()
no longer failed.It think this is because in #96 in
\Drupal\taxonomy\TermViewsData::getViewsData()
it hadbut then in #114 this had changed to
Note the key change from 'parent_target_id' to 'parent'
Patches after that kept that change and
testTaxonomyRelationships()
has not failed since.I am still try to get back into this issue but maybe someone else can figure out if we should change it back and postpone this issue on #2846614: Incorrect field name is used in views integration for multi-value base fields
Comment #148
Wim Leers#147: are you saying that those changes in
\Drupal\Tests\taxonomy\Functional\Views\TaxonomyRelationshipTest::testTaxonomyRelationships()
would no longer be necessary if #2846614: Incorrect field name is used in views integration for multi-value base fields landed?I think it'd be great to have @damiankloip comment here again, he knows best how this relates to #2846614.
Comment #149
damiankloip CreditAttribution: damiankloip at Acquia commentedI think some of the changes may still be needed here, we will have to reroll and remove the changes to the field naming in TermViewsData once #2846614: Incorrect field name is used in views integration for multi-value base fields has landed. I think the start of these changes were introduced in #110, and a few subsequent patches after that. It's a bit harder to track as not all files have interdiffs.
So based on recent comments in #2846614: Incorrect field name is used in views integration for multi-value base fields - It will get committed in the next few days, so we can rely on that on move forward here. Based on that short time frame, I won't bother marking this postponed again.
Comment #150
Wim Leers#2846614: Incorrect field name is used in views integration for multi-value base fields just landed!
Now Damian can reroll this according to his comment in #149 and we can finally push this one to the finish line — it's been blocked on #2846614: Incorrect field name is used in views integration for multi-value base fields since #88 — January 25, 2017, or almost 9 months ago!
Comment #151
damiankloip CreditAttribution: damiankloip at Acquia commentedThis rerolls to use the views data changes I originally made (#96 and before maybe). Let's see how we get on. The changes I've made to \Drupal\Tests\taxonomy\Functional\Views\TaxonomyRelationshipTest::testTaxonomyRelationships should make them pass now. We would need an update handle to reflect the changes needed in views.view.test_taxonomy_term_relationship.yml for example, but #2846614: Incorrect field name is used in views integration for multi-value base fields should have already taken care of that for us.
TL;DR is that we're replacing 'parent' in the views table data with the correctly named column of 'parent_target_id'.
Let's see how the rest of the tests go.
Comment #153
jibranYay! #2846614: Incorrect field name is used in views integration for multi-value base fields is in.
We need a subsystem maintainer review for this.
Can this be replaced by EFQ?
The schema version needs an update.
Comment #154
claudiu.cristeaI wonder why I was removed from credits here as I spent a lot of effort on this issue?
Comment #155
damiankloip CreditAttribution: damiankloip at Acquia commentedWell, it's not near being committed, so I wouldn't panic just yet. Can't you just check yourself again?
Comment #156
claudiu.cristeaI'm not in panic.
EDIT: Tried to check but it has no effect.
Comment #157
volegerWe should be careful with
removeItem
method. It trigger reindex item deltas.so it should be like :
Comment #158
Wim Leers#151 Yay! The only failures are in
TaxonomyParentUITest
(with failure:"The handler for this item is broken or missing." not found
), which is not being modified by this patch. But that test does useviews.view.test_taxonomy_parent.yml
, which is being modified by this patch. That test was added by #2207229: Attempt to use 'Parent Term' relationship results in broken/missing handler. I wonder if this issue solved the real root cause of the symptom that #2207229: Attempt to use 'Parent Term' relationship results in broken/missing handler fixed?#153: added
@claudiu.cristea: only committers have the power to assign or unassign credit. I think it's merely a bug, perhaps because your oldest comments with patches date to the early days of the issue credit system. I'm sure that when this gets committed, you will get credit :)
Comment #159
damiankloip CreditAttribution: damiankloip at Acquia commentedYeah, #2207229: Attempt to use 'Parent Term' relationship results in broken/missing handler is certainly related but I think it just fixed the 'fix' that we already had in place to fix the field data that we can remove here.
This test just need the same fix as the other test; fixing the field used. This should fix that.
@voleger and @jibran I'm not ignoring you guys, honestly :) I just want this passing again first so we know where we are.
Comment #160
damiankloip CreditAttribution: damiankloip at Acquia commentedComment #161
damiankloip CreditAttribution: damiankloip at Acquia commentedThis updates the removeItemsByTargetId() method to use setValue() instead. That seems like a simpler way to go rather than use a $bias variable to tweak the the delta as we go. This seems a bit easier to read? Really good catch on that though!
Updated the update functions too. Renamed to 850_* also added the field rename to the update function too. Not sure the updates from the views issue will tackle this as we're moving from a totally different table, not just broken views data.
RE #153.2: I am not sure we want to be converting that here? It seems to be intentionally not using the entity system. Might be just legacy, not sure. If that were the case, we wouldn't need an EFQ, it would just be the value from the loaded term entity I think?
Comment #163
damiankloip CreditAttribution: damiankloip at Acquia commentedThat fails because setValue() is not expecting actual items to be passed in. Maybe this instead... (Remove them from list directly, then rekey once after).
Comment #164
volegerI gues using
array_reverse()
can protect from removing item by incorect delta.Comment #165
Wim Leers#153.2: updating a test from using a DB query to an entity query, a test that was written in 2008, that seems out of scope. I'd suggest creating a new issue.
Good to see all other feedback being addressed!
Comment #167
Wim Leersfoo
andbar
, term IDs 1 and 2). After applying the patch and running the update path, I added another tag (baz
, term ID 3). This uncovered a bug: only terms created after this patch is applied get the necessary entries in thetaxonomy_term__parent
table!Then this is what you get for
foo
:json
hal_json
parent
at all!Yet for
baz
:json
hal_json
Worse, this impacts more than just the REST API responses, it also affects the UI: when you want to specify a parent term, the only terms you get to choose from are the ones with entries in the
taxonomy_term__parent
table. Therefore,foo
andbar
are not available as choices:So as far as I can tell, the update path is incomplete, and so are the update path tests. Sorry 😔
This should not be necessary, because
\Drupal\Tests\rest\Functional\EntityResource\Term\TermResourceTestBase::getExpectedNormalizedEntity()
already contains this.Which means there's a bug in
\Drupal\Tests\hal\Functional\EntityResource\HalEntityNormalizationTrait::applyHalFieldNormalization()
, because it's removing this field, even though it should not.I stepped through it with a debugger, and the root cause lies here in
\Drupal\hal\Normalizer\EntityReferenceItemNormalizer::normalize()
:$target_entity === NULL
, which means the body of the if-statement is executed, which is why theparent
field is handled as if it were not an entity reference field: because it's referencing a non-existing entity!This means that in theory, a term with a parent that is not
<root>
(i.e. non-existing term zero), should actually work. So building upon the example for review point 1, I added another term,qux
(term ID 4), withbaz
(term ID 3) as its parent. And yes, sure enough, this is now the output:json
hal_json
which is what I had expected all along.
We'll probably want to add a
testGetTermWithParent()
to\Drupal\Tests\rest\Functional\EntityResource\Term\TermResourceTestBase
to ensure proper test coverage/prevent this from regressing.Nit: this should be
\Drupal\taxonomy\TermInterface[]
?Should we create a
TermInterface::ROOT_ID = 0
constant?Nit: s/id/ID/
Nit: 8.5.x.
Nit: 8501, 8502, 8503 by now… We've been at this for a while now.
Only point 1 and 2 are real concerns, all others are nits. 🎉 Point 1 is pretty deep, but hopefully is a simple, small oversight. Point 2 likely requires changing
\Drupal\hal\Normalizer\EntityReferenceItemNormalizer::normalize()
. I'll be looking into that more, and hopefully we can spin that off into its own issue.Comment #168
voleger#2915782: Modernize ForumTest: use entity queries instead of DB queries
Added patch for ForumTest
Comment #169
Wim LeersWhew 😓, many hours later, I have a solution for point 2 of #168!
I think point 1 is best addressed by @damiankloip.
Comment #171
Wim LeersD'oh, some last-minute refactoring and not re-running all tests let a silly tiny thing slip through…
Comment #172
jibranWhy do we need the first part of the statement? I think all we need is $target_entity_type_id is fieldable.
Comment #174
jibranFixed #167 3,4,5,6,7 and then some.
Comment #175
Wim Leers@jibran++ 👏
That leaves only #167.1 (which hopefully damiankloip can address) and #172.
#172: but an entity type ID is just a string, you need an actual class or object to be able to check that it's implementing a certain interface. That's why I added this
@todo
in #171:.
Comment #176
Wim LeersJust talked to @damiankloip — he's going to work on fixing the gap in the update path described in #167.1. But I'll first tackle the update path test coverage. IOW: I'll make the patch fail, he'll make the patch pass :)
Comment #177
yched CreditAttribution: yched commentedHi there :-)
Just a note about the removeItemsByTargetId() / "removeItem() reindexes deltas" thing mentioned in #157 and following :
digging back in the code around #2392845: Add a trait to standardize handling of computed item lists reminded me that we added a \Drupal\Core\TypedData\Plugin\DataType\ItemList::filter($callback) helper for exactly that kind of cases :-)
Comment #178
Wim LeersThanks, @yched! ❤️👌
Comment #179
Wim LeersFirst, a reroll. This conflicted with #2909183: Add path alias (PathItem) field PATCH test coverage. Rebased, solved a trivial conflict.
Stay tuned for that failing update path test coverage — but I won't be able to finish it today.
Comment #180
Wim LeersGuess what!? I can no longer reproduce the problems I reported in #167.1! I carefully started with a fresh D8, created terms, then applied the patch, ran the update path and … everything worked just fine!
So, yay, there's nothing to be done in terms of update path, it works perfectly, and it actually is already testing all of this!
The environment I previously did the testing in had lots of cruft, that's probably the reason! So we can start the final rounds of review? :)
In looking at the update path test coverage, I found these nits though:
Nit: should be renamed to
TermParentUpdatetest
, "taxonomy" is too broad.This is actually already trying to set up a site to test things like #167.1. But it's doing it in a way that's unlike every other
UpdatePathTestBase
test.The best practice is to either
setDatabaseDumpFiles()
setDatabaseDumpFiles()
, which is then executed there (seecore/modules/rest/tests/fixtures/update/drupal-8.rest-rest_post_update_resource_granularity.php
for an example)Comment #181
claudiu.cristeaRelated to taxonomy API clean-up: #2917209: Remove node code from taxonomy. Make it pluggable.
Comment #182
dawehnerIs there a reason this is not an array_filter?
Is there a reason we call this method content entity even we deal with fieldable here?
It feels like this belongs more on the term storage as it returns terms
From a BC point of view, couldn't it be the case that someone still accesses the data directly? Maybe in order to not cause exceptions it might be worth dropping the table in D9 or so?
Comment #183
yched CreditAttribution: yched commentedre #182.1
see #177 - removeItemsByTargetId() should just be :
ItemList::filter() takes care of the gotchas around the fact that an unset($items[$delta]) rekeys the subsequent items.
Comment #185
Wim LeersThis blocks JSON API and GraphQL. And it of course blocks you from building any app that needs access to the parent term. Adding issue tags.
Comment #186
damiankloip CreditAttribution: damiankloip at Acquia commented#177: Nice! Thanks @yched. That's exactly what I needed. Can't believe I haven't seen that method on there before!
#180.1: Agreed - renamed.
#180.2: Yes, it's a bit odd (not really less odd than the pattern we use elsewhere but that's another story). Changed to bring in line with the other update tests. The only thing is the tids have now been hard coded in the test. I think this is why it was a method on the test before - to collect the tids that were inserted diretly into the database. Similar with the view ID - there isn't really a need for it to
#182.1: That is fixed by ^^
#182.2: Hmm. This is a good question. The comments are all pointing to content entities but the previous code was also just checking for fieldable.. So we either change the comments or the interface that's being used? I know the plan was always that hal just embedded content entities.
#182.3: I think we can/should just remove that. VocabularyStorageInterface already has this method, it's just been copied to another place. I removed the other usages and just used the
#182.4: Hmm. It's a good question. We would just have a table of stale data though? So if queries were still using it, it wouldn't do much anyway. I'm fine to keep it in, do we support people just querying stuff outside of the API?
TL;DR is we need to decide what to do in the hal \Drupal\hal\Normalizer\EntityReferenceItemNormalizer::normalize and whether to keep the legacy table or not.
Comment #188
damiankloip CreditAttribution: damiankloip at Acquia commentedLet's try that again with the correct variable.
Comment #190
damiankloip CreditAttribution: damiankloip at Acquia commentedSorry, back on a working install. Actually testing the test now!
Comment #191
Wim LeersActually, this is 8.5-only, because #2846614: Incorrect field name is used in views integration for multi-value base fields was committed to 8.5 only.
Therefore #190 is actually green!
Comment #192
damiankloip CreditAttribution: damiankloip at Acquia commentedI think based on https://www.drupal.org/core/d8-bc-policy we should remove the schema. We would have stale data in it pretty quickly, unless we kept them in sync. Seems like people should be using the API here instead. @dawehner do you have any other cases where that is not possible. Seems like a semi generic devils advocate example :P
Hmm, looking into stuff related to @dawehner's other point in #182.2. It seems like #2275659: Separate FieldableEntityInterface out of ContentEntityInterface changed this behaviour (in error IMO), but didn't update the comment anyway :/ so I guess we have to go with the fieldable behaviour now either way.
Comment #193
Wim LeersAgreed with #192 regarding https://www.drupal.org/core/d8-bc-policy#schema and the
taxonomy_term_hierarchy
table quickly becoming stale, therefore returning incorrect results anyway.#182.2: calling the method "fieldable entity" or "content entity": we've been equating "content entity" with "implements fieldable entity interface" for a long time. See
\Drupal\Core\Entity\KeyValueStore\KeyValueEntityStorage::doCreate()
andeditor_entity_insert()
for example. Also, it was a pre-existing problem in this very code even, it was just moved into a new helper method!Anyway, #192 made the change you asked for. I don't feel strongly about it either way. It's just a protected helper method.
Quoting #186:
These are now done.
Time for final review!
Nit: s/id/ID/
Nit: s/content/fieldable/
we'll need this issue created now.
Nit: These can now be updated to
TermInterface::ROOT_ID
.That doesn't sound right. I think this was meant to read:
Nit: Only change in this file, can be reverted.
Comment #194
damiankloip CreditAttribution: damiankloip at Acquia commentedCool - thanks Wim! I'll address this feedback shortly.
Comment #195
damiankloip CreditAttribution: damiankloip at Acquia commentedok, I addressed all that feedback. Some good catches in there! One thing; instead of creating a new issue (that could potentially block this as its missing functionality?) I went ahead and implemented the class checking for different entity types in
targetEntityIsFieldable
and removed the todo.Comment #196
Wim LeersEven better!
I think this is finally ready :) 🎉
Comment #197
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedI'd like to take look at this patch.. I'll try this weekend so I don't keep it blocked much longer.
Comment #198
dawehnerIt would be nice to have its own test for this, \Drupal\KernelTests\Core\Entity\EntityReferenceFieldTest would be the place for that.
Comment #199
damiankloip CreditAttribution: damiankloip at Acquia commentedYeah, that's a good idea. I'll add that.
Comment #200
damiankloip CreditAttribution: damiankloip at Acquia commentedHere is a test for that. I am just about to go out, so I haven't tested this yet! I'll fix it later if it's failing.
Comment #201
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedOk, here's my review. I'm a bit sad to say that the patch still needs quite some work if we want to keep it aligned with the goals of the Workflow Initiative.
However, the basic functionality needed for the API-First Initiative could be implemented *a lot* easier by making the 'parent' field a computed one, use the trait from #2392845: Add a trait to standardize handling of computed item lists and simply implement the
computeValue()
of that trait to ensure that$term->parent
returns the correct value instead ofNULL
. I think this approach could be tried in a new issue so we don't ruin the scope of this one.$target_id can also be a string :)
$ref
doesn't really describe this argument, it's a field item so let's call it like it is:$item
:)We can use
is_a()
here:return is_a($target_entity_type_class, FieldableEntityInterface::class, TRUE);
One of the goals of the Workflow Initiative is to add a revision_parent field that will be used to generate revision trees, and (at my request) it will be implemented as a generic hierarchy framework on top of entity reference fields, which is basically what we had in D7 with the Tree module.
That generic hierarchy framework will apply to every tree-like structure in Drupal (taxonomy terms, menu links, books), and the basic API that I have in mind for it is to have the methods that interact with the tree at the field item list level, not at the entity level.
For example, getting the ancestors would be a
getAncestors()
call on the 'parent' field:$term->get('parent')->getAncestors()
.Anyway, where I'm getting with this long review point is that I would prefer to not introduce this
TermHierarchyInterface
here because it kind of goes against the plan outlined above.In the Tree module, roots are stored with a NULL value for target_id, and I would like to keep that concept here for the same reason mentioned above.
If we remove the interface, this entity type update should not be needed anymore. Although I wonder why is it needed in the first place.
This whole update function looks like it could use a simple INSERT FROM SELECT query, which is very fast and doesn't need any batching.
This is a strange message to return from an update function. It will be displayed in the final screen after running the updates and it looks like there was some error during the conversion process, especially because there's no definite 'success' message returned by the next two update functions.
Is there any reason why the table can not be dropped in
taxonomy_update_8502()
?Does the previous syntax not work anymore? I'm reasonably sure that it should still work, otherwise this is a BC break..
Are these changes fixing a bug, or why do we need to change the assertions?
Comment #202
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedI hacked together a patch with my suggestion from #201 in #2921048: Introduce temporary work-around to make $term->parent work in REST API + JSON API in Drupal 8.5 — 8.6 ships with root cause fix.
Comment #203
damiankloip CreditAttribution: damiankloip at Acquia commentedThanks for looking amateescu! Hopefully we can align this with what everyone needs...
Isn't #2921048: Introduce temporary work-around to make $term->parent work in REST API + JSON API in Drupal 8.5 — 8.6 ships with root cause fix just directly conflicting with this issue? I get it's simplifying things slightly, it's not really fixing the underlying issue though. It's just pushing it further down the road. We could have fixed this at the field level or normalizer level in the mean time but they would all just be temporary solutions, pretty much.
I have implemented a few of the smaller points from #201 1,2,3,6. Will have to look at the update based feedback. I didn't write those originally. Seems like there might be a small amount of logic for the insert data.
201.4: Hmm, then we would have to wait for this idea you have that doesn't actually exist yet?! :) Not sure we want to do that. It seems ok to add this additionally but then internally depend on your generic hierarchy work internally when it exists (this would essentially be a wrapper around it then)? In D9 what we have here can be removed in favour of your field level API stuff only.
201.5: using NULL as a way to signify the root term seems like an odd choice, is there a reason for that? Generally NULL as a first class citizen is kind of weird. Plus 0 denotes the top level root now anyway.
201.11: Please see https://www.drupal.org/node/2543726#comment-11918427 (Comment #99). That summarises stuff me and Wim talked about quite well IIRC.
Needs review for now to test the patch.
Comment #205
damiankloip CreditAttribution: damiankloip at Acquia commentedComment #206
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedNot at all :) I'll answer in detail below, I just wanted to be clear that there's no conflict between this issue and the other one.
I guess that depends on what you consider the "underlying issue" is.
In HEAD, the 'parent' field is a field that uses a custom storage and there's nothing conceptually wrong with that. The actual bug is that, just like computed fields, custom storage fields need to behave like regular fields and return the correct value(s) when requested via API calls (i.e.
$term->parent->target_id
).#2921048: Introduce temporary work-around to make $term->parent work in REST API + JSON API in Drupal 8.5 — 8.6 ships with root cause fix is about fixing the real bug in 8.4.x (the fix being ~5 lines of code) at the field level, while this issue is actually about making the 'parent' field not use a custom storage anymore, which is a task that can be fixed at any point in the future, and the final patch here will just need to remove the custom field item list class added in #2921048: Introduce temporary work-around to make $term->parent work in REST API + JSON API in Drupal 8.5 — 8.6 ships with root cause fix because it won't be needed anymore.
Comment #207
jibranSo can someone explain now what is the usecase of this conversation now and please update the issue summary as well?
Comment #208
Wim Leers#206: Thanks for that explanation, that helps a lot! ❤️ Also, thank you very much for doing a review, and such an in-depth one at that! 👌
Review posted at #2921048-12: Introduce temporary work-around to make $term->parent work in REST API + JSON API in Drupal 8.5 — 8.6 ships with root cause fix — I found one significant problem, which you probably should be able to address. If you can, then I think you're right that #2921048 could land first, and then this issue could land later, to improve the way term parents are stored. Thanks to the test coverage that was developed in this issue, we can know for a fact that there won't be a BC break: if the REST test coverage in both this issue's patch and #2921048's are identical, then we can be absolutely certain! But right now, the test coverage doesn't match, and that's precisely the significant problem I found :) But again, I'm pretty confident you can address that!
I have a call with @damiankloip in ~1.5 hours, I will discuss it with them, to make sure I didn't overlook something.
EDIT: https://twitter.com/wimleers/status/927532217370333184 :)
Comment #209
damiankloip CreditAttribution: damiankloip at Acquia commentedIn my mind, I would prefer to go down this patch route, as we have a whole fix here. This also formalises the way the parent field works with views, as it's just another ER field. I am totally open to the computed field work in your patch too though - it looks nice and simple. I just kept away from adding a computed before in quest of the 'proper' fix. If that's the best way to keep everyone happy, I am ok with that too.
If the main reason is a clash of interfaces, I think we could solve that. I mentioned briefly above; if the TermHierarchyInterface was a wrapper around any new work, when it exists. We could replace the internal code with the new hierarchy system. Then remove this in D9, leaving just the generic solution.
And yes, the summary needs an update :)
Comment #210
Wim LeersI had a call with Damian. I think there are 3 things to discuss/clear up:
You're absolutely right — we've been saying for ages that this is about API-First, but it's about much more: the scope is much broader than just API-First. The scope in the current patch includes fixing Views integration (rather than lots of custom stuff, use the same well-tested mechanisms/code paths as entity reference fields), which is also why this was blocked on #2846614: Incorrect field name is used in views integration for multi-value base fields for a very long time. This badly needs an issue summary update, as @jibran indicated in #207.
So, to answer your question: this issue's scope is about making
$term->parent
behave like any other entity reference field, with two goals:Updated issue title + summary.
For the Views half, we need the storage changes that this patch is making, where it's moving us away from custom storage. @amateescu, do you agree that this de-customizing of storage is something that is A) desired, B) would be implemented the same in the future you describe in #201.4?
TreeInterface
, the introduction ofTermHierarchyInterface
is A) an unnecessary intermediate step, B) an annoyance. But … we have plenty of BC layers. Any reason why we can't just deprecateTermHierarchyInterface
and keep a BC layer around? That BC layer would only exist forTerm
entities. In fact, even in the current patch, it's effectively@deprecated
: there's a@todo
to merge it withTermInterface
in D9. That would then just change/be removed once yourTree
plan works out.Damian has been working on this since January. We're finally super close to the finish line. I very very much like the https://www.drupal.org/project/tree idea (I remember discussing it with you in Leuven in 2015 or so), but I don't think this moves you further away from that necessarily. I think it only means one more BC layer.
Furthermore, I think there's an opportunity to have even fewer BC worries. The current patch does this:
We could choose to not do that, and keep those APIs as the official ones, and instead mark
TermHierarchyInterface
and its implementations as@internal
. That way, the public API would remain unchanged compared to HEAD, but we'd at least internally be using this improved way of doing things. Then when the Tree vision materializes, there would truly be zero extra BC layers.Thoughts?
Comment #211
jibranThanks @Wim Leers and @damiankloip for updating the IS.
I think we should also do some performance testing for the changes in
TermStorage
class.Comment #212
amateescu CreditAttribution: amateescu for Pfizer, Inc. commented@Wim Leers, re #210:
A) Absolutely! and B) Yes :)
This patch in its current form (well, except for the new interface) is pretty much critical for the "great tree structure unification" described in #201.
Like I said in #206, the reason for getting #2921048: Introduce temporary work-around to make $term->parent work in REST API + JSON API in Drupal 8.5 — 8.6 ships with root cause fix in first is that 1) it fixes the API-First bug now (in Drupal 8.4.x), not in 4-5 months (Drupal 8.5.x), and 2) the only additional "work" for this patch would be to remove the custom field item list class added by that issue.
@damiankloip, re #209 and @Wim Leers, re #210.3:
Actually, it's a bit more than an unnecessary intermediate step and/or a simple annoyance. I tried to explain the problem in #201 but maybe I couldn't get the point across very well so I'll try again.
The
TermHierarchyInterface
added by this patch forces the Term entity to be part of a single hierarchy, meaning that the methods for getting the parents, ancestors, etc. are at the entity level. The Workflow Initiative wants to introduce the concept that an entity will be part of multiple hierarchies, the second one being a revision hierarchy, for example.Here's a code explanation which might make things more clear.
API provided by the current patch:
Desired API:
Where
$term->getTree()->...
is simply a shortcut for$term->get('parent')->...
and$term->getRevisionTree()->...
is a shortcut for$term->get('revision_parent')->...
.This is why the distinction between the hierarchy being at the entity level vs. field level is very important to the future of the Workflow Initiative.
Comment #213
Wim LeersThanks for expanding your explanation! I only have one remaining question:
This is your concern. But:
Why couldn't
$term->getParents()
(from theTermHierarchyInterface
) then not also be another shortcut for$term->get('parent')
, and therefore a deprecated synonym for$term->getTree()->getParents()
?Comment #214
amateescu CreditAttribution: amateescu for Pfizer, Inc. commented@Wim Leers, you highlighted the answer to that question in the first qoute (my concern).. :)
Comment #215
Wim Leers#214: I don't think that highlighted part answers it.
getParents()
would just become a synonym for the "default tree" (getTree()->getParents()
). I don't see what the problem is with that.Comment #216
Wim LeersPer #2921048-13: Introduce temporary work-around to make $term->parent work in REST API + JSON API in Drupal 8.5 — 8.6 ships with root cause fix, where @amateescu said:
Adding explicit test coverage for that, to prove that that is supported in this patch. Good call, @amateescu! 👍
Comment #217
Wim LeersStill looking for your feedback, @amateescu.
Until #200, we thought this was ready. The review you posted in #201 was invaluable! 👍❤️
@damiankloip then addressed everything, except for point 4.
I tried to explain the different POVs and bring them together in #210. But now with #215 and #216, this is AFAICT blocked on you clarifying your feedback/concerns/proposals. Let me know if there's anything I can do to help (perhaps I can review some other issues to help you find the time for this one)?
Comment #218
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedThe problem, as I see it, is that we would introduce some methods which will be 1) incomplete, compared to list of methods from the TreeInterface and 2) deprecated basically from the start. Also, some review points from #201 were not addressed, the ones about the upgrade path IIRC.
So my proposal is still the same: get #2921048: Introduce temporary work-around to make $term->parent work in REST API + JSON API in Drupal 8.5 — 8.6 ships with root cause fix in to fix the REST problem and then work on this patch as a followup when we settle everything with the new generic interface for hierarchies.
Comment #219
dawehnerSettings needs work for the review in #201
@amateescu
While I totally agree that a potential tree would be super nice, I don't think we should block this issue from going on forward:
Comment #220
Wim LeersSo #201.6, #201.7, #201.8 and #201.9. IOW: the
taxonomy.install
changes.@damiankloip did address #201.6. But 7+8+9 indeed still need to be addressed.
Comment #221
Wim LeersTrying this.
Comment #222
Wim LeersFirst, straight rebase, plus keep tests green, because #2800873: Add XML GET REST test coverage, work around XML encoder quirks automatically changes the normalization of every entity type, which means we need to update our expectations. (That is being fixed in #2928702: Make EntityResourceTestBase's field_rest_test_multivalue test field less invasive: omit it from normalizations. Once that lands, we'll be able to revert this interdiff.)
Comment #223
Wim LeersI followed a simple strategy:
Term::(getParents|getAncestors|getChildren)
toTermStorage
TermInterface $term
parameter$this
with$term
@internal
@todo
which will be resolved by @amateescu's futureTreeInterface
issueTermHierarchyInterface
This means the APIs remain 100% identical to today, and the logic remains 100% identical to @damiankloip's patch.
Comment #224
dawehnerJust some super quick scan.
Couldn't we replace that with loadAllParents ?
Vocabularies are config entities, the ID is well defined upfront :)
This looks like it shouldn't work
Comment #225
Wim LeersComment #226
dawehnerWell, by not changing it you'll cause a fatal
Comment #228
dawehnerLet's see what this fix results into :)
Comment #230
Wim Leers#228 was indeed an oversight, here's another.
Comment #232
dawehnerThis should fix the remaining failure.
Comment #234
lawxen CreditAttribution: lawxen at Sparkpad commentedThese line caused the patch can't be applied by composer-patchesSo I apply the patch, then use "git diff" to recreate the patch, No functional change.I'm sorry, this is our own composer-patch bug, not this issue ignore this patch
Comment #236
Wim LeersComment #237
Wim LeersThe last green patch is #216, from November 10.
#2800873: Add XML GET REST test coverage, work around XML encoder quirks landed on November 24, after #216, and added XML test coverage. This explains the remaining new failures: they'd also have occurred for #216.
This small change to
XmlEntityNormalizationQuirksTrait
makes theTermResourceTestBase::testGetTermWithParent()
tests pass when using the XML format.Comment #238
Wim LeersI created a mess in #223. I did follow a simple strategy, but I made many small mistakes, which resulted in these ~15 comments that gradually got the patch closer to green. Sorry about that :(
Here's an interdiff showing all the differences between #216 and #237. That should make it much easier for @damiankloip to review this when he gets back next week.
The interdiff shows two kinds of changes:
Comment #240
Wim Leers#201 points 7+8+9 still need to be addressed. Assigning to @damiankloip for that.
Comment #241
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedThe interdiff from #238 looks very good! I have to re-read the whole patch again in depth but here's a minor point for now:
parents
,parentsAll
andchildren
are gone, but we have a newancestors
member that we need to unset.And initialize here :)
Comment #242
Wim Leers#241: excellent nits!
Comment #243
lawxen CreditAttribution: lawxen at Sparkpad commented@Wim Leers
ArgumentCountError: Too few arguments to function Drupal\hal\Normalizer\EntityReferenceItemNormalizer::__construct(), 2 passed in /var/www/d85/core/lib/Drupal/Component/DependencyInjection/Container.php on line 266 and exactly 3 expected in Drupal\hal\Normalizer\EntityReferenceItemNormalizer->__construct() (line 55 of /var/www/d85/core/modules/hal/src/Normalizer/EntityReferenceItemNormalizer.php) #0 /var/www/d85/core/lib/Drupal/Component/DependencyInjection/Container.php(266): Drupal\hal\Normalizer\EntityReferenceItemNormalizer->__construct(Object(Drupal\hal\LinkManager\LinkManager), Object(Drupal\serialization\EntityResolver\ChainEntityResolver)) #1 /var/www/d85/core/lib/Drupal/Component/DependencyInjection/Container.php(171): Drupal\Component\DependencyInjection\Container->createService(Array, 'serializer.norm...') #2 /var/www/d85/core/lib/Drupal/Component/DependencyInjection/Container.php(480): Drupal\Component\DependencyInjection\Container->get('serializer.norm...', 1) #3 /var/www/d85/core/lib/Drupal/Component/DependencyInjection/Container.php(508): Drupal\Component\DependencyInjection\Container->resolveServicesAndParameters(Array) #4 /var/www/d85/core/lib/Drupal/Component/DependencyInjection/Container.php(230): Drupal\Component\DependencyInjection\Container->resolveServicesAndParameters(Array) #5 /var/www/d85/core/lib/Drupal/Component/DependencyInjection/Container.php(171): Drupal\Component\DependencyInjection\Container->createService(Array, 'serializer') #6 /var/www/d85/core/lib/Drupal/Component/DependencyInjection/Container.php(480): Drupal\Component\DependencyInjection\Container->get('serializer', 1) #7 /var/www/d85/core/lib/Drupal/Component/DependencyInjection/Container.php(230): Drupal\Component\DependencyInjection\Container->resolveServicesAndParameters(Array) #8 /var/www/d85/core/lib/Drupal/Component/DependencyInjection/Container.php(171): Drupal\Component\DependencyInjection\Container->createService(Array, 'rest.resource_r...') #9 /var/www/d85/core/lib/Drupal/Component/EventDispatcher/ContainerAwareEventDispatcher.php(105): Drupal\Component\DependencyInjection\Container->get('rest.resource_r...') #10 /var/www/d85/vendor/symfony/http-kernel/HttpKernel.php(193): Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher->dispatch('kernel.response', Object(Symfony\Component\HttpKernel\Event\FilterResponseEvent)) #11 /var/www/d85/vendor/symfony/http-kernel/HttpKernel.php(260): Symfony\Component\HttpKernel\HttpKernel->filterResponse(Object(Symfony\Component\HttpFoundation\Response), Object(Symfony\Component\HttpFoundation\Request), 1) #12 /var/www/d85/vendor/symfony/http-kernel/HttpKernel.php(79): Symfony\Component\HttpKernel\HttpKernel->handleException(Object(Symfony\Component\HttpKernel\Exception\NotFoundHttpException), Object(Symfony\Component\HttpFoundation\Request), 1) #13 /var/www/d85/core/lib/Drupal/Core/StackMiddleware/Session.php(57): Symfony\Component\HttpKernel\HttpKernel->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true) #14 /var/www/d85/core/lib/Drupal/Core/StackMiddleware/KernelPreHandle.php(47): Drupal\Core\StackMiddleware\Session->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true) #15 /var/www/d85/core/modules/page_cache/src/StackMiddleware/PageCache.php(184): Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true) #16 /var/www/d85/core/modules/page_cache/src/StackMiddleware/PageCache.php(121): Drupal\page_cache\StackMiddleware\PageCache->fetch(Object(Symfony\Component\HttpFoundation\Request), 1, true) #17 /var/www/d85/core/modules/page_cache/src/StackMiddleware/PageCache.php(75): Drupal\page_cache\StackMiddleware\PageCache->lookup(Object(Symfony\Component\HttpFoundation\Request), 1, true) #18 /var/www/d85/core/lib/Drupal/Core/StackMiddleware/ReverseProxyMiddleware.php(47): Drupal\page_cache\StackMiddleware\PageCache->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true) #19 /var/www/d85/core/lib/Drupal/Core/StackMiddleware/NegotiationMiddleware.php(50): Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true) #20 /var/www/d85/vendor/stack/builder/src/Stack/StackedHttpKernel.php(23): Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true) #21 /var/www/d85/core/lib/Drupal/Core/DrupalKernel.php(657): Stack\StackedHttpKernel->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true) #22 /var/www/d85/index.php(19): Drupal\Core\DrupalKernel->handle(Object(Symfony\Component\HttpFoundation\Request)) #23 {main}.
Comment #244
Wim Leers#243:
EntityReferenceItemNormalizer
, in an incorrect way. Note that https://www.drupal.org/core/d8-bc-policy#constructors explicitly says that constructors A) aren't APIs, B) can change in minors.I investigated the
commerce
module, and they have zero normalizers. So this seems … impossible.It looks like you didn't rebuild the container after applying the patch?
Comment #245
Wim LeersWe can remove this thanks to #2928702: Make EntityResourceTestBase's field_rest_test_multivalue test field less invasive: omit it from normalizations having been committed. (This reverts the addition that #222 made to keep tests passing.)
Comment #246
lawxen CreditAttribution: lawxen at Sparkpad commented@Wim Leers
Then #243 error occurs.
The "drush cr" has rebuild the container.
Uninstall one of hal and rest, The error will gone away.
Comment #247
Wim Leers@caseylau:
I followed the steps in #246 (albeit without using
drush
). I reproduced it.The root cause is not Commerce, nor this patch. It's the
entity_reference_revisions
module's\Drupal\entity_reference_revisions\EntityReferenceRevisionsServiceProvider
. That will need to be updated.(And if that code used the service decorator pattern, it wouldn't even need to be updated. Look at the
html_response.attachments_processor.big_pipe
service for an example of that.)Created #2931496: Add serializer for json/xml for this.
@caseylau's feedback is great, but is definitely not a blocker: he's just being very proactive (impressively so actually!), by testing a patch for the next minor release with his entire set of modules. In doing so, we found a problem in the
entity_reference_revisions
module that can easily be fixed, even before 8.5 is released.Comment #248
Berdir> And if that code used the service decorator pattern
We are not decorating an existig service, we're adding our own that extends one from core. So no, decorator would not help here, but I guess it would be possible to re-use the arguments that the core service for that class is using instead of adding them hardcoded, then this wouldn't happen.
Technically, __construct() is excluded from BC so it we are allowed to make that change. However, whenever we can, we usually add such arguments with = NULL and a fallback in the constructor, so that we do not break child classes.
Comment #249
Wim LeersTrue in some cases, but not here. In this case, it's subclassing only to callparent::METHOD()
in two places. It could easily be a pure decorator.Darn, it's decorating
protected function constructValue()
, which of course is not possible to decorate because it's not public.Of course, normalizers never were APIs at all (we're trying to fix that at #2920536: Force all serializer encoders + normalizers services to be private), but evidently some contrib modules started to use them as if they were APIs anyway.
So, changing it to an optional argument. Once again, not being clear about what our APIs are comes back to haunt us :(
Comment #250
Berdir> Darn, it's decorating protected function constructValue(), which of course is not possible to decorate because it's not public.
Yes, but that's not even relevant. A decorator is about decorating an existing service, we define a new one, because this is for a different field type. Because you also overlooked the most important change in the subclass. The fact that we change "protected $supportedInterfaceOrClass".
As I wrote, even if it would be an API, the constructor is technically excluded. But it can already be quite painful to do minor updates, so lets try to be as nice as possible to contrib and custom modules :)
And \Drupal\hal\Normalizer\EntityReferenceItemNormalizer is actually a very important class, because there are many sub-field types of that, so it is only logical that those also need to support normalization and don't want to duplicate 120 lines of code. I'm seeing 4 subclasses in my development installation: file_entity, webform, entity_reference_revisions, dynamic_entity_reference.
I also add this paragraph as a comment into the issue you referenced.Re-reading that issue, that is only about the services. That's actually a completely different aspect of API/Code-reuseability than what we are talking about here.Comment #251
Wim LeersYou're right, I got the terminology wrong :) Decorating the existing service would work, but would effectively remove the existing service, i.e. no more normalizer for entity reference fields.
What I should have said, is inject the existing service, so that you can reuse it. But that would have failed due to the
protected function constructValue()
, as I said in #249.Anyway! #249 addressed your feedback :)
Comment #252
BerdirI would recommend to just add the fallback directly in the constructor, that's easier to remove than having to use an already deprecated method, see for example: \Drupal\Core\Entity\ContentEntityForm::__construct(). Then we can just remove the = NULL and the ?: ... in the 9.x branch, without having to update any calls.
Yeah, just making sure we understand each other :) Wouldn't "what I should have said, is inject the existing service, so that you can reuse it." then actually make that service an API again, which is kind of exactly what you want to prevent in the other issue? Yes it would still work to inject that into the service, but it still seems to be the opposite of what you suggested before, conceptually. :)
IMHO, especially that class is specifically *designed* to be subclassed and extended, with that protected constructValue() method that all 4 of the subclasses that I mentioned are overriding.
Comment #253
Wim LeersYou're right once again. I did #249 the way I did it to prevent unnecessary construction of a service used only in an edge case, but once it's actually injected, it doesn't make a difference anyway. So +1.
No, because:
In either case, it's the decision of the contrib module that they choose to bear the burden of staying in sync with upstream changes of something that isn't an API.
The problem is that we never clearly communicated what are our APIs — which I talked about in https://wimleers.com/talk/bc-burden-benefit — which is why we are forced to deal with this BC stuff in this issue at all. In practice, everything that isn't explicitly marked as
@internal
could be seen as an API, and so effectively for ~90% of code we're forced to maintain BC despite it not being an API…It's FieldItemNormalizer that's designed to be subclassed, not
EntityReferenceItemNormalizer
. Also:::constructValue()
was added in 2013, in #1880424: Handle entity references on import. Only much, much later did we start seeing the consequences (once again, see my talk). Back then, we were still creating interfaces for everything, turning everything into an API. It's also long before we decided on semver and keeping BC all the way through D9. It's before we had@internal
even, I think.Comment #254
Wim LeersThis discussion is making this issue very hard to follow I think :P
I agree with you we shouldn't break BC. I addressed that in #249 (and refined it in the attached patch per your suggestion).
Fact is that normalizers should not be APIs; I think core's normalizers never were meant to be APIs, but maybe they were, but reality is that they are seen and used as such, and therefore we must maintain BC. This is part of a much bigger problem in Drupal: we don't explicitly mark APIs as APIs and everything else as internal implementation detail, and hence developers see almost everything as APIs. In no small part because that was pretty much true in the past. Fixing that is definitely out of scope for this issue. #2912642: Add @internal to automated tests classes is an example of a good step forward to fix that, and #2873395: [meta] Add @internal to core classes, methods, properties is the plan issue for that. Maybe we should move this discussion there? Or at least link to it from there?
Comment #255
damiankloip CreditAttribution: damiankloip at Acquia commentedNail on head! Yes. I have always said this, especially about normalizers. I have had this come up a few times, people have been using normalizers or encoders stand alone, I tell them the serializer instance is the API. Alas, we didn't have internal officially back then etc etc etc... :)
That diff in #254 looks ok to me - in the name of BC.
Comment #256
Wim LeersGreat!
@damiankloip: it's now up to you to do the remaining things listed in #240:
Comment #257
damiankloip CreditAttribution: damiankloip at Acquia commentedIt's great to have some consensus and progress on this now.
RE #201: 8 & 9 I have done in this patch. I think I mentioned briefly before in an earlier comment, but I'm not sure this is entirely possible, as we need to create delta values for each entity field values:
So I don't think we can nicely achieve the same thing with a single SQL query?
Comment #258
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedThanks, @damiankloip, that looks a bit better :) As promised in #241, I've reviewed the entire patch again in depth, and here's what I found:
The target ID ... :)
This won't play well with the Dynamic Entity Reference module from contrib, because it also has to know the target type for the IDs that should be removed.
Currently,
EntityReferenceFieldItemListInterface
contains only one 'additional' method,referencedEntities()
which can be easily implemented by DER, but the new method that we're adding here will be impossible for them to implement without a second argument. Since we're talking about only 3 lines of code, would it make more sense to simply inline that code where we currently need it and open a separate issue for adding this new method so it can be discussed and reviewed properly?This is the last architectural concern that I have with this patch.
The fact that the root ID is
0
is simply an implementation detail of the current hierarchical storage for taxonomy term parents, and IMO it is not something that we want to expose to the outside world (i.e. via REST).Consider a scenario where someone adds a hierarchical field to user entities, for example to represent a family tree. In that hypothetical scenario, we won't be able to say that
root = 0
because0
is an actual entity in the system. So how do we explain that REST calls need to know about the special ID 0 for taxonomy terms, and a different special ID for users.At the very least, we should move this constant to the storage class, and my suggestion is to expose a proper constant (e.g.
<root>
) as the official API for REST calls that interact with taxonomy parents. This way, if we manage to re-architect and unify all the separate hierarchical implementations in core, we won't need to break the API or deal with ugly BC layers for REST. Let's be API-forward-thinking for a chance, not only API-first-focused-only-on-individual-issues :)Since we're not adding the new interface anymore, I guess we can try to remove this again?
We should also add a
@group Update
here.This update function does not exist anymore.
We can use
assertCount()
here.... to use *the* {taxonomy_term__parent} table ...
This assertion is not really useful since we're checking the entire array structure below.
Also, #241 still needs to be addressed.
Comment #259
jibran#258.2 😍
Comment #260
damiankloip CreditAttribution: damiankloip at Acquia commentedThanks @amateescu - great review!
RE #2: That's a good point, I have just inlined it. As we don't have the additional method now, I've removed the corresponding test method. We already have test coverage testing children get deleted (I think TermKernelTest has most of that).
RE #3: I would be keen to see what Wim's view is on this one. It's a really good point about a 0 ID having a different meaning between entity types. From my point of view a string constant would be strange, I would prefer to just use
NULL
in these cases personally. Having something like '<root>
' would still need special handling for consumers I think?The rest of the points I think I've addressed in this patch.
Comment #264
Wim LeersLooks like this should not have been removed.
Comment #265
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedThat's really weird, we should try to investigate why we need to update the entity type.
@Wim, this is still waiting for your feedback on #258.3 / #260.
Comment #266
SocialNicheGuru CreditAttribution: SocialNicheGuru commentedjust FYI .. this change breaks modules like Taxonomy Manager that rely on the the taxonomy_term_hierarchy table that is removed.
Comment #267
Wim Leers@amateescu Oh, thanks, I didn't realize that!
User
entity with ID zero existing!The danger with the value being different in-storage vs in-memory is that they need to be consistently translated, always. It's easier to have a single representation: less that can go wrong. What about
NULL
?I like it! I like the idea conceptually, but I do think that
<root>
versus numeric IDs in all other cases is gonna be a bit strange. But my alternate proposal of usingNULL
supports that just fine. Many languages have the concept of a "nullable type". In this case, it's then a nullable integer (in case of numeric IDs) and a nullable string (in case of string IDs). "string or int" is moreconfusing. Plus, in theory, the string
<root>
could conflict with an existing string ID, whereasNULL
definitely can't.This would actually be a trivial change for the test coverage:
Today, that constant maps to
0
, but now it'd change toNULL
.And actually, for the "link" concept that the HAL normalization has, the link already is a
NULL
link in the current patch:So it's just applying the idea that this patch is already doing more consistently.
+10000
@amateescu: so what do you think about
?Comment #268
damiankloip CreditAttribution: damiankloip at Acquia commentedAdding back the entity type update code for now. I would like to know the reason this is still needed too. We've tried to remove it 3 times now though, so this issue maybe needs it either way :P
Comment #269
Wim LeersWe'll need to change this comment now though.
Isn't this the reason we still need that line that #268 brought back?
Comment #271
damiankloip CreditAttribution: damiankloip at Acquia commentedRerolled.
Comment #272
Wim LeersThe #271 interdiff didn't make any sense. I manually diffed the #268 and #271 patches — they're identical, only context changes. IOW: straight rebase to make the patch apply cleanly to HEAD.
Back to NW for #258.3/#260/#267: we can't keep
const ROOT_ID = 0;
Comment #273
damiankloip CreditAttribution: damiankloip at Acquia commentedYes, sorry -that was just a diff of the fixed hunk from the reroll.
Comment #274
damiankloip CreditAttribution: damiankloip at Acquia commentedLet's try this to start with. Not sure if/how many issues we will hit trying to have no values stored...
Comment #275
Wim LeersWhy these changes in #274? This is removing lots of test coverage. Or is this just to check that at least things still work when using non-root parents, and you'll restore this test coverage in a next patch?
Comment #277
damiankloip CreditAttribution: damiankloip at Acquia commentedI'm not sure, there is no parent anymore, so no parent data will exist if we going with this NULL idea. What do we test in that case? setting a NULL value does in fact lead to no value being set? That seems like something that is already tested with fields in general.
Comment #278
damiankloip CreditAttribution: damiankloip at Acquia commentedMe and Wim had a call and decided trying to change the actual storage from 0 for root, to NULL (nothing persisted at the field level) is a much broader change than this issue should tackle. It opens a can of worms with how the UI handles 0 as the root value, amongst other things.
This is based on the patch from #271, It does:
- Removes ROOT_ID constant from TermInterface. I think @amateescu should be happier if we just don't have this? :)
- Adds term specific behaviour to entity reference normalization, so we will have NULL returned instead of 0 for the REST API.
- Adjust test coverage to use NULL for root parent instead of 0 (We'll see how testbot gets on).
- New unit test for EntityReferenceFieldItemNormalizer for the new term trait/behaviour.
Comment #280
damiankloip CreditAttribution: damiankloip at Acquia commentedComment #282
Wim LeersMissing docblock.
This would refer to @amateescu's
TreeInterface
once it exists.Comment #283
Wim LeersSooo … the failures are in
Comment
+Node
tests for thehal_json
format, specifically for updating fields.This is related to #2824851: EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior, which landed two days ago, because that patch removed the
hal_json
overrides for those tests ofprotected static $patchProtectedFieldNames
. See comments 74 and 75 on that issue.I did a very deep dive on debugging this. Turns out that those lines should not have been removed, and the changes in
EntityReferenceItemNormalizer
here happen to fix that. Specifically,\Drupal\Tests\rest\Functional\EntityResource\EntityResourceTestBase::getModifiedEntityForPatchTesting()
(which was added in that issue) sets a value of99998
for entity reference fields. Turns out HAL's entity reference field normalizer is broken in HEAD, because it doesn't deal correctly with references pointing to non-existent entities. That bug in HEAD caused therevision_uid
field forNode
to end up in thehal_json
serialization, but it shouldn't have been: that field should always have been removed in favor of a link in HAL's_links
section.This patch fixes that, and in doing so, is bringing back the discrepancy in
PATCH
field error checking, because this once again ensures that all reference fields in HAL normalizations live in_links
.Bringing back those deleted lines (reverting the #2824851-74: EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior + #2824851-75: EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior interdiffs) make tests pass, and per this deep debugging session, that's actually the correct behavior.
MAN this was hard to debug!
Comment #284
Wim Leers@amateescu's latest round of feedback from #258 has been addressed.
I don't want to hold this up for the tiny nit in #282.1. This should really land in 8.5, because it has been functionally ready for a long time now. 8.5 should not be yet another release in which you cannot access term parents via REST/JSON API/GraphQL.
I only fixed REST test nitpicks in #283 and helped out with tiny bits here and there. The real work has been done by others, primarily @damiankloip.
Let's get this in!
Comment #285
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedI've been thinking a lot about this in the last few days and I'm happy to say that I arrived at the same conclusion independently from your discussion with Wim :) Allowing the entity reference field storage to store
NULL
values for roots goes very much into the tree field territory, and should not be handled in here. I'm glad that it was so easy to expose the proper API for REST (root asNULL
) while keeping the current term storage intact (root as0
).#284 gives the feeling that we're in rush mode with this issue, so here's a patch which addresses my final remarks and a bunch of coding standards fixes.
I also debugged this and I found out why the entity type needs to be updated. Rewrote the comment above that line to say what's really going on.
Comment #286
Wim LeersWe are, so I very much appreciate your help in #285! ❤️ Those changes all look excellent!
Comment #288
Wim LeersLooks like #285 was rolled relative to #280 instead of #283.
Re-applying #283's interdiff.
Comment #289
BerdirI had a quick look at what happens here exactly, because based on that comment, I would expect that it then *removes* that no longer existing table, but that doesn't happen here because otherwise 8502 would fail hard.
Discussed with @amateescu, what we think is happening is that it sees that something changed but because we don't explicitly check for custom additional tables in \Drupal\Core\Entity\Sql\SqlContentEntityStorageSchema::requiresEntityDataMigration(), so we say removing a custom table is not a breaking change and we happily move on in \Drupal\Core\Entity\Sql\SqlContentEntityStorageSchema::onEntityTypeUpdate().
Then we get to the next check, if there is data, we just re-generate the indexes. And we save the new generated entity schema, where now the table doesn't exist anymore, but it still exists as an actual table in the database.
But one thing where it gets interesting when there are no terms, because then it will go into the if where it removes *all* tables and re-creates them, so now the table is gone.
So if I understand this correctly, running this update on a site with no terms would fail? I thought we also have empty update tests? Maybe I'm missing something...
Anyway, we should possibly....
a) open an issue to figure out if \Drupal\Core\Entity\Sql\SqlContentEntityStorageSchema::requiresEntityDataMigration() *should* care about custom additional tables somehow
b) Possibly move the entity type update into another update that runs *after* 8502, conceptually that makes more sense to me, first we migrate the data into the new table, then we tell the update manager that the old one now no longer exists?
Comment #290
amateescu CreditAttribution: amateescu for Pfizer, Inc. commented@Wim Leers, right, I forgot rebase my changes onto the latest patch :/
I looked a bit more into it after discussing with @Berdir and the reason why our custom table is not dropped when there are no existing terms is because
\Drupal\Core\Entity\Sql\SqlContentEntityStorageSchema::onEntityTypeDelete()
removes only the *known* entity tables with this code:In case we change that logic in the future, let's be safe here and move the entity type update right above the place where we delete the old table manually. This essentially implements #289.b), but without adding a new update function for it.
There's also a small change which I missed in #285, resetting the new
$ancestors
property in\Drupal\taxonomy\TermStorage::resetCache()
, not only in\Drupal\taxonomy\TermStorage::__wakeup()
.Comment #291
damiankloip CreditAttribution: damiankloip at Acquia commentedThanks Andrei! This is great. The reasoning does make sense when you think about it... :)
+1 on your recent changes.
Comment #292
larowlanThe issue summary references #201.4 as TBD but it seems that has been resolved, so tagging for IS update.
Comment #293
Wim LeersYou're right. Updated IS.
Comment #294
Wim LeersAlso: this has been reviewed and RTBC'd by the Field API maintainer. I think that means we can remove the
issue tag.Comment #295
larowlanI had some concerns about the scope here.
The issue is fixing the parent field to make it an ER field and fixing rest/serialization support, which feels like its doing two things.
I discussed it with @WimLeers and @amateescu on slack/irc.
Wim pointed out that
If it were months out from alpha, we could punt on both being in, but if we were to split it now, and commit the first bit (data model changes) but miss the second bit (rest changes), we'd ship 8.5.x with a broken REST api that would be an API break if we changed it later.
So in that regards, we have to do both at once.
Comment #296
larowlanDid some manual testing as follows
Screenshots from testing attached
Going to have a crack at upgrade path test locally.
Comment #298
jibranI think we need a change record here.
Comment #299
larowlanTested the update path.
Before and after shots of my hierarchy - worked.
But the views update didn't work.
Here's my view as yaml, before and after. No change. I had two taxonomy fields on my article.
Also, the code expects everything to be .parent. But as the yaml shows, if I add two filters of type parent, then the second one gets parent_1
And doesn't get updated.
Comment #300
larowlanAfter the update hook, my filters disappeared
Comment #301
damiankloip CreditAttribution: damiankloip at Acquia commented@larowlan, thanks for testing this out thoroughly. I think I see the issue - I guess (and this is just a assumption for you to confirm or deny at this stage :) ) you did:
- Fresh install on 8.5.x branch
- Create views
- Run updates
-> View has broken handlers
?
If so, I think the issue is that we split the underlying views fix out into #2846614: Incorrect field name is used in views integration for multi-value base fields, which contains the update hooks for the views data. They were originally intended to be run together until we split them up (to reduce the scope of this issue). So your site install based on 8.5.x before this patch would not run the views update handler in the above issue. We then change the final pieces of the views data here, and your view doesn't get updated because the only new update handlers to run are the ones in this issue.
If this is not the case, we will obv. need to dig further.
In a nutshell, I think this is working as expected if the update path contains both, i.e. 8.4 > 8.5.
EDIT: Disregard this, mostly. We do have the other updates in this patch! The other patch is multi value fixes...
Comment #302
Wim LeersBack to RTBC per #301 then.
@damiankloip: it'd be great if you could verify your guess/assumption though, because @larowlan is in Australia and hence is basically awake when we're asleep and vice versa. Then we'll know for certain that either your guess is correct, or this patch needs changes, which you can then still work on today, rather than on Monday, at which point it'll likely be too late.
Also, does this mean that if this patch doesn't land in 8.5, that 8.5 will be in an inconsistent state?
Comment #303
volegerI guess this code don't include the next possible configs for update :
Comment #304
damiankloip CreditAttribution: damiankloip at Acquia commentedYes indeed - we need to add more smarts to that update hook logic.
Comment #305
heddnLinking #2935951: Copy migrate source plugin from migrate_drupal_d8 into migrate_drupal as another reason for this change. REST wants to export things. Migrate's d8 source does too.
Comment #306
Wim Leers#305, nice! Adding tag — hope it's the right one, feel free to correct!
Comment #307
larowlan@damiankloip yes I did start with 8.5.x head. Should I try again with 8.4.x as the start point?
What about the parent_1 issue?
Comment #308
Wim Leers@larowlan: @damiankloip confirmed (but not in many words) the "parent_1" issue in #304. He's working on it.
Comment #309
damiankloip CreditAttribution: damiankloip at Acquia commented@larowlan, I thought that was originally the problem, but I jumped to a conclusion without refreshing my memory on exactly what we covered in the other issue. Subsequently, my memory let me down slightly :D So we definitely need a fix for the static use of 'parent' in the update path ( @voleger, you are also correct in #303).
This patch should fix that. This works for me, with your test view from #299. I have also expanded the update path test to cover this case; The test view used contains 2 filters now, and the update path asserts that both filters get updated correctly.
@larowlan, would be grateful if you could help confirm this again! cheers.
Comment #310
Wim LeersSo from a single loop and hardcoded handler type in
$base_path
…… to the former loop becoming the outer loop and this one becoming the inner loop, and
$base_path
including a parametrized handler type.Plus expanded test coverage.
Back to RTBC!
Comment #311
larowlannice work!
I manually tested this again, this time going from 8.4.x HEAD to this patch, and it works as designed.
We need a change record here.
But also, I'm thinking about the impact on contrib and custom modules here.
We've got a fair few changes in this patch that are dealing with using the new table instead of the old hierarchy table.
So I looked into our BC policy on that
So I think that our change notice should reference that policy with a direct link and this text.
NR for the change record
Comment #312
larowlanAnd we definitely need to highlight this in the release notes
Comment #313
Wim LeersWill write CR in the CET morning.
Comment #314
Wim LeersCreated CR: https://www.drupal.org/node/2936675.
Note that 99% of them are for updating the Views integration!
Comment #316
larowlanAdding review credits for those who helped shape the patch.
Comment #317
larowlanA survey of contrib modules was conducted with damien mckenna and berdir.
We surveyed 89 common contrib modules for Drupal 8 and found only one, Facets module, was using a direct reference to taxonomy_term_hierarchy table.
They already have an existing bug for that, as using the table direct won't work with non SQL entity storage.
@SocialNicheGuru pointed out that this will also break taxonomy manager module.
This module is only in -dev status, and looks like it has a lot of D7 era code - e.g. http://cgit.drupalcode.org/taxonomy_manager/tree/src/TaxonomyManagerHelp...
@SocialNicheGuru has already opened #2932067: Future patch to remove taxonomy_term_hierarchy for that case.
I've discussed the impact of committing this to Drupal 8.5 with @catch.
We've agreed to commit this to 8.6.x and make a decision about backporting to 8.5.x as soon as possible.
Committed as 8640a9d and pushed to 8.6.x
Will hold off publishing the change record until final decision is made on backport.
Leaving at RTBC for 8.5.x
Comment #318
Wim LeersThanks! And thanks for the impressive research you applied to contrib!
Comment #319
heddnDiscuss among yourselves, but waiting 8 months to get term parent seems a little long for things like #2935951: Copy migrate source plugin from migrate_drupal_d8 into migrate_drupal. Without this feature, we won't be able to fully export the term entity.
Comment #320
larowlanYes, but we also need to consider the perceived stability of Drupal 8 as a platform.
There are 1000 sites running taxonomy_manager and 6000 running facets.
Breaking them with short notice doesn't help the image of Drupal 8.
Yes, both of those modules are doing the wrong thing by querying the table direct.
Yes, one of them has a patch already.
We just need to be mindful that people have live sites that this will impact on.
Comment #321
Wim Leers@larowlan 👏
The main problem is that Drupal has traditionally not been great at marking what is an API and what is an implementation detail, which is why we end up with those perceived stability problems and tricky BC decisions (I did a talk about that: https://wimleers.com/talk/bc-burden-benefit).
taxonomy_manager
module that makes the module work both with and without this patch/commit!Therefore I think it's safe to say that by the time Drupal 8.5.0 ships in March, both modules will be updated and ready. IOW: it should be safe to commit this to 8.5 now!
Comment #322
xjmRegarding #321: Having fixes for known contrib modules is, unfortunately, not the only thing we need to consider in terms of disruption. If we know about multiple contrib modules that will be broken by this, then there are very likely sites with custom code that also relies on the current behavior that will be broken.
Comment #324
Wim LeersAgreed. We have to weigh risks, pros, cons, and so on.
On the other hand, if developers choose to not use the in this case pretty clearly documented APIs, and choose to depend on internal implementation details, then Drupal would only able to add things.
Good luck with making a decision, I know it's tricky to balance all the requirements!
Comment #327
xjm@heddn, regarding #319, is this issue actually a hard blocker for that one? @catch and I were unsure how it would be at first glance. (It seems to already have a patch without this.)
Comment #328
heddnre #327: No, it is a soft blocker. What I'd imagine we would do is have two patches. One for 8.6 and one for 8.5. Where 8.5 wouldn't have the parent functionality in it. Then, as a work around if someone needed the parent, they'd have to continue using contrib migrate_drupal_d8 or create a follow-up to add a deprecated class to core that adds the term parent via a prepareRow callback. This would be introduced as deprecated and only be a patch for 8.5.
We can get away without having parent for core's initial purposes because we don't need that data to unblock the critical release blocker for migrate_drupal. But it is an incomplete implementation in 8.5, so the change record could get a little confusing. And I don't really like adding a deprecated class, because that just gets messy for BC. TLDR; it is a soft blocker, so we'll do what the community desires.
Comment #329
Wim Leers#328: Do I understand you correctly that with just core's Migrate module, you'd lose your term parents, you need the contrib module https://www.drupal.org/project/migrate_drupal_d8 to not suffer any data loss during migrations?
Comment #330
heddnre #329: for core's initial use of its own version of the contrib rendering of d8 source plugin that is forked and improved from contrib... we do not need the parent. In 8.5 we'd be OK to unblock the last few stable blockers for migrate without that feature.
If someone needs/requires the parent in the 8.5 timeframe, they would need to open an issue to have the feature backported and we'd have to alter the deriver to spit out the special snowflake term plugin that returns the parent. Or, alternatively, they can use contrib. Or they can wait for 8.6 or upgrade to 8.6 and use core's generic source plugin.
The reason this works in 8.6 is that we use the entity API to gather all the source details. And the entity API for terms only get fixed with this issue.
TLDR; data loss could occur to the uninformed developer using the entity source plugin during the 8.5.x timeframe. Because the uninformed developer wouldn't know that parent isn't available. Alternatively, we could introduce a special snowflake that is @deprecated from its infancy. This special snowflake would only exist in the 8.5 branch. But this special snowflake stuff would all go into a follow-up where we could discuss actual implementation because our goal it to unblock migrate. We just need to know if we need to open that follow-up, and switch out the @TODO to point to it.
Comment #331
Wim LeersSo the answer to #329 is "yes, there would be data loss".
Comment #332
Wim Leers#2919034: FacetsHierarchy Taxonomy uses old (taxonomy_term_hierachy) table. landed, which means https://www.drupal.org/project/facets will soon ship a release that is compatible with this patch.
For https://www.drupal.org/project/taxonomy_manager, I posted a working patch two days ago at #2932067-9: Future patch to remove taxonomy_term_hierarchy, but haven't gotten any feedback. That issue was first filed on December 20, almost a month ago. Yet since then, only 3 followers: the original reporter (@SocialNicheGuru), @larowlan, and I. Which is understandable, since that module A) has no stable release for D8, B) its D8 codebase is littered with dead D7 code, C) has no active maintainer. Hence waiting to commit this patch to 8.5 until that patch gets committed may be disproportionate. When maintainers do respond to users complaints, they will have a patch awaiting them.
Comment #333
effulgentsia CreditAttribution: effulgentsia at Acquia commentedI discussed this with @larowlan, @catch, @xjm, and we're all more comfortable with not cherry picking this to 8.5.x. Because even though everything it does is compatible with our policy for what's allowed into a minor release, the reality is that per #320 and #322, there are some known contrib modules and unknown custom modules, that are accessing internal implementation details rather than stable APIs, that will be disrupted by this. That's fine, but it's far more courteous to provide reasonable lead time for those affected by this to discover it and adapt, and I think the time left before 8.5.0 is released isn't sufficiently courteous.
I realize that's disappointing for REST/JsonApi/GraphQL/etc. use cases. If there are decoupled sites in desperate need of reading or writing
$term->parent
, perhaps some temporary taxonomy hierarchy API could be provided in contrib until 8.6.0 is released?I discussed #331 with the Migrate maintainers. While a data loss in 8->8 migrations is not ideal, we can all live with it for now, because while getting 6->8 and 7->8 migrations stable is critical, 8->8 migrations are currently a minority use case and therefore of lower priority for now. Also, @heddn suggested that a workaround to fix the 8->8 data loss problem of
$term->parent
is relatively straightforward and will open an issue in which to do so, which might land in time for 8.5.0, or if not, then perhaps in a patch release soon following. Thank you, Migrate maintainers, for your understanding and flexibility about this.Most importantly, a big thank you to everyone who's worked on this issue! I'm thrilled that it's in HEAD, so that all future core work (such as #2836165: New experimental module: JSON API) can start benefitting from it, even if it's still a bunch of months away from making it into a tagged release.
Comment #334
xjmAnother thing that we discussed is that we'd like to explore some way of warning developers that the table is deprecated in 8.5.x. I would have preferred that that happen before commit, but can we get a followup issue for that?
Comment #335
borisson_I tagged a new release right after I committed that issue. Thanks again for helping out on that @larowlan, @Wim Leers @Berdir.
Comment #336
Wim LeersSuch a temporary API in contrib requires disproportionately much effort from those who helped fix the root cause of the problem, i.e. those who helped fix this issue.
Those who want this to work should just apply this core patch to Drupal 8.5, just like they already have been doing in 8.4. With the knowledge that as soon as they update to Drupal 8.6, that won't be necessary anymore. That's what e.g. the JSON API module has been recommending (in #2775635: [BUGFIX] Parent is always empty for taxonomy terms).
I don't see how we could throw
E_USER_DEPRECATED
errors for database queries. I think a change record for 8.5 that warns about the transition in 8.6, and an explicit mention in the 8.5 release notes are the best (only?) ways to communicate this.Woah, awesome, thanks @borisson_! :)
Comment #337
dawehnerOne could come up with an idea which involves the database layer directly, but I'm not sure its worth the performance impact.
Comment #338
Wim LeersRight, that is what I implied: we could do parsing of queries, but that seems pretty excessive.
Comment #339
catchYes I think this is fine, and it's much nicer having a patch applied when you know it's been applied to the next minor branch than when it's stuck at CNW/CNR.
Comment #340
BerdirAbout the messages, I'm also against doing anything there. It's not actually deprecated, it's just not supported by BC and should not be done *if possible*. But the entity/term storage itself continues to and will always do those kind of queries, obviously. And it would be insane to try and detect who can do such a query and who can't :) Oh and views of course ;)
Truth is that sometimes there are scenarios where it can't be avoided, especially for your own entity types but also others.
Comment #341
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedWe also have the option to do #2921048: Introduce temporary work-around to make $term->parent work in REST API + JSON API in Drupal 8.5 — 8.6 ships with root cause fix for 8.5.x, which would give us all the benefits of this patch (working REST support, etc.) without the BC-breaking storage changes.
Comment #342
larowlanPublished the change record
Comment #343
Wim Leers@amateescu: good point! ❤️👍 Commented at #2921048-20: Introduce temporary work-around to make $term->parent work in REST API + JSON API in Drupal 8.5 — 8.6 ships with root cause fix.
Comment #344
Wim LeersIn other happy news, #2932067: Future patch to remove taxonomy_term_hierarchy was also committed, so Taxonomy Manager is also forward-compatible with Drupal 8.6/this patch!
Comment #346
larowlanFound another module that uses the taxonomy_term_hierarchy table - shs module.
#2942358: The taxonomy_term_hierarchy table will be removed in Drupal core 8.6.0
Comment #347
Wim LeersMinor follow-up: #2942522: Follow-up for #2543726: remove work-around in POST test coverage for bug in Term entity type that has been fixed.
Comment #348
Wim LeersFor #346.
Comment #349
heddn#2935951: Copy migrate source plugin from migrate_drupal_d8 into migrate_drupal just landed and #2940198: Load taxonomy term parents in ContentEntity source plugin can not be unpostponed to backport the term parent to 8.5.
Comment #350
heddnFound another module #2951288: Do not use removed 'taxonomy_term_hierarchy' table
Comment #351
giorgio79 CreditAttribution: giorgio79 commentedWould be nice to review Drupal where other parent fields are used, eg menu entities: #2845485: Refactor and document the MenuLinkParent process plugin so custom implementations can be removed.
Comment #352
adhariwal CreditAttribution: adhariwal at Axelerant commentedThe patch #309 is failing on 8.5.4 branch.
Comment #353
bander2 CreditAttribution: bander2 as a volunteer commentedRe-roll for drupal 8.5.4
Comment #355
bander2 CreditAttribution: bander2 as a volunteer commented#353 was bad. This one is better. It's just a re-roll of #309 for drupal 8.5
Comment #356
otrolopezmas#355 fails to apply for me in drupal 8.5.4.
Comment #357
markdorison#355 applies cleanly on 8.5.4 for me. It's too bad testbot isn't running on this since the issue is closed.
Comment #359
Wim LeersComment #360
alzz CreditAttribution: alzz at Metadrop commented#355 applies ok on 8.5.6
Comment #361
xjmNote that this will not be committed to 8.5.x.
I think https://www.drupal.org/node/2936675 covers the important update information we'll want to reference in the release notes. (Whenever we tag something for release notes, we should document why it needs to be listed, i.e. what the impact on site owners or developers is.) Thanks!
Comment #362
fgmJust noticed that trying to upgrade from 8.5.6 to 8.6.0-rc1 fails because drush updb (drush 9) tries to use taxonomy_term__parent while the DB still contains taxonomy_term_hierarchy instead. Not checked why this happens, but I think this is a release blocker.
Comment #363
BerdirDid not have such a problem (with drush 8), are you sure that's not a custom update function that's incorrectly loading entities or something like that?
I'd suggest to open a seprate issue, with the full output that you have, specifically interested in a backtrace as well as the list of update functions to run and what has been executed by that point.
Comment #364
fgmWill do that in a moment, right now AFAICS: install core 8.5.6 standard profile / install drush 9.3.0 / checkout core 8.6.0-rc1 / drush updb: get a fatal error before starting the first update hook. Hope I missed something.Found the culprit: the instance where it happened happened to have extra Drush 9 plugins present, and Drush 9 instantiates all of its command plugins even when not using them... and the service constructor happened to load terms. Deferring the load to service usage instead of construction removes the load and makes the issue go away. So it's not a core bug, sorry for the noise but given the planned release tomorrow, I thought it best to raise an alarm ASAP.
Comment #365
Wim LeersTurns out there still was a problem in the update path: #2997960: Missing taxonomy hierarchy items in 8.6.0 after running taxonomy_update_8502.
Comment #366
huyby CreditAttribution: huyby commentedWhen updating core to 8.6.1 from 8.4.x, which was already patched, having update hooks that "drop" the "taxonomy_term_hierarchy" table.
How do proceed?
Obviously as 8.6.x contains the fix for this issue, it has taxonomy_update_8502() that will cover the same logic and "drop" that was already introduced in the patch, which throws errors because the "taxonomy_term_hierarchy" table doesn't exist anymore.
Is it common to use another patch? So it's possible to bail early in taxonomy_update_8502, if "taxonomy_term_hierarchy" table is missing in the DB?
Patch that was used on 8.4.x: https://www.drupal.org/files/issues/2543726-143.patch
https://www.drupal.org/files/issues/2174633-174.patchComment #367
effulgentsia CreditAttribution: effulgentsia at Acquia commentedI don't see how the patch linked to from #366 is relevant, but if you were running this issue's patch #159 or earlier, then yes, you might already have {taxonomy_term_hierarchy} dropped from taxonomy_update_8304().
Here's a patch that should bail early from taxonomy_update_8502() if you're in that situation. Note, I haven't tested it, so make sure to make a backup of your database before trying it.
Comment #368
huyby CreditAttribution: huyby commented@effulgentsia sorry, I've messed up linking to another patch which I was updating 🙈
Thanks for the patch. I had created a similar patch, good to have a reference.