Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Ignore this one for reviews, this is just to post new periodic patches for the conversion of field api to cmi, now starting with adding more properties to the config entity objects. Not for the faint of heart ... This happens also in the sandbox, but in a separate branch - http://drupalcode.org/sandbox/yched/1736366.git/shortlog/refs/heads/fiel...
Comment | File | Size | Author |
---|---|---|---|
#165 | field_CMI_BC-1906218-165.patch | 215.24 KB | yched |
#163 | field_CMI_BC-1906218-163.patch | 214.71 KB | yched |
#161 | field_CMI_BC-1906218-161.patch | 214.67 KB | yched |
#160 | field_CMI_BC-1906218-160.patch | 214.6 KB | yched |
#157 | field_CMI_BC-1906218-157.patch | 195.03 KB | yched |
Comments
Comment #1
swentel CreditAttribution: swentel commentedComment #3
swentel CreditAttribution: swentel commentedCommon bot, login ! :)
Comment #5
swentel CreditAttribution: swentel commentedShould fix lot of tests, including upgrade path.
Comment #7
swentel CreditAttribution: swentel commentedFixes fatal error in Field UI - more tests free to run - but potentially more exceptions ;)
Comment #9
swentel CreditAttribution: swentel commentedMore fixes
Comment #11
swentel CreditAttribution: swentel commentedFixes in entity reference and forum
Comment #13
swentel CreditAttribution: swentel commentedField deletion works fine now too
Comment #14
swentel CreditAttribution: swentel commentedWrong patch
Comment #16
swentel CreditAttribution: swentel commentedMore test fixing - moved some properties from 'data' to top level of entity.
Comment #18
swentel CreditAttribution: swentel commentedShould fix the upgrade tests
Comment #20
swentel CreditAttribution: swentel commentedShould fix the imagedisplay tests - debugging on the others now
Comment #22
swentel CreditAttribution: swentel commentedFixes more translation tests and a logic fix in field_update_field()
Comment #24
swentel CreditAttribution: swentel commentedFixes options module tests - you have to see the interdiff for this one ....
Comment #26
swentel CreditAttribution: swentel commentedGod damn it, lost hours on those new notices - see http://drupal.org/node/1862758#comment-7035586
Comment #27
swentel CreditAttribution: swentel commentedDown to two failures
Comment #29
swentel CreditAttribution: swentel commentedBig chance this is going to be green - although I removed a variable that I introduced relatively early, not sure how that's going to react.
Comment #30
swentel CreditAttribution: swentel commentedAnd the patch
Comment #32
swentel CreditAttribution: swentel commentedTest 2 for green
Comment #33
swentel CreditAttribution: swentel commentedShould still be fine - changed $field->uuid to $field->id() which returns $this->uuid - still the most unique way for a field as we can delete them.
Comment #35
swentel CreditAttribution: swentel commentedBah
Comment #37
swentel CreditAttribution: swentel commentedLet's try this
Comment #39
swentel CreditAttribution: swentel commentedOh well - anyway lets see how tests behave with field instance config objects
Comment #41
swentel CreditAttribution: swentel commentedInstallation should work now
Comment #43
swentel CreditAttribution: swentel commentedremove key exceptions
Comment #45
swentel CreditAttribution: swentel commentedSimple fix in text_field_load() - frees up lots of tests - last of one today
Comment #47
swentel CreditAttribution: swentel commentedFix tons of notices - and with that tests
Comment #49
swentel CreditAttribution: swentel commentedMore fixes
Comment #51
swentel CreditAttribution: swentel commentedMore fixes
Comment #53
swentel CreditAttribution: swentel commentedShould fix the upgrade path
Comment #55
swentel CreditAttribution: swentel commentedShould fix a ton more tests
Comment #57
swentel CreditAttribution: swentel commentedGetting there
Comment #59
swentel CreditAttribution: swentel commentedLet's see
Comment #60
yched CreditAttribution: yched commentedNot loosing my hopes to get back to this early next week :-/
Is there really no way to keep the old $field['some_property'] syntax working ?
The BlocksNG patch was 400K and drove @xjm half crazy. And we're nearing 50% bigger here.
Comment #61
swentel CreditAttribution: swentel commentedYeah, I'm just trying to figure out how much this change has impact on the code base - and in a way there could be some major refactoring in more parts, that hasn't been even touched yet.
I'll implement arrayAccess on both entities in another branch to make the patch significantly smaller.
Comment #63
swentel CreditAttribution: swentel commentedRelying on the schema version isn't safe during upgrade.
Comment #65
swentel CreditAttribution: swentel commentedMore fixes
Comment #67
swentel CreditAttribution: swentel commentedShould be green ....
Comment #68
swentel CreditAttribution: swentel commentedHere's a diff on only field, field_ui, field_sql and file + a few new files (entities, storage controllers and test module) - it's still relatively big, but maybe a bit more sane to review.
Comment #69
swentel CreditAttribution: swentel commentedDatetime field has landed - not sure if this also affects the custom_block module, but we'll see. Once it's back green, I'll make a smaller diff than #68.
Comment #71
swentel CreditAttribution: swentel commentedOh damn unit test conversions
Comment #73
swentel CreditAttribution: swentel commentedUsing ArrayAccess because this is getting out of hand :)
Let's see what the bot thinks, because I might have missed some obvious reverts
Comment #75
swentel CreditAttribution: swentel commentedMissed some obvious onces, kill previous testrun, patch got smaller
Comment #77
swentel CreditAttribution: swentel commentedA couple of other misses, going to let it run for real now ..
Comment #79
swentel CreditAttribution: swentel commentedRe-renamed widget_settings to widget - what the hell was I thinking
Comment #81
swentel CreditAttribution: swentel commentedAnd now for real real
Comment #83
swentel CreditAttribution: swentel commentedI'm a moron
Comment #85
andypostCurrently most of broken tests are caused by mixing argument
This is a primary trouble. You converting array to object that does not have the method!!!
Any reason using uuid as ID? Also it's confusing because field_entity have id that supposed by DX to be used as $field->id()
I think this would lead to many errors in contrib
should be different order:
but makes no sense ... there's no relation
needs a blank line before php-doc
Comment #86
swentel CreditAttribution: swentel commentedActually it does have the method, so this should work fine.
As to uuid vs id, yeah it should be using $field->id() to begin with. I think I'm just finding out here whether id (which would be the field_name) is safe enough - needs more testing. However, in field land, the uuid is the safest and the unique thing as it's possible to have a deleted field with the same name, but maybe it's safe to use the field name as id, I think yched can confirm this or not.
Comment #87
andypost@swentel I seen more then one array to object conversions in path. Also tried locally to fix'em but suppose there's more.
Already object!
$field is array. So this wont work
This object does not have the method... so upgrade path broken
Comment #88
swentel CreditAttribution: swentel commentedYeah, there's a reason I said on IRC to wait before posting feedback as I knew there were problems ;) They work fine now locally, finishing the last ones, will post new tomorrow.
Comment #89
swentel CreditAttribution: swentel commentedI'm hitting a weird endless loop again somewhere, even though I can install fine ... uploading because patch is even smaller now.
Comment #91
swentel CreditAttribution: swentel commentedGreen on upgrade path - well at least one :)
Comment #93
yched CreditAttribution: yched commentedre: field_name / id / uuid :
Right, obviously there's one too much here.
So yeah, as @swentel said, field_name is unique amongst non-deleted fields, which is what most regular runtime operations need to deal with. The $field['id'] (a serial numeric id in D7) is the unique way to identify a field when considering deleted fields as well.
All config entities have a uuid in D8 anyway - and uuids are better than serial ids since they are deployable (i.e they can reliably be used for : "when importing a field CMI file, is it an update of an existing field, or a 'delete existing + create new' ?").
So we should ditch the serial id altogether and go with field_name & uuid.
I'm not sure what $field->id() should be, though - field_name ? uuid ?
I'd guess field_name (the id of an image style is its machine name, right ?), but I'm not fully up to speed on what ConfigEntities / CMI impose (or just settle as "reasonable DX expectancies") on that regard.
That would mean
$field['id'] --> $field->uuid,
$field['field_name'] --> $field->id()
Which, I agree, looks like fun mind twisting for people coming from D7, but is meaningful / not surprising when considered in the context of the other D8 ConfigEntities - which IMO is what we should be designing for.
Comment #94
yched CreditAttribution: yched commentedAlso, this scenario would mean "when looking a deleted + non-deleted fields, there can be several $field with the same id()", which is not great DX wise.
This strengthens my feeling that we should ultimately get rid of API functions that do "get me all fields, deleted + non-deleted".
Regular API functions return regular (non deleted) fields. If you need to do stuff on non deleted fields as well, you call a separate helper function. Those are different sets of data.
Doesn't need to be done in the initial "Field API / CMI" patch, of course, just saying.
Comment #95
alexpott+1 for get rid of API functions that do "get me all fields, deleted + non-deleted"
I agree 100%
Comment #96
swentel CreditAttribution: swentel commentedAlmost there - off until the weekend
Comment #98
swentel CreditAttribution: swentel commentedThis should be green.
- edit - yeah had some time on the train ;)
Comment #99
yched CreditAttribution: yched commentedStarted delving into this - I'm still at the periphery, slowly moving to the heart of it.
Merged latest 8.x, and pushed a couple fixes along the way:
1c96a19 Fix missing id -> uuid replacement
68c0b51 Update docs in field.api.php : IDs ->UUIDs where relevant
44d888c Upgrade path: typo in var name (only for deleted fields)
93815d0 Upgrade path: Fix seemingly broken logic around UUID generation and deleted fields / instances
Comment #100
yched CreditAttribution: yched commentedJust checking whether this change is still needed :
Comment #102
yched CreditAttribution: yched commentedMy bad. Reverted "1c96a19 Fix missing id -> uuid replacement" above.
Comment #103
yched CreditAttribution: yched commentedAnd re-checking the EntityManager::processDefinition part.
Comment #104
yched CreditAttribution: yched commentedOK then, pushed
e3099ce revert the fix for EntityManager::processDefinition(), not needed anymore
Comment #105
yched CreditAttribution: yched commentedJust checking whether the precautions in field_sql_storage_schema() are still needed.
Comment #107
yched CreditAttribution: yched commentedDoh - not
if (db_table_exists('field_config')) {
, dumbass :-/Interdiff with green #103
Comment #109
yched CreditAttribution: yched commentedOK. What about with just the
if (!defined('MAINTENANCE_MODE')) {
check ?Comment #111
swentel CreditAttribution: swentel commentedUsing EFQ for field_read_fields() - the interesting thing about field_read_fields() though is that it's called very often now due to the menu_link conversion, see #1931900: EFQ calls field_info_field() too liberally
Not committed yet - will wait until this is green - and if we really want it ?
Comment #113
swentel CreditAttribution: swentel commentedThis is probably better.
Comment #115
swentel CreditAttribution: swentel commentedOk here's the interdiff that won't generate exceptions - but bulk delete tests fail because of the way the tests are setup. So either we add the get_me_deleted_fields_function() (and refactor a lot more) or keep that for a follow up.
Comment #116
swentel CreditAttribution: swentel commentedAlso added a comment over at #1740378: Implement renames in the import cycle (Link to comment) regarding renames and deletions.
Comment #117
yched CreditAttribution: yched commented(merged latest 8.x in the field-configentity-BC branch)
Comment #118
yched CreditAttribution: yched commentedNow that they're green, merged the changes from my field-configentity-BC-yched branch (and deleted it)
Most notable changes:
- 41225f2 Remove schema info and duplicate storage_* entries from CMI files
Derived data (schema...) is accessible through methods on $field entities
- 711d8b7 Method for $field['storage']['details'] + static caches for "derived data" methods
- 3c312fd Do not export $field['deleted']
- fd482c9 Do not export $instance['deleted']
- ae730a5 Perfrmance: use raw config instead of instance entities for getFieldMap()
- 3b7501a Performance: only load needed fields and instances in getBundleInstances()
- 7a7547e Use $entity->delete() instead of entity_delete_multiple() in field_[CRUD]_*()
- 8a61b83 Hide test fails caused by purge on comment fields - EFQ doesn't know how to select comments by bundle, see #1845372-40: Deleting a field from a non-node entity bundle results in an Entity Field Query Exception
Comment #119
yched CreditAttribution: yched commentedWork in progress : Start moving field_[CUD]_(field|instance)() to save() / delete() methods.
The goal is to keep the old functions as dumb BC wrappers to minimize the patch size, but have the real Entity API working on $field and $instance objects.
Done :
- field_create_field(), field_update_field(), field_delete_field()
- field_delete_instance()
TODO :
- field_create_instance(), field_update_instance()
Comment #121
yched CreditAttribution: yched commentedShould fix most test fails.
Remaining fails in LocaleUninstallFrenchTest are triggered by field_sync_field_status() doing $field->save(), which now does a lot more stuff than just updating the config record.
The function should probably act by direct manipulation of the config() data, just like it writes directly to {field_config} rather than field_update_field() in D7/HEAD.
But I won't have the time to tackle this today :-)
Comment #123
swentel CreditAttribution: swentel commentedThe TextPlainUnitTest and SQLStorageTest pass on my machine - we might want to change the description of the test though.
Comment #124
yched CreditAttribution: yched commentedOK, this should fix the remaining fails (hopefully I didn't introduce new ones when rerolling)
Comment #125
yched CreditAttribution: yched commentedOK, pushed the corresponding changes :
5cf2032 field_delete_*() --> Entity API delete() methods
0d5b111 field_(create|update)_field() --> Entity API save() method
d682baa test fixes for FieldEntity::save()
0eb344f move container reads to Drupal::service()
Now, on to field_CU_instance() :-)
[edit: fixed commit links above, wrong commit ids somehow]
Comment #126
yched CreditAttribution: yched commentedStill very unpolished, but giving the bot something to chew on for tonight :-)
Comment #128
swentel CreditAttribution: swentel commentedQuickly talked something through with alexpott on IRC re: what to return in import* methods.
So, we know what todo. However, as yched pointed out yesterday already, there's a big chance our import hooks can actually go away, but we at least know what todo now :)
Comment #129
yched CreditAttribution: yched commentedShould fix the fails ?
Comment #131
yched CreditAttribution: yched commented#1883152: Field level access for EntityNG introduced to references to the old field_config* tables.
Discussing with @swentel in IRC, the FileFieldWidgetTest fails was apparently the resurgence of an über nasty bug with drupalPost and file uploads, triggered by random field names and their respective orders.
Thus, renamed all test file fields to explicit names rather than random strings, that are a pain to begin with...
Comment #132
yched CreditAttribution: yched commentedRefactored a bit, double checking with the bot.
Comment #133
yched CreditAttribution: yched commentedFun experiment : remove the field_name entry in CMI files (duplicates the id), filling it only for BC at runtime.
Comment #134
yched CreditAttribution: yched commentedHah, newly added hal.module wants to install field_config_* tables in its tests. OK, no biggie.
We'll see how the "remove the field_name entry in CMI files" patch in #133 does, for now pushed the changes relative to #132 :
1e79298 field_(create|update)_instance() -> Entity API save() method
3013d92 use 'id' also for the Field 'label' key
fec17cd remove import*() overrides in controllers
ba83901 test fixes
640461a minor refactor in Field::save()
020b290 rename widget plugin property
f4301c2 rename FieldEntity to Field
6fd11ca remove recent upstream references to field_config_* tables
Comment #135
yched CreditAttribution: yched commentedAnd :
8e12014 adjust update function for latest changes in the CMI files
Comment #136
yched CreditAttribution: yched commentedWith that - off to the main issue :-)
Comment #138
yched CreditAttribution: yched commentedTrying to cleanup / sanitize the use of field_name / field_id in the CMI files :
- field.field.*.yml:
remove 'field_name' (duplicates the 'id') - In the future, we'll be talking about the $field->id, like for any other ConfigEntity, not "the field name".
(that part was already done in patch #133 above)
- field.instance.*.yml:
rename 'field_id' to 'field_uuid' (it's a uuid)
remove 'field_name' (not strictly needed, we have the field_uuid, it's better; for humans, it's present in the file name and 'id' key)
- Adds BC handling of what the legacy "array" properties expect:
$field['field_name'] (is $field->id)
$field['id'] (is $field->uuid)
$instance['field_id'] (is $instance->field_uuid)
$instance->field_name is still there for now (at runtime, not in the CMI file). It should be renamed field_id for consistency, but that will probably have to wait after the old $instance['field_id'] have been cleaned from all the code base...
See what the bot has to say.
Interdiff is with the current field-configentity-BC branch.
Comment #140
podarok#138: field_CMI_BC-1906218-138.patch queued for re-testing.
Comment #142
swentel CreditAttribution: swentel commentedStore raw config in field.field.deleted and field.instance.deleted.
Comment #143
yched CreditAttribution: yched commented[edit: heh, crosspost - that one is a continuation of #138]
Still seems to have fails in upgrade tests, let's see about the rest.
Comment #144
yched CreditAttribution: yched commentedStupid me. Still forgot to update field_update_8002() for the new keys in the CMI files, no wonder upgrade tests fail.
They work locally now.
OK, I'll let the bot finish its run see if there are other fails.
Comment #145
yched CreditAttribution: yched commentedRegarding #142 :
Not sure about that part: we're not *deleting* new fields here, we're just updating the data.
If the field is deleted, it's because it was already taken from state() and already has 'deleted' => TRUE.
So we shouldn't be trying to read it back from CMI (will probably fail, it's not in there anymore).
Comment #146
swentel CreditAttribution: swentel commentedHmm right, good catch, so I just take the config compare with what is changed and then save - or would casting to an array work fine enough too ?
Comment #148
swentel CreditAttribution: swentel commentedOk, this is probably better. (for the field.field/instance.deleted raw config storage)
Comment #149
swentel CreditAttribution: swentel commentedAnd this should fix the cleanup in the config entities from #143
Comment #151
yched CreditAttribution: yched commentedre #148:
Ah, right - $field is a Field object, so $field->getExportProperties() + explicit reset of 'deleted' to TRUE.
Works, but is not too great.
I think it would make much more sense for field_sync_field_status() to work on raw data altogether (as it does in HEAD).
I.e. at the beginning of the function, get $fields not from field_read_fields() but from config_get_storage_names_with_prefix() + state()->get('field.field.deleted'). My bad for using field_read_fields() here.
[edit: this means the code in the function will need to be updated for the new raw CMI properties ('field_name' -> 'id', etc).
@swentel : I'll try to tackle that monday night if you don't beat me to it]
Comment #152
yched CreditAttribution: yched commentedOn the "other" patch :-) - thanks for #149 !
Fixed the last missing change in the update func ($config['field_id'] --> $config['field_uuid'] in the db_update('file_usage')), pushed to the BC branch, and rolled to a new patch in the main issue.
Comment #153
swentel CreditAttribution: swentel commentedUsing only raw config - it feels a little bit more convoluted to me, but that's maybe only me.
Comment #154
swentel CreditAttribution: swentel commentedSmall change in loop
Comment #155
yched CreditAttribution: yched commentedYes, reading collecting the definitions from config involves more LoC, but I still think it makes more sense for the function to operate at one single level - entities or raw config.
Minor nits:
$field_names --> $ids (or just inline the config_get_storage_names_with_prefix() call inside the foreach ?)
$name in foreach () --> $id
$id in foreach() (three of them) --> $uuid
Other than than, I think we're good.
Comment #156
yched CreditAttribution: yched commentedSorry, couldn't stop myself from doing a couple additional refactorings / cleanups...
Comment #157
yched CreditAttribution: yched commentedChecking why that comment_uninstall() change is needed.
(interdiff is with current field-configentity-BC branch)
Comment #159
swentel CreditAttribution: swentel commented#157: field_CMI_BC-1906218-157.patch queued for re-testing.
Comment #160
yched CreditAttribution: yched commentedMy laptop has troubles running upgrade tests :-/
Comment #161
yched CreditAttribution: yched commentedAnd so does the testbot...
Let's try this one.
Comment #163
yched CreditAttribution: yched commentedCan't remember whether that
if (!defined('MAINTENANCE_MODE')) {
check in field_sql_storage_schema() is really needed now...Comment #164.0
(not verified) CreditAttribution: commentedUpdated issue summary.
Comment #165
yched CreditAttribution: yched commentedCheck after reroll.
Comment #166
jibran#1735118-257: Convert Field API to CMI is committed.
Comment #167.0
(not verified) CreditAttribution: commentedUpdated issue summary.