Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Since #1847596: Remove Taxonomy term reference field in favor of Entity reference is not going anywhere, we need another way of grouping reference-like field types together. Let's start with the taxonomy term reference field and follow-up with file and image.
Comment | File | Size | Author |
---|---|---|---|
#53 | taxonomy-target-id-1965208-53.patch | 27.71 KB | amateescu |
#51 | taxonomy-target-id-1965208-51.patch | 27.38 KB | amateescu |
#46 | taxonomy-target-id-1965208-46.patch | 27.28 KB | amateescu |
#38 | taxonomy-target-id-1965208-37.patch | 27.35 KB | amateescu |
#33 | taxonomy-target-id-1965208-33.patch | 27.35 KB | amateescu |
Comments
Comment #1
amateescu CreditAttribution: amateescu commentedAnd here's the patch.
Comment #3
amateescu CreditAttribution: amateescu commentedThis should fix it.
Comment #4
BerdirThis should probably use the versioned, update-safe field api functions.
That's a tricky one, if not we, then who's going to do it? :)
The changes themself are simple enough and make sense I think. We'll save more code as we'll start to move additional things into there (validation, schema possibly, field API stuff as it will get merged with Entity Field API).
We should probably do the same for image and file items.
Comment #5
amateescu CreditAttribution: amateescu commentedYeah, I see that the Field API to CMI conversion didn't remove the data from {field_config} so I guess
_update_7000_field_read_fields()
should work at least until we get a 8000 version of it.The module that provides the alternate field storage? :/ Core can only do updates for the storage it provides and knows how to deal with..
Those are next, as I mentioned in the OP :)
Comment #6
BerdirYes, there are some follow-up field api issues about providing new upgrade path helpers and deciding when the upgrade should exactly happen. Fun stuff ;)
/me reads OP
Oh. ;)
Comment #8
amateescu CreditAttribution: amateescu commentedOk, so that won't fly. How about just a @todo for now?
Comment #10
Berdir#8: 1965208-8.patch queued for re-testing.
Comment #12
amateescu CreditAttribution: amateescu commentedRerolled and added a update requirement to be sure that we're running after field api has been converted to config.
Comment #14
BerdirMost of that conflicted due the term ng patch. Will probably still fail installation.
Comment #16
BerdirThis should fix that.
Comment #18
BerdirWeird. See interdiff. When those indexes were still there, the whole installation process just hang up within config_import_invoke_owner(), it comes until field.field.field_tags and then it just stops and doesn't continue.
@yched/@swentel, any clues as to what's going on there in terms of validation logic and pre/post save? This is.. interesting to debug ;)
Comment #19
BerdirDiscussed with @yched and it turns out that field api does throw an exception, I'm just not seeing it. When checking the ajax response with developer tools, it says "SQLSTATE[42000]: Syntax error or access violation: 1072 Key column 'field_tags_tid' doesn't exist in table: CREATE TABLE {field_data_field_tags} (" which is perfect.
Comment #20
BerdirRe-roll, conflicted in taxonomy_field_presave()
Comment #21
yched CreditAttribution: yched commentedSo, the conclusion in #1969662: Provide new _update_8000_*() CRUD helpers for Field and Instance definitions was that a specialized helper function for "read fields" would be of little interest as opposed to just iterating over raw config() data.
So the current plan is to *not* include helpers for "read existing field/instance definitions", since (unlike the "create" ops, that are a little more involved since they require pinging the storage backends) they would not really bring functionality specific to those Field API structures. If anything, a generic update-safe "EFQ on config records" could be useful for all config entities, but that's outside the scope of field API.
In short - maybe try to write this on top of raw config reads ? It doesn't seem it shouldn't be too hard.
Comment #22
amateescu CreditAttribution: amateescu commentedSomething like this then?
Also rerolled for #1985138: New autocomplete taxonomy terms not previewable.
Comment #23
BerdirCan't RTBC the upgrade path part but the other part of the patch looks fine to me. I did a few re-rolls but just fixed some tests. So if @yched or maybe @swentel can confirm the last interdiff, then this can be RTBC'd by them.
Comment #24
swentel CreditAttribution: swentel commentedI would do something like this:
$field = new Field(config($config_name)->get());
It's a pattern we're using in other places as well see http://drupalcode.org/project/drupal.git/blob/refs/heads/8.x:/core/modul...
This will also guarantee we don't have to touch this again when removing the BC layer.
Comment #25
amateescu CreditAttribution: amateescu commentedThat looks much better indeed.
Comment #26
amateescu CreditAttribution: amateescu commented@swentel pointed out in IRC that we need to update the indexes value in the field config too.
Comment #27
amateescu CreditAttribution: amateescu commented@swentel also wants to play nice with other modules that might have overriden those indexes :)
Comment #28
swentel CreditAttribution: swentel commentedSweet, thanks :)
Comment #29
Berdir#27: taxonomy-target-id-1965208-27.patch queued for re-testing.
Comment #31
Berdir#27: taxonomy-target-id-1965208-27.patch queued for re-testing.
Comment #33
amateescu CreditAttribution: amateescu commentedRerolled for #1620010: Move LANGUAGE constants to the Language class.
Comment #34
fago#33: taxonomy-target-id-1965208-33.patch queued for re-testing.
Comment #36
amateescu CreditAttribution: amateescu commentedI thought I linked this here before, but apparently not. Here's the FileItem/ImageItem equivalent of this patch: #1996714: Convert FileItem and ImageItem to extend EntityReferenceItem
Comment #38
amateescu CreditAttribution: amateescu commentedRerolled.
Comment #39
das-peter CreditAttribution: das-peter commentedLooks pretty good to me, couldn't find anything to nag about.
However, I'm not sure what's ok to change in the views code and what's not.
As far as I understand the following changes only affect "meta data" and are backward compatible because the
'base field'
still is'tid'
, right?Comment #40
amateescu CreditAttribution: amateescu commented'base field' is the taxonomy id property, and yes, that remains unchanged. The only thing this patch changes is the name of the taxonomy reference field column.
Back to rtbc per #28.
Comment #41
yched CreditAttribution: yched commentedNo need to wait for it, but when #1969728: Implement Field API "field types" as TypedData Plugins lands, this change will in practice mean "the 'taxonomy field type' plugin extends the "entity referrence field type' plugin", which makes very much sense.
In other words, +1.
Comment #42
fago#38: taxonomy-target-id-1965208-37.patch queued for re-testing.
Comment #43
Berdir#38: taxonomy-target-id-1965208-37.patch queued for re-testing.
Comment #44
moshe weitzman CreditAttribution: moshe weitzman commented#38: taxonomy-target-id-1965208-37.patch queued for re-testing.
Comment #46
amateescu CreditAttribution: amateescu commentedYAR. (yet another reroll)
Comment #48
Berdir#46: taxonomy-target-id-1965208-46.patch queued for re-testing.
Comment #49
swentel CreditAttribution: swentel commentedFinally!
Comment #50
alexpottNeeds another YAR...
Comment #51
amateescu CreditAttribution: amateescu commentedYARRR!
Comment #52
effulgentsia CreditAttribution: effulgentsia commentedI think this is crucial for rest.module to be able to handle exporting and importing of content with term reference fields (without writing new custom code for them), so raising to major.
Comment #53
amateescu CreditAttribution: amateescu commentedComment #54
Dries CreditAttribution: Dries commentedCommitted to 8.x. Thanks!
Comment #55
BerdirRemoving sprint tag.
Comment #57
yched CreditAttribution: yched commentedFeedback welcome on #2079517: Weird assignment of definition['settings']['target_type'] in FieldItem::getPropertyDefinitions() :-)