Problem/Motivation
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().
Example breakage from the above issue:
- Base field is create with name 'parent', and it's a multi-value field - this means it gets it's own storage table, 'taxonomy_term__parent'
- This table does not have the same format as base fields in the base table, so 'parent' would not be named in a single column 'parent', the actual value field is 'parent_target_id' - similar to any other configurable field
- When the views data is generated, it assumes the field name due to this code:
$multiple = (count($field_column_mapping) > 1);
$first = TRUE;
foreach ($field_column_mapping as $field_column_name => $schema_field_name) {
$views_field_name = ($multiple) ? $field_name . '__' . $field_column_name : $field_name;
$table_data[$views_field_name] = $this->mapSingleFieldViewsData($table, $field_name, $field_definition_type, $field_column_name, $field_schema['columns'][$field_column_name]['type'], $first, $field_definition);
$table_data[$views_field_name]['entity field'] = $field_name;
$first = FALSE;
}
So the wrong assumption is that any field that only has one column in the mapping must have the same column name as the field name itself! Which is not a correct assumption. This is only TRUE (and because the storage implementation happens to use the same names) for base fields that share a table.
Another side affect of this, the Views UI contains two handler entries for each of these fields, one working implementation and one broken one, for things like user roles. Only because UserViewsData is explicitly creating the 'roles_target_id' item itself, to basically get around this bug. Some of this data is just wrong, it doesn't work, and it should be be there.
Proposed resolution
Use the schema field name, as it already generates the correct field name. Still use the field name for the 'entity field' value. The tests need to be fixed as they expect the wrong values in some assertions.
Remaining tasks
User interface changes
Some duplicate handlers will be removed in the Views UI listing
API changes
Data model changes
Comment | File | Size | Author |
---|---|---|---|
#118 | 2846614-117.patch | 25.23 KB | tstoeckler |
Comments
Comment #2
damiankloip CreditAttribution: damiankloip at Acquia commentedComment #3
damiankloip CreditAttribution: damiankloip at Acquia commentedComment #5
tstoecklerI had opened #2838040: Multi-value multi-column base fields are queried with incorrect column names in Views for that before. Marking that one as duplicated, though, as I didn't have a patch there.
Comment #6
tstoecklerPatch looks great, makes totally sense. One nitpick:
Can we change that to return
string_value
instead, so it matches the real world scenario? This way it is very confusing, IMO.That's not introduced here, but I still it would be in scope to fix that as part of this issue since we have to change the tests anyway.
Comment #7
tstoecklerAlso we should look at how this affects the generated views data of the user roles field as that is a multi-value base field, as well.
UserViewsData
does contain some overrides for that, but not sure if in the aggregate anything is changed/broken by this.Comment #8
Wim LeersThe explanation in the IS makes total sense.
NW for #6 and #7.
Comment #9
damiankloip CreditAttribution: damiankloip at Acquia commentedThanks tstoeckler, and sorry for not seeing your original issue. Daniel pointed me to that afterwards, but by then you'd already commented here :)
Yes, changing the name to the 'real' column value is a good idea, ++ to that.
So yes, user__roles is really the only case we have in core right now, I think. It will mean the 'user__roles' > 'roles' item will not exist anymore, and the 'user__roles' > 'roles_target_id' will be created correctly, we would just need to modify it to add our own handler IDs instead of creating it from scratch like it currently happening. That's actually a clear sign that our integration is currently broken too.
So maybe we need to write an update handler to fix this in user module? but if people are using that version, it's probably not working correctly anyway? or, we just copy the new data over to 'roles' too, so it's still there.
Comment #10
damiankloip CreditAttribution: damiankloip at Acquia commentedHere is the cleanup for #6 and some other stuff we can remove now we have user__roles table data being generated correctly. Just need to address the loss of the 'roles' item. I'm thinking an update handler is the best way to go here..
Comment #11
damiankloip CreditAttribution: damiankloip at Acquia commentedUgh, forgot to attached the patch!
Comment #12
Wim LeersI'd love to RTBC, but can't really do that :)
In any case, this is blocking #2543726: Make $term->parent behave like any other entity reference field, to fix REST and Migrate support and de-customize its Views integration.
Comment #13
jibranIt is really great to see #2543726: Make $term->parent behave like any other entity reference field, to fix REST and Migrate support and de-customize its Views integration moving forward.
This should be updated as well.
This means we need an upgrade path and upgrade path tests.
Comment #14
damiankloip CreditAttribution: damiankloip at Acquia commented1. Why do we need to update that todo comment? That's not anything to do with this fix.
2. Yes we need an update handler I think. Not sure about update path tests though. That seems overkill to me, for essentially a broken handler. Also how do you test two versions of generated views data that's only cached? I guess the best we could do is have a view with the old data and test it gets updated maybe.
Comment #15
damiankloip CreditAttribution: damiankloip at Acquia commentedHere is the update handler.
Comment #16
jibranThanks for the update path.
This should be done in post-update hook and we should store the basefields in KV store.
After that update hook should run and update the views config.
Comment #17
dawehner@jibran
I'm confused about your suggestion. Post_update should be used in case you have a working environment, everything is up to date. I'm not sure how you imagine an environment is working, when views are pointing to invalid views data entries. These views are not runnable at that point.
Given that I think using
hook_update_N
is the sane choice to go here.Comment #18
claudiu.cristeaHere is the fix for #13.1.
Comment #19
jibran@dawehner let's break it into two update_N hooks so that contrib can also do their thing. Thanks @claudiu.cristea for the fix.
Comment #20
damiankloip CreditAttribution: damiankloip at Acquia commentedNo, IMO #13.1 is out of scope of this issue. This is not what it's trying to fix - otherwise I would have fixed it :)
Comment #21
dawehnerDo you have a concrete usecase? Why can contrib not just run after the other update_N? You know a concrete usecase would be really helpful.
Comment #22
claudiu.cristeaRe #20:
@damiankloip, OK. Opened #2847034: Use getMainPropertyName() instead of first column when building entity views data for that. We'll see which gets in first and adapt the other.
Comment #23
damiankloip CreditAttribution: damiankloip at Acquia commentedOK, thanks. Although #2543726: Make $term->parent behave like any other entity reference field, to fix REST and Migrate support and de-customize its Views integration is now dependent on this issue, but not #2847034: Use getMainPropertyName() instead of first column when building entity views data so that could slow this issue down...
Comment #24
claudiu.cristeaStill needs tests for upgrade path.
Let's give a chance to exit early here if $table_update_info is empty.
We need to open a new group for 8.3.x updates and include the new function there.
Comment #25
jibranRE #21: If you think it is not worth it then just leave it. :)
NW as per #24.
Comment #26
damiankloip CreditAttribution: damiankloip at Acquia commentedGoing to work on an upgrade path test with a view being updated.
Comment #27
damiankloip CreditAttribution: damiankloip at Acquia commentedHere are some of those changes and a start on update tests. They currently don't work. Lots of schema validation issues when the views are saved I think... Amongst other things.
Interdiff is from #15.
Comment #28
jibranThese should be enabled in additional dump script.
This should also be moved to active storage in extra dump script.
Comment #29
damiankloip CreditAttribution: damiankloip at Acquia commentedYeah, there is still a WIP currently :/
Comment #31
damiankloip CreditAttribution: damiankloip at Acquia commentedHere is an updated update path test with the standard d8 db dump but a new views data file to add the view the update handler should fix.
Comment #32
dawehnerThis looks exactly like I woul dhave done it.
Comment #33
Wim LeersYay!
Comment #35
claudiu.cristeaThis is new code so let's use square bracket array syntax.
Comment #36
damiankloip CreditAttribution: damiankloip at Acquia commentedOK, thanks. That will be in the next patch.
Comment #38
Wim LeersComment #39
dawehnerI'm still trying to fix the update path 100%.
Comment #40
Wim LeersThanks, @dawehner!
Comment #41
dawehnerThis was a bit of a fight.
Comment #42
tstoecklerWow, that looks like quite some fun. My brain ran away when I opened the patch file, so only a cursory review for now.
use a proper use ;-)
That's fun! :-)
Since you're only using the singular version in a single place, you could also just do the substr() in the loop instead of up-front, but that's just me.
The inline typehint got lost a bit, I guess.
Unneeded
Comment #43
xjmComment #44
damiankloip CreditAttribution: damiankloip at Acquia commentedOne word - legendary. This is pretty much what we need I think.
We might need to be more specific...
Nice that we have consistent name spacing form user module plugins :P
I think we might need to check the config schema here to determine whether this should indeed be an array or not. E.g. if the new handler is also a standard filter, a string would be correct.
Oh, do we need to do separately so the config is reloaded? Maybe a quick addition to that comment.
Comment #45
damiankloip CreditAttribution: damiankloip at Acquia commentedComment #46
dawehnerGood point. I think using singular by default is a better solution;
Indeed. Lost in code.
Fair point ...
Life isn't fair
Any idea how we could do that?
Yeah
Comment #47
damiankloip CreditAttribution: damiankloip at Acquia commentedI think as we already have the loaded schema object, we should be able to just check the type of the value property from there?
Comment #48
dawehnerSo something like this?
Comment #50
Wim LeersComment #51
dawehnerHehe, well as you see it doesn't work.
... I rewrote the logic to just load each view once. I don't understand why, but you know it works.
Comment #52
claudiu.cristeaCool! Looks nice.
Typo: he > the. Can be fixed when committing the patch.
Comment #53
damiankloip CreditAttribution: damiankloip at Acquia commentedYes ++ This is a change that I had locally to fix a small issue before I realised you were fixing the patch up.
The changes you've made for the update schema stuff makes sense to me now, and works nicely!
RTBC +1
Comment #54
BeakerboyIs this bug going to be fixed in The Drupal 8.3 series, or do we have to wait for 8.4?
Comment #55
BeakerboyPatch 51 has been tested on 8.2.4 and it works. Another RTBC.
Comment #56
BeakerboyIs it possible that this patch breaks something with multi-value fields? I created a view between a base table and another Entity that has unlimited cardinality. This Entity has a multi-value field also with unlimited cardinality. The view's results display fine as an unsorted list after applying the patch, but if I try to push the results into a custom views formatter, I have to do some sneaky stuff the get the correct value:
My field is named 'foo', and the subfield is 'bar'.
Comment #57
damiankloip CreditAttribution: damiankloip at Acquia commentedDo you have sample configuration you can attach? Also, are your custom fields attached to the new/correct columns (field_name > field_name_column_name), not the old ones (field_name > field_name)?
Comment #58
Beakerboyin the example above, where $field_name is 'foo_bar', the field is named 'foo', and it contains a few columns, one of which is 'bar'.
Comment #59
damiankloip CreditAttribution: damiankloip at Acquia commentedRight, I saw that, but that doesn't give me any additional info about your custom handler :)
Comment #60
Wim LeersThis has now been blocking for precisely the entire month of February: #2543726-96: Make $term->parent behave like any other entity reference field, to fix REST and Migrate support and de-customize its Views integration.
Comment #61
dawehnerWell getting 8.3.x out is much more important than a bugfix, which will be fixed after 8.3.x is out. Distracting committers is IMHO always a bad idea.
Comment #62
Wim LeersOf course. The core committers have been doing a fabulous job :)
But that doesn't mean Drupal would not benefit from more core committers.
Comment #63
dawehnerI kinda believe that this is not the biggest problem we have :)
Comment #65
daffie CreditAttribution: daffie commentedComment #66
jofitz CreditAttribution: jofitz at ComputerMinds commentedRe-rolled.
Comment #67
Wim Leers#66 matches the previous patch (in #51). Just updated for short array syntax.
Comment #69
gaurav.kapoor CreditAttribution: gaurav.kapoor at OpenSense Labs commentedComment #70
gaurav.kapoor CreditAttribution: gaurav.kapoor at OpenSense Labs commentedComment #72
BerdirSomething with the reroll in #70 went wrong, started again from #66.
Comment #73
damiankloip CreditAttribution: damiankloip at Acquia commentedLooks good - back to RTBC
Comment #74
alexpottComment #75
dubcanada CreditAttribution: dubcanada commentedWhat is the status of this? It's holding up a few other issues.
Comment #76
Wim LeersThis needs an answer to #74.2, possibly a reroll.
Comment #77
LendudeWell the answer to #74.2 is 'Yes, we do need something like that' :).
See #2870724: Define and document best practices for configuration entity updates/bc layers for the route we take at the moment for implementing #74.2.
Comment #78
dagmarThe main problem I see here is that the assumption of using hook_ENTITY_TYPE_presave mentioned here https://www.drupal.org/node/2851293#comment-12043751
Is not valid here.
The hook update provided by this patch first makes a list of all the tables relevant to specific entities and then iterates through all views/handlers to find matches. I'm seeing a big performance impact if we try to do the same that we did in #2851293: dblog is using the wrong views field, filter and relationships definitions
Do anyone knows a better aproach for this case?
Comment #79
damiankloip CreditAttribution: damiankloip at Acquia commentedI agree there is going to be a lot of logic needed if this does go into a presave hook. There is not much in the way of alternatives afaik. As I mentioned before, this is also an awful lot of work for a group of essentially broken handlers :( We pretty much have an unmaintainable system here.
Comment #80
Wim Leers:(
Are the last few comments really saying this is basically impossible to fix?
Comment #81
damiankloip CreditAttribution: damiankloip at Acquia commentedI think we certainly have to fix this, the current views data is just wrong. Which means by default all multi value fields are broken. It's just frustrating with all the hoops we need to jump through for BC for existing broken handlers. That if used, people would mostly have broken views anyway... I think it's quite an edge case that a module would be providing default configuration in a view that doesn't work? The only case that kind of works ok I think is the field handler(s). I wonder if we could copy views data to the old location for field handlers only? That could be a work around here.
Comment #82
dagmarQuestion. Could we make this a hook_requirement task and regularly check for new views/entity types created with cron? So we can discover broken views without affect the performance of the system each time a view is saved.
Maybe we can have a list of recently updated views with a timestamp and check the new ones based on that information.
Comment #83
damiankloip CreditAttribution: damiankloip at Acquia commentedPotentially.. still seems like a disproportionate amount of work for fixing broken views handlers though :)
What do you think about copying the views data back to the old key for fields only? The rest are almost certainly broken anyway. If people are making alterations in their code, we have no real way to fix that, unless we duplicate all data to the old location too, which I kind of don't like.
Comment #84
jibranThis is a major bug. I'm voting for committing it as is and announce the BC break.
I had a look at the patch and
foreach ($entity_type_manager->getDefinitions() as $entity_type_id => $entity_type) {
part of the patch can be replaced by collecting all the entity types of a saving view and rest of the code would be the same but to call that inpreSave
would have large performance impact.I really don't like solution purposed in #83 but if that's the only choice we have to maintain BC then I think we can opt it. As @damiankloip purposed the solution I think we need a +1 from @dawehner or @Lendude. Also before implementing anything, I think an approval from framework manager is also required.
Changing it to NR and adding tags.
Comment #85
damiankloip CreditAttribution: damiankloip at Acquia commentedYes, I would rather just have a break, as it's for stuff mostly..broken :) The copying of views data is kind of a last resort for me. Well, actually, I would prefer that over something that will be running on every view save operation.
Comment #86
Wim LeersPer #2543726-111: Make $term->parent behave like any other entity reference field, to fix REST and Migrate support and de-customize its Views integration, can we please move forward here? This has been blocking that issue since January at minimum now…
Comment #87
Wim LeersActually, per #84 and #85, this actually should be RTBC again it seems.
Comment #89
Wim LeersPatch no longer applies, conflicts with #2868841: \Drupal\user\Plugin\views\filter\Roles::calculateDependencies breaks when using the empty/not empty operators .
This one looks a bit more difficult to resolve, so I'm leaving it to those who know
\Drupal\user\Plugin\views\filter\Roles::calculateDependencies()
better to resolve it.Comment #90
damiankloip CreditAttribution: damiankloip at Acquia commentedI think this should do it.
Comment #91
Wim LeersThese are the changes now.
Versus this before.
… that makes sense :)
Comment #92
effulgentsia CreditAttribution: effulgentsia at Acquia commentedI don't think Views with field handlers is exactly an edge case, is it? So in other words, we have this in the patch:
This is for a module-provided View that works perfectly fine in HEAD (AFAIK), and has a
EntityViewsWithMultivalueBasefieldTest
test to prove that. But if we commit this patch without the above hunk, then that View (and test) would become broken. Which means if we commit this patch as-is, then it is a real BC break that would impact sites using contrib modules with similar kinds of exported Views that the contrib authors have not yet re-exported.To be honest, I'm not 100% convinced that we need to babysit non-updated contrib. Default config is part of code, and we allow for some BC breaks in code (just not in the API) between minor releases. However, https://www.drupal.org/core/d8-bc-policy says that "References to plugin IDs and settings in default configuration can be relied upon however.", and if we're serious about that, then I guess we do need to preserve BC in this case (since this part of the configuration is a plugin setting, I think). There's also some background reading in #2870724: Define and document best practices for configuration entity updates/bc layers and #2248983-130: Define the revision metadata base fields in the entity annotation in order for the storage to create them only in the revision table.
Tagging for release manager review as well as framework manager review, because I definitely see the tension here between keeping our promise as it's currently written in https://www.drupal.org/core/d8-bc-policy and the concerns expressed in #84 about the performance impact on every View save of doing so.
I'm also curious if it's possible to only do the update logic on the first save from default config to active config rather than on every save within active config. Not sure if that's already been discussed on any of the other issues where this has come up.
Comment #93
damiankloip CreditAttribution: damiankloip at Acquia commentedField handlers weren't included in the 'edge case'. It is sorts/filters/arguments (that would alter queries) that are the ones that'll be broken. Most field handlers use the field field handler which doesn't care what actual column it is really, as it just renders it from the entity.
As mentioned somewhere above, we could also omit fields from this patch so they can keep the wrong column names. I don't mind. It shouldn't be this hard to fix something which is completely broken.
Comment #94
catchMaking sure I understand the bc implications correctly:
1. Field handlers affected by this probably work, the reason they work is because they don't reference column names anyway usually (values are loaded from the entity). So while a view using them might work and be stale, it will continue to work even though it's stale.
2. sort/filter/arguments are currently broken (you could set a view up with them but it won't work), and they get fixed by the patch. Nothing that's not already broken will get broken.
If that understanding is correct, then it's a shame about stale default configuration, but seems like an OK trade-off to fix the major bug.
The update hook here does look too heavy to do in presave, the only way I can think to make it acceptable would be to do something like add a _2846614_updated key to views config entities, so it only runs once per view (and every time for default configuration). If this is going to break default config then that might be something to consider, but if it's not then adding that complexity feels worse than some potentially stale (but not broken) exported views.
Comment #95
effulgentsia CreditAttribution: effulgentsia at Acquia commentedI don't think that's true. I don't see how the config in field handlers is stale. It seems to me to be actually used config. To demonstrate that, here's the same patch as #90, but with the change to
views.view.test_entity_multivalue_basefield.yml
reverted. Predictably, it breaksEntityViewsWithMultivalueBasefieldTest
. AFAICT, this means that if we commit #90 as-is, then every contrib module's default Views with perfectly functional field displays of multivalued base fields will start breaking.Comment #96
effulgentsia CreditAttribution: effulgentsia at Acquia commentedAnother concern I have with the approach in this issue. Specifically, the IS's proposed resolution of:
This will then tie default config to storage assumptions. For example, suppose module A ships a default View exported from a site where a given base field is single valued. But now, suppose module B implements
hook_entity_base_field_info_alter()
to change the cardinality of that base field. With #90, doing this means that module A's default View is now broken for a site with module B enabled.I don't know if this is already a pre-existing condition elsewhere in Views config structure.
Comment #97
effulgentsia CreditAttribution: effulgentsia at Acquia commentedI completely sympathize with your frustration. So I thought more about what I wrote in #92: are we being too strict in our BC policy with respect to default config, specifically the part about "References to plugin IDs and settings in default configuration can be relied upon however.".
However, no, I think we're right to be that strict about that. Because everything else that we consider @internal is stuff that it should be possible to write a contrib module without using. But the canonical way for a module to provide default config is via default config, so I think we must consider plugin IDs and settings referenced by default config to be public API.
That would satisfy the BC requirement. And also #96. However, I still wonder why we want to make arguments/filters/sorts reference the db column name. Instead of changing the config, could we instead make the code expect the entity field name, and figure out the column name at runtime? If that's an expensive determination, can we cache it?
Comment #99
damiankloip CreditAttribution: damiankloip at Acquia commentedHow do you account for both instances in that case? Take a look at UserViewsData,
$data['user__roles']['roles_target_id']
and$data['user__roles']
and everywhere else - this is adding 'schema' knowledge to the views data, which is why that code works. It is really only needed due to this bug, so there are currently duplicate entries for user data, for the roles field - one broken and one working (added to work around this bug originally I guess). So there will likely be many many views using that configuration. There are also tons of other places that actual table and column names are used and needed.I don't think so, this would already break AFAICT. The code in EntityViewsData also tries to work out the table names based on the field table mappings. So your table would already change if you did that anyway. Your views field would move to a different table already. The same code also mimics the table mapping column name based on whether it's a multi value field or not too!
$views_field_name = ($multiple) ? $field_name . '__' . $field_column_name : $field_name;
So I guess I don't really get what you're getting at with that angle, sorry :/
Another potential option (which me and dawehner weren't really keen on originally) is to add a 'real field' key to every multi value field accross all views data. That would keep the current, incorrect, inconsistent column keys.
Comment #100
Wim LeersThanks, @damiankloip!
Back to RTBC for feedback from @effulgentsia.
Comment #101
jibranWhile we are waiting for @effulgentsia we can remove the subsystem and release manager tags.
Comment #102
jibran@damiankloip is actively involved in it so removing 'subsystem maintainer review' tag. @catch would you like to remove the 'release manager review' tag?
Comment #103
catchYes I'm happy with this patch so I think we just need effulgentsia to come back on it at this point.
Comment #104
jibranThanks! catch. Let's assign it to him.
Comment #105
Wim LeersThanks catch!
Comment #107
xjmWell, meanwhile, this also needs a change record if there is a BC break.
Comment #108
xjmSince @effulgentsia hasn't had a chance to provide feedback in a couple months, and since @catch said he was comfortable with it, we can unassign from @effulgentsia. Marking NW for the CR though.
Comment #109
xjmAlso, can we reattach the correct patch and a test-only patch while we're at it? Something is weird with it in the IS.
One small thing to fix while I'm here:
We should almost never refer to the issue in a code comment; instead, the code comment should sufficiently and succinctly explain the bug.
Comment #110
damiankloip CreditAttribution: damiankloip at Acquia commentedDoing this today!
Comment #111
damiankloip CreditAttribution: damiankloip at Acquia commentedComment #112
damiankloip CreditAttribution: damiankloip at Acquia commentedCR: https://www.drupal.org/node/2900684
Also, updated patch to move update hook to
8500
Comment #113
jibranRemoving the tags now. CN looks good to me. Back to RTBC.
I found
dynamic_entity_reference_entity_test_views_data
added in #2548395-35: Provide views relationships for DER base fields providing field data for DER basefields I added that before #2477899: Multiple valued Base fields won't work in Views and never got around to removing that. I had the correct mapping all along and hopefully, I can remove that hook once this is committed.Just minor nit. It can fixed on commit.
s/8300/8500
Comment #114
Wim LeersAgreed with re-RTBC'ing.
Comment #116
tacituseu CreditAttribution: tacituseu commentedNeeds a re-roll after #2901739: Fix 'Squiz.Arrays.ArrayDeclaration' coding standard.
Edited: confused myself with #112.
Comment #117
Wim LeersThis patch was RTBC for two months and one day, and now it needs to be rerolled that? 😢
Comment #118
tstoecklerGit was able to rebase this without any interaction so going straight back to RTBC. Bot will set me straight, in case anything went wrong.
Comment #119
catcheffulgentsia had some remaining concerns that this could break previous working views, but those concerns aren't expressed on this issue and I think the rest of us are pretty convinced that this shouldn't happen. Also that theoretical possibility shouldn't completely override fixing definite currently broken views/making working views possible.
So I'm planning to commit this to 8.5.x this week, but not to 8.4.x to give time to flush out any such issues.
Tagging for release notes for the same reason.
Comment #120
damiankloip CreditAttribution: damiankloip at Acquia commentedThanks catch, sounds like a sensible approach on this!
Comment #121
catchCommitted 3b684e5 and pushed to 8.5.x. Thanks!
Comment #123
tacituseu CreditAttribution: tacituseu commentedComment/nit from #113 wasn't fixed.
Comment #125
catchAargh well spotted. Made an additional commit to fix that.
Comment #126
Wim LeersWoot, this finally unblocked #2543726 — see #2543726-150: Make $term->parent behave like any other entity reference field, to fix REST and Migrate support and de-customize its Views integration!
Comment #128
BeakerboyAs an aside, Patch #95 works on my 8.4.2 installation