Sandbox :
http://drupalcode.org/sandbox/yched/1736366.git/shortlog/refs/heads/fiel...
This patch moves $field and $instance structs to CMI / ConfigEntities.
--> Sample CMI files :
field.field.[id].yml
field.instance.[entity_type].[bundle].[field_id].yml
Deleted fields & instances get out of the config folder, and are stored in state() until their data gets purged
(i.e. field deletion is deployed like everything else, by the fact that the file is not there anymore)
Patch comes with an update function, and test coverage for basic import operations.
In order to keep the patch size "reasonable", it includes a BC layer that keeps the existing APIs and structures working;
- keeps the old array syntax ($field['cardinality'], $instance['field_name']) fully working through ArrayAccess
- keeps the old field_CRUD_(field|instance)() functions working, on top of the new ConfigEntity way, that also fully works:
// Create
entity_create('field_instance', array(
'field_name' => 'field_foo',
'entity_type' => 'node',
'bundle' => 'article',
'required' => TRUE,
))
->save();
// Update
$instance->label = 'Some label';
$instance->save();
// Delete
$instance->delete();
There's a basic dump available to also test the upgrade path: http://dl.dropbox.com/u/15116672/d7-fields-cmi-upgrade-test.zip
The following issues have been identified as blockers or problems that need to be solved:
- #1740378: Implement renames in the import cycle (to handle field renames)
Fields and instances now have an uuid which is a string. However, the file_managed table id column is an int. This breaks for default imagesFixed in the patch by changing the file_managed.id column to a varchar
Follow ups
- #1953404: Add config schema to field and instance config entities
- #1953408: Remove ArrayAccess BC layer from field config entities
- #1953410: [Meta] Remove field_create_*(), field_update_*() and field_delete_*() in favor of just using the ConfigEntity API
- #1953414: Move logic of field_read_fields() and field_read_instances() to the storage controller.
- #1969662: Provide new _update_8000_*() CRUD helpers for Field and Instance definitions
- #1953418: Run "Field API : CMI" upgrade as early as possible.
- #1960574: Add more tests for Field config import
- #1966008: Decide which field and instance properties should be public/protected
- #1966998: Add a constant OR use TRUE/FALSE for active storage vs. inactive storage and other properties
- #1967106: FieldInstance::getField() method
- #1968960: Improve inline documentation in Field::save() when updating field storage information
- #1968986: Document expected inputs for Field and FieldInstance constructors
- #1968996: Add protected saveNew() and saveUpdated() methods to Field and FieldInstance to improve code readability
- #1969032: More sensible way of defining field default values
- #1969072: Clarify documentation around field storage and schema in Field::save()
- #1969136: Move field_reserved_columns() to a static method
Related configuration issues
- #1944368: React to / force order of imports
- #1942346: Convert Field API variables to CMI
- #1953528: Store 'per-bundle view mode settings' in CMI, get rid of field_bundle_settings() & its variable
- #1875992: Add EntityFormDisplay objects for entity forms
- #1961684: ConfigEntityStorage::import[Create|Change|Delete]() type hint on Config class
- #1653026: [META] Use properly typed values in module configuration
- #1959610: Remove public properties from entity classes
- #1969698: ConfigEntity::save() should disallow saving ID/UUID conflicts (Field UUID changes can badly corrupt field data)
Related
Comment | File | Size | Author |
---|---|---|---|
#255 | field_CMI-1735118-255.patch | 235.37 KB | yched |
#255 | interdiff.txt | 1.48 KB | yched |
#254 | field-cmi-1735118-254.patch | 235.26 KB | xjm |
#254 | field-name-interdiff.txt | 846 bytes | xjm |
#250 | 1735118-250.patch | 235.17 KB | swentel |
Comments
Comment #1
swentel CreditAttribution: swentel commentedAdding patch - it will absolutely not turn green ever as field deletions need to be converted as well.
Comment #3
chx CreditAttribution: chx commentedIMO field IDs are used so that when you delete a field and
re-create itcreate another with the same name the system doesn't get confused.Comment #4
yched CreditAttribution: yched commentedFirst off : sweet jesus ! Major yay, thanks @swentel for kicking this off.
We'll discuss the details in the Munich sprint this week-end, but in short :
- yes, field ids are there to handle the case of "I delete field_foo and want to recreate it straight ahead without waiting for the data to be purged".
In the context of CMI, they will also be used on config deployment to sort out : "oh, field.foo.yml has changed, is it an update or a delete / create new ?"
Of course, they will need to be UUIDs now to avoid clashing between site instances - and with {field_config} table gone now, we no longer have an autoincrement column to generate serial ids anyway...
- Discussed and pretty much agreed with swentel on IRC (and with @heyrocker in BADcamp last year...) :
stuff like 'module', 'is_active'... are not part of the actual config, and are not up for a site admin to manually edit : they are more "state / derived info". We'll probably want to strip them out of the config files, but then we need to store this 'state' info somewhere else, in the db.
Related :
#1175054: Add a storage (API) for persistent non-configuration state
#1202336: Add a key/value store API
Comment #5
chx CreditAttribution: chx commentedYou might want to create a sandbox, create a branch, apply those two patches, create another branch based on that and then your patch can be rolled as the diff of two branches.
Comment #6
yched CreditAttribution: yched commentedSandbox created : http://drupal.org/node/1736366
(and added swentel as admin for now, I'll grant access to other sprinters tomorrow)
No code in there yet. I'll upload my own "plugins-field-api" branch over there asap (cuurently in the "D8 blocks" sandbox, where plugins work happened).
Comment #7
swentel CreditAttribution: swentel commentedField tests turn green, rerunning to see what the rest says.
Comment #9
swentel CreditAttribution: swentel commentedExpected that, this should fix the installation
Comment #11
tim.plunkett#943772: field_delete_field() and others fail for inactive fields implies that forum_enable()'s call to field_associate_fields() is unnecessary. Uploading the last patch but removing that part.
Comment #13
larowlanThis should fix the failing image field default value test, but does it break something else....
Changes the id field in file_usage to a varchar (128) to allow for field ids, which are now UUID's and not serials.
Comment #15
larowlanThis should fix the node type failures for when the bundle name was changed.
field_attach_rename_bundle() was calling field_create_instance with the old field config (new bundle name) but this instance had key elements of the config such as label nested in $config_instance['data']. So the 'Body' field label became 'body', 'Image' became 'field_image'.
Comment #17
larowlanLast patch went backwards in terms of test-fails, issues with file field widget.
Attempting alternate approach.
Comment #19
larowlanOk, there are 35 failed tests in the File Field Widget Test but these tests pass locally - any ideas?
Comment #20
larowlan#13: field-api-to-cmi-1735118.13.patch queued for re-testing.
Comment #21
larowlan#11: drupal-1735118-11.patch queued for re-testing.
Comment #22
larowlanYep, something changed upstream.
Patch #11 was 218 fails, after re-testing - now 245
Patch #13 was 215 fails, after re-testing - now 223
Patch 13 is the best at this stage.
Comment #23
vlad.ilyich CreditAttribution: vlad.ilyich commented.
Comment #24
Anonymous (not verified) CreditAttribution: Anonymous commentedtried to look at those notices that appear when comment_uninstall() is called in ModuleApiTest::testDependencyResolution().
thing is, i don't know how HEAD should work. HEAD comment_uninstall() does:
those field_attach_delete_bundle() calls never delete any instances, because we do this in field_read_instances():
that condition means we'll never get to any instance which has its field set with delete = 1, which is exactly what comment_uninstall does. same for D7 - comment_body field_config_instance rows don't have delete set to 1 when uninstalling comment module.
i have NFI if this is the expected behaviour. i looked at it because with the patch, we change how we read field and instance info, so we start actually running field_delete_instance() in field_attach_delete_bundle(). this hits random bugs and get a million notices. but, what is the desired behaviour? reading the code for HEAD, i have NFI, so i don't know how to fix the issues in the patch.
Comment #25
swentel CreditAttribution: swentel commentedHey all, thanks for working on this! We indeed know about the issue of file_managed. We're going to discuss that more today.
Also, we have another issue and a sandbox as well (I know, not ideal) where try to discuss more and use this just to run the testbot.
We're going to start pushing to branch as well today in the sandbox.
That issue is over at #1738284: Field API CMI conversion
Comment #25.0
Anonymous (not verified) CreditAttribution: Anonymous commentedupdate so that others don't waste their time on this issue
Comment #25.1
gddUpdated issue summary.
Comment #26
alexpottI've created a branch 1735118-field-cmi-swentel in http://drupal.org/sandbox/yched/1736366 - see http://drupalcode.org/sandbox/yched/1736366.git/shortlog/refs/heads/1735...
Comment #27
alexpottThe previous patches used a 'conf' key that is unnecessary. Patch attached fixes this. This patch has been created from http://drupal.org/sandbox/yched/1736366 so contains a whole load more of Swentel's great work :)
Comment #27.0
(not verified) CreditAttribution: commentedAdded default images
Comment #29
swentel CreditAttribution: swentel commentedThe cool thing about driving home for 8 hours is that you can let the tests run on your laptop in the meantime ;)
I pushed my latest changes to the sandbox branch, which now contains an upgrade path and I added 3 more remarks in the summary above.
Let's see if our testbot is happy, my local machine was. I'm doing mostly client work today, but I'm on irc for questions (although it could be that I'm heavily distracted)
Comment #30
webchick"The cool thing about driving home for 8 hours is that you can let the tests run on your laptop in the meantime ;)"
LOL you are hardcore, dude. :) Yay! Thanks!
Comment #32
swentel CreditAttribution: swentel commentedAh man, forgot the push some stashes, I'll do that tomorrow.
Comment #33
yched CreditAttribution: yched commentedPunting for swentel's latest changes to be pushed.
Meanwhile, #1739900: Add a rename operation to config storage controllers got in, which means field_attach_rename_bundle() should probably move over to "rename() / update the 'bundle' value" rather than "delete old file" / "save new file".
Also, that function is currently a mix of "access to $instance definitions through field_read_instances() API calls vs. direct config() calls". Since the previous code worked by direct updates on the field_config_instances table, switching to direct config() calls if possible would be more consistent.
Comment #34
swentel CreditAttribution: swentel commentedPushed everything, here goes.
Comment #35
swentel CreditAttribution: swentel commentedAnd with the patch ...
Comment #36
gddIts 3:00 am right now but here are some comments based on a quick scan.
We have config_rename() committed now so we can clean this up a little.
I'm hoping that we can get #1175054: Add a storage (API) for persistent non-configuration state and #1202336: Add a key/value store API moving pretty quickly, and that's where this would go. In the meantime I'd probably just prefer to keep using variables for state stuff. That's what we'll have to use if the above patches don't land.
I asked swentel about this and it has to do with a field length issue. Could probably use a slightly more descriptive comment. There are a couple other places we do this and I'd almost rather have it in a helper function even thought it would only be two lines or something.
<3
I guess for me the big question here is whether or not we do the configurables conversion before we commit this, and from my standpoint it depends on how long that conversion will take. The longer we wait, the less time we have to start testing import flows and finding the potential dependency problems and that's a big priority. With swentel getting 15 days to work on this maybe we can crank it out pretty quick, which is obviously preferable.
It's *really* awesome seeing this take shape. You all rock.
Comment #37
gddComment #38
cosmicdreams CreditAttribution: cosmicdreams commentedI will attempt to itemize every file that would be effected by this change tonight.
Comment #39
cosmicdreams CreditAttribution: cosmicdreams commentedcore/modules/field/field.attach.inc
core/modules/field/field.crud.inc
core/modules/field/field.install
core/modules/field/field.module
core/modules/field/lib/Drupal/field/Tests/CrudTest.php
core/modules/field/lib/Drupal/field/Tests/FieldInfoTest.php
core/modules/field/lib/Drupal/field/Tests/FieldInstanceCrudTest.php
core/modules/field/lib/Drupal/field/modules/field_sql_storage/field_sql_storage.install
core/scripts/generate-d7-content.sh
Nowhere else in drupal can the string field_config be found. All in all, 42 usages of "field_config" was found.
Comment #40
swentel CreditAttribution: swentel commentedWorking on getting rid of the extremely ugly hacks now re: the deletion of fields/instances.
Comment #41
swentel CreditAttribution: swentel commentedOk, new patch. Changes:
Comment #41.0
swentel CreditAttribution: swentel commentedUpdated issue summary.
Comment #42
Anonymous (not verified) CreditAttribution: Anonymous commentedquick review, overall looking really good.
- maybe something like this for exploding on the config name:
- there's lots of variations on this:
we're stuck with caring about filesystem crap in our apis, but we're not enforcing the length of generated names anywhere. maybe this can be a follow up, but we need to create it before committing the patch, because this will lead to strange intermittent failures as the combination of these dynamic fields exceeds filename lengths.
- clarified with swentel that this comment "@todo we should try and identify the fields that only need to be updated." means only update fields that have changed. possible implementation for that below:
- references to 'row' needs to be updated in tests and elsewhere
Comment #43
chx CreditAttribution: chx commentedi was under the impression that md5 has been banned from core. It doesn't make any sense, yes. Still. There were people who have removed two-way compatibility in our password framework just to please the US government. So. It's banned.
Comment #44
aspilicious CreditAttribution: aspilicious commentedIt is banned for security reasons but also for just encoding stuff?
Comment #45
chx CreditAttribution: chx commentedDoesn't matter. Wish it did. It's just banned.
Edit: see #723802: convert to sha-256 and hmac from md5 and sha1 for some random government's standards trumping proper core process :(
Comment #46
aspilicious CreditAttribution: aspilicious commentedSo what function can we use to just encode stuff (like needed in this issue)
Comment #47
effulgentsia CreditAttribution: effulgentsia commentedhash('sha256', $string_to_encode)
Comment #48
KarenS CreditAttribution: KarenS commentedSee #790520: Does the hashing of context array need to switch from md5()? for ideas about workarounds for MD5.
Comment #49
swentel CreditAttribution: swentel commentedNew patch, changes:
Comment #49.0
swentel CreditAttribution: swentel commentedRemove the 'rename' and 'field_system_info_alter' hack blockers'
Comment #53
chx CreditAttribution: chx commentedFirst thanks a ton for this work. I am a little surprised the issue does not even entertain the thought whether these should be ConfigEntities. Perhaps as a followup? Because this one looks quite close, congratulations. A few things surprising me:
Is 6 enough? How did we end up with 6? I do not get most of this, why is 32 too long? I thought table names had a 64 char limit?
Why are you using module_load_include to load field.module? Just two lines below this, you are using drupal_load.
Likely not in this issue but requires a followup, for example user pictures definitely will need a change. You can of course store an integer in a string column and even join on it just it's not optimal.
Most importantly:
+ // @todo no idea yet why this suddenly becomes lowercase, should be fixed in field api in field_sql_storage_field_attach_rename_bundle().
instead of changing the test please comment it out and open a critical task as a followup to uncomment it. That's what we agreed on for these kind of problems.Comment #54
xjmSo I talk to chx about this on IRC, but I'll document what I said here just for the record: Can we try to fix these in this issue? My idea was that the patch's @todos about tests were part of the work in progress here (and in the sandbox). I'm not sure it's a good precedent to set, especially when making such a major (awesome) change, and I think we should be really cautious about going the route of spawning followup tasks yet.
@swentel, thanks for doing such a great job of documenting the difficult points in the patch! It's possible you've uncovered a number of different bugs here.
Comment #55
swentel CreditAttribution: swentel commentedI actually found out what goes wrong with that test, will be easily fixed.
I also investigated the failures on the comment vs forum problems and that is most likely been handled over at #1264756: Inline field purge within field_delete_field() - will know for sure now that
that the key/value api is in so I can remove the utter most cruel hack that is still in the patch. I'll be working on this the next days, new patch will be around next week, also addressing the questions and comments in #53.
Comment #56
swentel CreditAttribution: swentel commentedChecking for another test bot run:
- chasing head after all the awesome commits.
- the allowed values hack is gone now that #1785560: Remove the sorting of configuration keys is in.
- the nodeTypeTest.php as well, thanks to the _field_info_field_collate() patch.
Should be green, tackling the deletion of fields is next.
Comment #58
swentel CreditAttribution: swentel commentedNo clue why ModulesDisabledUpgradePathTest fails on the bot, works fine on my local machine, patch attached fixes the rest though.
Comment #59
swentel CreditAttribution: swentel commentedmeh, nice going.
Comment #61
swentel CreditAttribution: swentel commentedSo both failures don't happen on my local machine, no clue what's going on here. Anyway, focusing on the deleting of fields now.
Comment #62
swentel CreditAttribution: swentel commentedSo, let's see what this one does:
Upgrade tests and field api tests are green on my machine, I have a good feeling the rest will follow.
Comment #64
cosmicdreams CreditAttribution: cosmicdreams commentedIt looks like the exceptions in other tests are related to some stale code that is either in the EnableDisableTest or related to it.
Comment #65
swentel CreditAttribution: swentel commentedThis was fun to debug, especially since I had a typo in the patch since august 24 ... the call to field_read_fields() in
field_sync_field_status()
was done with included_deleted instead of include_deleted.So, next try, no ugly hacks anymore, only one param addition in field_attach_delete_bundle() so the comment uninstall can work nicely.
The patch also adds upgrade path tests for the conversion (maybe we can add others later for Field API too), located in core/modules/system/lib/Drupal/system/Tests/Upgrade/FieldUpgradePathTest.php for testing
field_update_8001()
.Comment #67
swentel CreditAttribution: swentel commentedForgot to add the cardinality while reading the instance - the scandirectory fail is probably a glitch as that really has nothing todo with field api at all :)
Comment #69
swentel CreditAttribution: swentel commentedPosting to mainly chase head and fuhby has something todo on the plane :)
The file field widget still randomly fails. I have dump of 10 failing and 10 passing tests locally which we'll investigate starting from tomorrow.
Comment #70
swentel CreditAttribution: swentel commentedMore storage cleanup - no review though, still fails on the same test.
Comment #71
swentel CreditAttribution: swentel commentedOk, the failing test in File field widget has been found after 10 hours of debugging by alexpott - this is one of the most legendary debugging sessions and simpletest bug ever seen that will be told over and over again. It's too long to describe, but here's a hint for those who want to try and find out what the problem is :) https://twitter.com/swentel/status/263991937906913281/photo/1/large
Other changes:
- deleted instances are kept in state now as well
- some cleanups
Anyway, this should be green.
Comment #74
alexpottFirst effort at convert fields to configentities...
This is by no means complete but is an effort to prove that we can overcome some of the "inception" issues.
Comment #74.0
(not verified) CreditAttribution: commentedAdd link to 1785560
Comment #76
alexpottHere you go... testbot test. :)
Comment #77
alexpottSo... thank you testbot!
There are a couple of issues that have been highlighted by this conversion. Having
drupal_get_schema()
depend onentity_get_info()
is messy to say the least. (It's dependent becausefield_sql_storage_schema()
usesfield_read_fields()
to build the schemas)One of the really tricky issues is due to this code in
module_enable()
:Due to this code during the
drupal_get_schema()
callhook_entity_info_alter()
's are fired before modules are enabled. That's why the patch does:There other tricky thing about the patch is that field_read_fields is not safe to use before
field_update_8001()
has run. Hence we have the checks infield_system_info_alter()
andfield_sql_storage_schema()
.This patch is by no means complete - at the moment all the field/bundle/instance data is still stored in a "data" property on the config entity.
Comment #78
alexpottOne more thing....
This is called FieldEntity and not Field since that (unintentionally) caused
hook_field_load()
,hook_field_save()
, hook_field_etc... to fire when working with the new Config Entity.I don't like the name FieldEntity but it was expedient.
Comment #78.0
alexpott#1785560: Remove the sorting of configuration keys & #1175054: Add a storage (API) for persistent non-configuration state are implemented
Comment #79
andypostI think having a good class diagram in issue summary will help a lot for reviewers. Having all this as entities anyway will fire a hooks so maybe it's ok to have field_entity and field_instance as names
Suppose we need to decide on naming because Bundle looks ugly and field_entity better naming.
Also I think field instance is just a kind of derivative from field plugin.
Bundle is used by Symfony and core and will confuse DX
We have field_test module and field so any reason to make another one?
We are trying to get rid of this table in #1552396: Convert vocabularies into configuration
EDIT There's some changes in config entity was introduced in #1798944: Convert config_test entity forms to EntityFormController
Comment #79.0
andypostAdding Entity translation UI problem.
Comment #80
swentel CreditAttribution: swentel commentedChasing head as the Entity translation UI broke the installation of itself and the config import UI broke the import tests.
I also added an additional create test. Let's hope it still stays green.
- edit - #1832932: translation_entity_entity_insert() assumes entity IDs are integers is in so we can remove that in our code
Comment #81
swentel CreditAttribution: swentel commentedSo with the manifest file we could maybe easily speed up field_read_fields() and field_read_instances() by first reading in the manifest file and then only loading the field/instances if it matches some parameters. I quickly read #1826602: Allow all configuration entities to be enabled/disabled that will add enabled/disabled (which wil help us for the status of the Bundles/bundle settings that I'll move out soon in favor of #367498: Introduce 'display' as a way to group and reuse instance and formatter settings.), and it seems that adding custom properties might not work without losing them, or am I wrong there ?
- edit - wrong issue number re: display object
Comment #82
sunSorry, but no, the manifest file is not to be abused as an config object index or lookup facility.
Comment #83
yched CreditAttribution: yched commented@alexpott, @swentel : Added a note on why moving bundles out of entity_info is critical, in #1822458: Move dynamic parts (view modes, bundles) out of entity_info(), #6 and #7.
Comment #84
swentel CreditAttribution: swentel commentedNew patch with a suggestion from alexpott to not call field_read_fields() in field_sql_storage_schema(). I also attached the interdiff of the changes between that patch and this one to see how much 'hacks' this removes.
The Bundle entity has also been removed as that will be handled in another patch, see #81
Comment #86
moshe weitzman CreditAttribution: moshe weitzman commentedThis looks like a fantastic improvement. I really hope folks can finish this up and get to RTBC. There is some commented out code and @fieldcmi markers that need cleaning. Otherwise, the code looks ready to me, and features a bunch of new and updated tests.
There are Field changes which are disallowed like changing storage details for an instance. It would be great if CMI offerred a validate phase so that modules could reject proposed config changes. I might throw up a patch for this.
Wrong Doxygen
Comment #86.0
moshe weitzman CreditAttribution: moshe weitzman commentedWe are converting to config entity already in the patch
Comment #87
swentel CreditAttribution: swentel commentedThis should fix the 2 failures - skipped rest services for fields and instances for now and added one small hack back in EntityManager.php (strangely enough, I can't seem to reproduce the failing comment test manually)
Will cleanup the references to @fieldcmi and making the entities more sane too later this day/weekend.
Comment #88
swentel CreditAttribution: swentel commentedChasing HEAD, especially with the user picture conversion. Removed all references to '@fielcmi' as well.
Comment #90
mbrett5062 CreditAttribution: mbrett5062 commentedNot sure if this is relevant, but in all other plugin definitions, the id uses '_' to separate the name. This patch has 'fieldentity', it looks plain ugly to read, could/should we make this 'field_entity'?
Comment #91
yched CreditAttribution: yched commentedRight, but I'd vote for 'field' & 'field_instance'.
Comment #92
mbrett5062 CreditAttribution: mbrett5062 commentedSounds OK to me, and yes, just 'instance' could well conflict with later implementations.
So either 'field' and 'field_instance' or 'field_entity' and 'field_entity_instance'. Either would be better then current 'fieldentity' and 'instance'.
Comment #93
Berdirfield and field_instance sounds good to me as well, +1.
Comment #94
swentel CreditAttribution: swentel commentedfield would be nice yes, but that currently clashes with hook_field_load, etc, see #78
Comment #95
mbrett5062 CreditAttribution: mbrett5062 commented@swentel, I see where your coming from, so 'field_entity' and 'field_entity_instance', would they be OK. It does follow current convention and is more readable.
Comment #96
yched CreditAttribution: yched commented@swentel: ah crap, of course, forgot about that. Ok, then naming is not too important right now, we can clean that up when hook_field_load() gets moved to a method. Meanwhile, though, would be good to add a comment stating the reason for those temporary names, because this will trip more people.
Comment #96.0
yched CreditAttribution: yched commentedEntity UI translation problem is gone.
Comment #97
chx CreditAttribution: chx commentedWhat happened to this issue...?
Comment #98
swentel CreditAttribution: swentel commentedWe're still on it, don't worry :) See field-configentity-swentel branch in yched's sandbox. Currently stuck again on upgrade path, the most fun thing in Drupal world.
Comment #99
swentel CreditAttribution: swentel commentedLet's see how this one turns out, the upgrade path should be working - sorry for not having an interdiff.
Comment #101
swentel CreditAttribution: swentel commentedShould be green - unless one of the latest commits adds new fails
Interdiff misses the two lines that dissappear in FieldItemUnitTestBase that fix all tests extending on this class.
Comment #102
swentel CreditAttribution: swentel commentedGreen - trying a new patch removing all the hacks and checks - although berdir reported that he needed those while running an upgrade on a big site - or is there a difference between testbot and manual upgrading ?
Comment #102.0
swentel CreditAttribution: swentel commentedUpdated issue summary.
Comment #103.0
(not verified) CreditAttribution: commentedUpdated issue summary.
Comment #104
sun1) I've the impression that the top-level property should be $field_name instead of $name?
2) But, isn't $id always the same as $field_name?
We definitely need to document these architectural details on the entity class properties.
Also, the Field class currently documents $id as the "config name" - but that term generally refers to the full/complete name of the config object; e.g., "field.field.field_test_import" in this case, which is not the intended meaning. It seems the $id is identical to the field_name for fields. Only for field instances, the $id is compound of $entity_type.$bundle.$field_name.
Speaking of, I wonder whether the field instance entity type should be a derivative plugin, which automatically gets a config_prefix of field.instance.$entity_type.$bundle — so that the actual entity + field instance ID and $id property on the class is == $field_name.
Various properties in the exported configuration should not be contained, as they are plugin definitions, not configuration.
Most probably just needs a
getExportProperties()
implementation on the FieldInstance entity class.I don't really understand why all of the properties in $data are not top-level class and configuration properties...?
Are we going to fix that and remove the data bucket in a follow-up? Or what's the plan?
Comment #105
swentel CreditAttribution: swentel commented@sun yes - I'm now in process of adding more top level configuration properties - I've created a dedicated issue just for the testbot, because it's already a massive patch and I don't want to add more noise here, because that process of adding more is a massive tedious job. Code is in the sandbox, testbot helper issue over at http://drupal.org/node/1906218 - I've also ping yched to tell me whether I'm crazy or not as the patch is now already 378kb and I haven't even fixed all tests nor started on the instance yet ....
Comment #106
aspilicious CreditAttribution: aspilicious commentedI would make it a 2 step process. First get the cmi conversion in and than put everything again in a sane position.
Comment #107
andypostI think bout
uri()
for Instance - and sun's idea is greatWorking on #731724: Convert comment settings into a field to make them work with CMI and non-node entities comments as fields are attached via bundle but we need to store Field name and entity_type with entity_id because there's no ability to ident the Bundle in perfect world each Bundle should have some machine name to attach data
Comment #108
webchickThis feels like at least major; until this is done, we haven't really battle-tested the API.
Comment #109
yched CreditAttribution: yched commentedSo, after tons of heroïc work by @swentel, this is getting really close.
More details tomorrow, in short:
- $field and $instance are now ConfigEntities, created and saved by the regular Entity API :
- config files are in :
field.field.[field_name].yml
field.instance.[entity_type].[bundle].[field_name].yml
- Imports seem to work fine (we don't yet handle node types being renamed though), patch includes test classes for that
- Deleted fields & instances get out of the config folder, and are stored in state() until their data gets purged
(i.e. file deletion is deployed like everything else, by the fact that the file is not there anymore)
In order to keep the patch reviewable / rerollable (by not being 800Kb) :
- the old field_CRUD_(field|instance)() functions are still here as wrappers around the new Entity API CRUD code, and are used by the whole rest of core
- Field and FieldInstance implement ArrayAccess, so that the $field['property'] syntax used in like 6000 existing lines in HEAD still works
- The Field and FieldInstance classes are not hinted in all the places that now receive $field and $instance entities...
- No config schema yet.
Cleaning that up will be for followups :-)
Comment #111
yched CreditAttribution: yched commentedDrats. As could be expected, my last minute refactoring in the update function was broken :-/
Should be better now.
Comment #112
yched CreditAttribution: yched commentedPushed minor commits:
98d3a3a Fix possible bug on importDelete()
b3824f0 Add comment about why importCreate() works as is
Comment #113
alexpottAwesome work guys!
This should be using config's rename function. Atm it seems you might leave the old config around.
Very minor nit... this change is unnecessary
This looks like it can be simplified. No need for the
if (is_array($value)) {
:)Awesome work to remove this very weird circular dependency...
We've removed the dependency but with this function we still have it. On irc with @yched and @swentel might suggest convert to cmi early… so this function should be removed and all field changing hook_update_N's to just use config()
Think we need to declare that block_update_8008 and user_update_8011 need to run before field_update_8002
An issue with using setData is that config values are not cast to strings which means that it's likely that when a user goes to the field admin and presses saves the yml file might change but the user actually has not made any changes.
As above... this a tricky issue that would be resolved if we didn't cast everything to strings!
Hmmm... can't we use config here just to check whether or not we have a configuration object already?
This should be $instance->delete(FALSE);
Hmmm... can't we use config here just to check whether or not we have a configuration object already?
Does this not work now? Or are these test pointless?
Not sure why this change is necessary...
This test still seems valid looking at field_update_field(). Do we need to remove it?
Unnecessary
Should use DrupalUnitTestBase::installConfig()
Using config and not Config entity system... whereas below...
Uses config entity system... seems inconsistent
Should we be using config here... to read the raw config?
This might be clearer if it was
$field_definition = $this->field_definition;
Need to keep part of this @todo only the field stuff is being removed.
should be
$this->field = field_create_field($this->field_defintion);
to be consistent with other tests.Minor. Should have a leading \ as in
\Drupal\field\FieldInstanceStorageController
As above
Should be
Contains \Drupal\field\Tests\FieldImportChangeTest
Should be
Contains \Drupal\field\Tests\FieldImportCreateTest
Should be
Contains \Drupal\field\Tests\FieldImportDeleteTest
This comment proves we need to refactor the config import process so we can implement a way for config providers to say that this config needs to be imported before this other piece of config. Relying on the alphabet is smelly :)
Comment #113.0
alexpottUpdated issue summary.
Comment #114
BerdirOk, did run the upgrade path on my big site :)
Upgrade worked fine, didn't get an error during the upgrade. The only thing I noticed was #1943494: Use batch in field_sql_storage_update_8000(). Also, file.module wasn't enabled but image was, this blew up but that's not your fault I think.
Getting the following notices now sometimes, e.g. when clearing the cache:
Notice: Undefined index: deleted in Drupal\field\FieldInfo->getFields() (line 199 of core/modules/field/lib/Drupal/field/FieldInfo.php).
Notice: Undefined index: deleted in field_info_fields() (line 302 of core/modules/field/field.info.inc).
Notice: Undefined index: deleted in _field_sql_storage_tablename() (line 47 of core/modules/field_sql_storage/field_sql_storage.module).
.. and some more places. Given that it doesn't repeat, I guess it's only a single field that doesn't have it, not sure why.
Also, some of these:
Notice: Undefined index: bundles in field_views_field_default_views_data() (line 113 of core/modules/field/field.views.inc).
Warning: Invalid argument supplied for foreach() in field_views_field_default_views_data() (line 116 of core/modules/field/field.views.inc).
Notice: Undefined index: columns in field_views_field_default_views_data() (line 174 of core/modules/field/field.views.inc).
Warning: array_keys() expects parameter 1 to be array, null given in field_views_field_default_views_data() (line 240 of core/modules/field/field.views.inc).
And some additional similar ones in that function.
Note that I have a lot of field types provided by non-existing modules and possibly also some strange field configurations.
On the manage field page of a content type, I also got this warning:
Notice: Undefined index: urlwidget in Drupal\field_ui\FieldOverview->buildForm() (line 140 of core/modules/field_ui/lib/Drupal/field_ui/FieldOverview.php).
And the same for numberfield. I guess that's a field with a widget that doesn't exist?
And also the deleted notice in Fieldinfo again. Adding a debug there tells me it's a seemingly normal, enabled, text field.
Was confused for a moment because my field data wasn't displayed/loaded but that was because locale.module was disabled and field langcode was de everywhere.
So, apart from those notices and warnings, this seems to be working very well! All field information has been migrated correctly, as far as I can see.
Also:
:)
Comment #114.0
BerdirAdded list of follow ups.
Comment #115
swentel CreditAttribution: swentel commentedThanks Alex and Berdir!
Quick question for you Alex:
We are using rename() there no ? Or am I completely overlooking something ?
Comment #116
alexpottDoh! You're right... somehow just missed the rename()... so it's me doing the overlooking...
Comment #117
yched CreditAttribution: yched commentedFor now, just reroll after #1942000: Node NG broke Edit module (conflicts in Drupal\edit\Tests\EditTestBase)
Comment #118
yched CreditAttribution: yched commented@alexpott: thanks for the great review - many nice catches !
Committed those changes :
37c3cdf fixes for @alexpott's review #113
Some answers :
- field_read_*() calls in Field / FieldInstance()
Wasn't sure about reading raw config. Turned those into entity loads for now.
- unnneded change in hook_field_attach_purge() :
LOL - $field->field_name is definitely wrong in current HEAD. Was introduced in #776694: hook_field_attach_purge is not documented.
It becomes OK with current patch, so I reverted that change for now - however, the field_name property is going to be removed in the next iteration of the patch, in favor of $field->id ($field['field_name'] will still work while we keep the BC ArrayAccess layer). So I might very well reintroduce the change in the next patch :-p.
- Deleted tests in field/lib/Drupal/field/Tests/CrudTest::testReadFields():
Those tests were : "// Check that criteria spanning over the field_config_instance table work."
field_read_fields() doesn't support this anymore, coz this is not needed anymore.
(this was for FieldInfo::getBundleINstances(), that now uses the FieldMap() built from raw config data).
- changes in field/lib/Drupal/field/Tests/CrudTest::testDeleteField()
Right, not strictly needed, we can take care of that in the followup where we deal with becomes of field_read_(fields|instances)()
- Deleted field/lib/Drupal/field/Tests/CrudTest::testUpdateNonExistentField()
Updating a non-existing field is not possible anymore with the new Entity-based syntax :
$field->save() will be either a create or an update, but you cannot update an entity that doesn't exist, it will be a create.
So the code that generated the exception that is tested here is now just gone.
- "we need to refactor the config import process so we can implement a way for config providers to say that this config needs to be imported before this other piece of config. Relying on the alphabet is smelly :)"
Agreed. Opened #1944368: React to / force order of imports, and added a comment linking to the issue.
While the bot does its thing, looking into the notices reported by @Berdir.
Comment #120
yched CreditAttribution: yched commentedSilly typo in field_update_dependencies().
+ had to fix notices for missing properties in the instance created by block_update_8000() - which shows that it ran after the "Field -> CMI" update without the explicit dependencies.
6057c2d upgrade path fixes
Also, forgot this in @alexpott's #113 :
This sounds like a bug in setData() to me ?
$config->setData($array) !== foreach ($array as $key => $value) {$config->set($key, $value);}
sure is misleading...Comment #121
yched CreditAttribution: yched commentedPushed a fix for the 1st bunch of notices mentioned by @Berdir in #114:
3a34cc9 fix missing 'deleted' property on deleted fields in upgrade path
The 'deleted' property is not stored in the CMI files, but added as a default value at runtime in Field::__construct(). It's explicitly set to TRUE before moving the deleted field definition to state() in Field::delete() - but then field_update_8002() needs to do the same for the deleted fields it puts in state() during the upgrade path.
I'm not too clear whether this will also fix the notices in field_views_field_default_views_data(), those are more intriguing.
"Undefined index: bundles, columns... ": It's as if the function receives a $field that is not a Field object. For now, not clear how that happens.
Comment #122
yched CreditAttribution: yched commentedActually I think I get it now. Yeah, patch #121 should also fix the field_views_field_default_views_data() notices.
Dunno about the ones on "manage fields" page though.
@Berdir, if it's possible for you without too much hassle to re-test your upgrade, would be great !
Comment #123
BerdirWill do another run.
I think those on the manage fields page are missing widget types, which I guess is something that can happen, e.g. if you disable a module that provided a widget type that was still being used.
Comment #124
yched CreditAttribution: yched commentedPushed some cleanups for the contents of 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".
- 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)
--> Sample CMI files :
field.field.[id].yml
field.instance.[entity_type].[bundle].[field_id].yml
This settles us on a reasonable final target for the D8 CMI files. Updating the codebase to account for the new properties will be done in the followup that removes the ArrayAccess BC syntax on $field and $instance objects.
Comment #125
yched CreditAttribution: yched commentedOne last change is being worked on in #1906218: Helper issue for "Field CMI", and after that we should be good to go !
Comment #126
webchickAccording to #1703168-72: [Meta] Ensure that configuration system functionality matches expected workflows for users and devs, this is one of the blockers to wide-scale user testing of CMI, which is about as critical of a task as I can think of for D8. Raising this one's priority in accordance.
Comment #127
yched CreditAttribution: yched commentedOk, here we are. Pushed :
42220b2 (by swentel) only store raw config in state() for deleted fields
a1611ae a couple refactor / cleanups
And I think we're ready to fly.
Comment #127.0
yched CreditAttribution: yched commentedUpdated issue summary.
Comment #127.1
yched CreditAttribution: yched commentedUpdate for "ready to review" patch.
Comment #128
yched CreditAttribution: yched commentedAs a reminder for reviewers :
This patch moves $field and $instance structs to CMI / ConfigEntities. Comes with an update function, and test coverage for basic import operations.
In order to keep the patch size "reasonable", it includes a BC layer that keeps the existing APIs and structures working;
- keeps the old array syntax ($field['cardinality'], $instance['field_name']) fully working through ArrayAccess
- keeps the old field_CRUD_(field|instance)() functions working, on top of the new ConfigEntity way, that also fully works.
[edit : updated the summary, which already had a list of followups - expanded that a bit]
Comment #128.0
yched CreditAttribution: yched commentedtypo
Comment #129
xjmI reviewed up through
file.install
(more later). Few comments:Holy run-on sentence Batman. Also id => ID.
This should probably have its own separate upgrade path test. Also, nitpick, "ID" should be caps, and "UUIDs" should not have an apostrophe. We can also put
{file_usage}
in curlies.Why? (And elsewhere.)
Why?
There isn't an API change with
include_inactive
from D7; why is this change necessary?Missing preceding slash.
Why aren't we also deleting the previous definition of
$field
here?Mmmm, does this test still provide the intended coverage? Shouldn't we be installConfig()ing? Ditto may places elsewhere in the patch.
Comment #130
effulgentsia CreditAttribution: effulgentsia commentedHere's some minor stuff that jumped out at me:
Should this be $body_field_definition?
Why are we passing FALSE here? This can leave unused fields around: do we want that?
Huh? Don't we want to distinguish between a field definition array and a field object? Doesn't this blur that distinction?
Should the 2nd $field be $field_definition instead?
Do we have a follow up for removing the hard-coded class name here?
Do we have a follow up for removing the hard-coded class name here?
Should this be {field_data_FIELD_UUID} and {field_revision_FIELD_UUID}?
Why only 6? Does that offer a large enough hash space to make collision unlikely considering there's millions of Drupal sites (i.e., a 1 in 1 million chance of collision per site means several sites out there are likely to encounter a fatal error)? Perhaps raising to 32 (the length field ids are allowed to be) would be safer?
Since we're changing from name/id to id/uuid, I think we need to change the fieldsByName/fieldsById properties to match. Otherwise fieldsById is misleading.
9 days to next Drupal core point release.
Comment #131
swentel CreditAttribution: swentel commentedSome answers - but not all.
The user picture upgrade path proves that this works, so can we potentially defer this to a follow up if really needed ?
See http://drupal.org/node/1735118#comment-6366070
This is an actual bug in core - if you would enable the module again, the instance will still be there (but inactive), in D7 right now, we would create a new instance and the old one would still be left there.
There are not other settings in field currently besides the purge batch limit and this is not used. And we don't need tables anymore, win! We're working on a separate 'simple' conversation of other settings over at #1942346: Convert Field API variables to CMI - maybe that might influence this tests, but for now it doesn't.
Regarding the changes in FileFieldWidgetTest. This especially affects testMultiValuedWidget. This is a simpletest bug which we never hit in HEAD. There's a difference how instances are read now in memory:
This somehow confuses simpletest heavily in regard with file uploads. This bug is not reproducible manually. If you want to test for yourself, hardcode $field_name and $field_name_2 to different names (simply swap them) and the test will always fail, and pass vica versa. If you would do this manually, this will never ever fail.
Not sure, this was a suggestion at the Munich sprint (what a long time, seriously heh). 32 might work too I guess, that means table names of 54 chars which is still ok (mysql has a 64 char limit). Otoh, these tables are cleanup up relatively fast, so maybe a smaller nummer is better ? In the end, I don't have a strong opinion on the length though.
This is an existing bug in HEAD - fields and/or instances (especially the second one) would never be deleted at all by cron, because the entity doesn't exist anymore (the module was disabled first, and then uninstalled) (also PHP doesn't blow up with arrays when they are NULL - I should look up the code again, but that wasn't a funny one either.)
I'm not sure exactly what you mean. Do you mean we want to use entity_create() or entity_load() ? The second one is not possibly at all, as the field is already gone by done and doesn't physically exist anymore, I'm not sure about the first one, I have a feeling that one would work.
Comment #132
fagoI like where this is going! Just one thing that bothers me:
With the entity field API we already have a field object which is $node->body for example. So I think having $field object for that instance holding the data and $field for the config might become confusing. I see that this is just going on with the $field array and making it a object though. So I think that's something that needs more discussion and a follow-up maybe.
Comment #133
yched CreditAttribution: yched commentedWill reply to @xjm & @eff review when I get a chance but,
re #132
This patch is turning the $field / $instance definition arrays into objects, sure - that's by definition when moving some piece of config to CMI / ConfigEntities. I don't see how that is a problem with $node->body being an object too ?
Sure, there is a potential ambiguity about whether a variable named $field contains a field definition or a field value, but that's nothing new and also exists in D7 where both values were arrays. IMHO we're not going to go all over core and rename all $field variables to $field_definition, even in a followup. I guess the class names themselves can be discussed, though - but right, followup.
The examples pointed above are in a couple existing tests that use a very partial array with only basic properties (like field name and field type), to pass to field_create_field(). This partial array != the full fledged $field structure that is the result of the field_create_field(), and in some tests, depending on the way this specific test is structured, we need to distinguish the two.. Those will be cleaned up in the followup that removes the old field_CRUD_*() functions.
Comment #134
yched CreditAttribution: yched commentedThanks for the reviews so far, folks :-)
Little time tonight to address all the points, but for now pushed a couple changes :
@xjm #129 - 1 & 2
a02dd03 simpler handling of file_update_8001 dependencies
(I kept underscore 'id' when talking of the db column of the same name)
@xjm #129 - 3
8e5f8dd Revert needless/controvesial changes in file tests, + provide comment for the needed change
@xjm #129 - 6
73d9707 fix phpdoc
@eff #130 - 3
68c472c remove $this->field / $this->field_definition ambiguity in DateTimeFieldTest
@eff #130 - 5 & 6
271ea52 use entity_create() rather than hardcoded class name
@eff #130 - 7
1cda1ed update comment about {field_data_FIELD_UUID} table names
additionally :
89c28a8 More targeted update on ids stored in {file_usage} for 'default_image'
Global interdiff attached.
Comment #136
yched CreditAttribution: yched commentedDrats. Reroll.
Comment #137
podarok#136 nice work
+1 RTBC
Comment #138
amateescu CreditAttribution: amateescu commentedThis is not rtbc yet, what are you +1ing exactly?
Comment #139
podarok#138 I`m not parsed via all 130+ comments for rtbc statuses already and just reviewed patch witch looks good for me
+ - its just a habit )))
Comment #140
podarokfew more bits to review
this has to be described in change notice and better documented
this has to be described in change notice and better documented
this has to be described in change notice
not easy to grok why
+ if (entity_get_info($entity_type)) {
http://drupal.org/node/1929006 deprecated
Edit: not nessesary as it loads only one instance...
skip...
http://drupal.org/node/1553180 entity_load renamed
Comment #141
swentel CreditAttribution: swentel commentedThose crud functions are scheduled to be removed in a follow up, and as long as entity_load and entity_get_info exists, let's keep them for now, writing
Drupal::service('plugin.manager.entity')->getDefinition($entity_type);
is so horribly annoying.Comment #142
yched CreditAttribution: yched commentedOops, somehow forgot to post the interdiff in #134.
Here it is.
Comment #143
yched CreditAttribution: yched commentedReroll.
Comment #144
xjmAha! Excellent; thanks @yched. This comment (plus only changing the tests that include multiple fields) assuages my fear. :)
We could do a
test_file_field_1_randomstring
andtest_file_field_2_randomstring
maybe... but mostly, let's file a followup issue for the simpletest bug.And, agreed @swentel -- there's no reason to remove BC wrappers in this patch; we want to keep the diff as small as possible.
Comment #146
xjmRound 2, from the top down to just before
field.install
. :)Note to self: API change to add to the summary (also look for why).
This is hard to read. Also, whenever I see
concatenation.and.string.manipulation.galore
I suspect we should have named methods for doing whatever manipulation. If the config object name format for fields isentity_type.bundle_name.field_name
, let's codify that in a method on the class. (This same problem applies to blocks and other entity types, but I think it's not too hard for us to fix here.)Edit: Also, hmm, we seem to be skipping the entity system here? This seems like something that should DEFINITELY allow hooks to respond to the rename.
This is lovely. :)
Huh, so we're supporting either an array of values to update, or the actual entity, for BC? We might want to consider filing a followup to remove support for the array (if we don't remove this function entirely, that is). Meanwhile, though, we at least need to update the docblock to indicate that either an array or a field entity is accepted. The same also applies to
field_update_instance()
and friends.This hunk could use a clearer comment; something like "Update the field with the passed-in values." (Also, comments should end in a period.)
Out of scope, but I would dearly love to call this
$conditions
instead of$params
so that the function was more human-readable.Ahhh, comma splice! (Yes, I know it already was there.) :)
We need to update this parameter description to include all the possible flags and what they mean. I suggest a nice list.
wat
(Translation: Uh, let's add a comment for all this handsome inline logic.)
"Configuration fields"? Maybe "fields stored in the active configuration" or something? I don't actually know what the comment is trying to say.
Nitpick: double period.
Okay, what? You just lost me. "Conditions" is not helpful. :) Also, this switch statement inside a foreach with a continue 2 seems like an odd way to do... whatever it is we're doing?
Maybe:
// Invoke hook_field_read_field().
I'm guessing we need to use the UUID for deleted fields because they no longer have a serial ID? Why not the machine name like before?
Also, maybe out of scope, but it seems like a bit of code smell for the return value to have different possible keys. In any case, in means we need to at least update the return value info in the docblock.
I think all the stuff I said about
field_read_fields()
applies here too. Also, maybe we need to DRY this up a little? (Maybe out of scope, maybe not?)Do we have an explicit followup issue filed for this already? Despite the dissent of certain other core contributors with three-letter usernames who are in the D8 top 5 by commit mentions, sometimes I think it would be nice to just link the issue to fix the @todo in the codebase.
Comment #147
swentel CreditAttribution: swentel commentedReroll - fixing both tests. Interdiff also contains comment for #129.5
- edit - weird, something went wrong in creating the patch re: views analyze tests .. but it should still be green though - note, yeah, didn't fully merge sandbox with 8.x - bah
Comment #148
swentel CreditAttribution: swentel commentedBetter patch
Comment #149
Anonymous (not verified) CreditAttribution: Anonymous commentedi did some profiling work on the patch in #148.
hitting the front page as an admin user, no content, 4.7% regression.
a lot of that seems to be the CMI cache, because we do a query per config file, so not really the fault of this patch.
i've attached the xhrpof aggregate files if anyone else wants to dig in.
Comment #149.0
Anonymous (not verified) CreditAttribution: Anonymous commentedtypo
Comment #150
yched CreditAttribution: yched commented@beejeebus: Thanks for running this.
The patch should not have such an impact on warm field info cache. Investigating.
[edit: problem seems to be on the massive use EFQ now makes of field_info_field(), even when *not* querying a condition on a field - and now that menu links are entities loaded by EFQ... More digging tomorrow.]
Comment #151
amateescu CreditAttribution: amateescu commented@swentel found the same thing here: #1931900: EFQ calls field_info_field() too liberally
Comment #152
yched CreditAttribution: yched commentedA couple pushes by @swentel & me, and replies to the remaining points in @xjm & @effulgentsia's reviews in #129 / #130
(leaving out @xjm's 2nd chapter in #146 for now)
- @xjm #129 - points 4 & 5 - Changes in forum_enable().
This one required some investigation, since different people got involved in those changes a couple months ago :-).
So the code in current HEAD tries to account for a possible error condition when both forum and taxonomy are being re-enabled at the same time, after having been disabled. In this case, the 'taxonomy_forums' field still exists, but has been marked inactive because taxonomy got disabled
When forum_install() runs, the field hasn't been switched back to 'active' yet (it will be, but just after hook_enable()), so field_info_field('taxonomy_forums') returns NULL, but field_create_field() would fail because the field *does* exist.
HEAD does:
Patch does a more reasonable:
- @xjm #129 point 7 & @eff #130 point 1
--> 471d519 Reverted the changes in _update_7000_field_create_field()'s signature, those were not strictly needed
- @eff #130 point 2
--> 478c5f0 Reverted the changes in comment_uninstall(), they don't seem needed either
- @eff #130 point 9
Yes, but that's also true of a lot of other code that does stuff like $field_id = $field['id'] / $instance['field_id'].
I'd really rather do this kind of cleanups in the followup that removes the ArrayAccess syntax: since it will touch all those same places anyway. Doing it now seems like a slippery slope towards doubling the patch size again.
Comment #153
yched CreditAttribution: yched commented- @eff #130 point 8
So the problem here is table prefixes - like simpletest uses, for example :-)
This produce tablenames like "simpletest292903field_deleted_data_[SOME_ID]". That's 35 chars before the "[SOME_ID] part, 29 chars left - not enough for a full UUID :-(
We can't really predict the length of table prefixes that are used on acual sites out there, but maybe we can assume that simpletest's ones are amongst the longest ?
@swentel pushed from 6 to 10 chars, maybe we can go up to 20 ? (
--> ea9edd3 Generate slightly longer hashes for deleted table names, and move the logic to into field_sql_storage
Comment #154
yched CreditAttribution: yched commentedLastly for tonight, regarding the perf issues mentioned in #149 :
I can't seem to reproduce @beejeebus' results on a fresh site hitting front page with no content, I can't see any significant impact between patch and HEAD.
On slightly more meaty setup though (3 node types, 10 fields each, 10 nodes on frontpage), I do see a noticeable impact.
Most of it is caused by #1931900-3: EFQ calls field_info_field() too liberally - EFQ calls field_info_field() just to check whether a given query condition is about a Field API field, and this doesn't play well with FieldInfo cache strategy. There are better ways to do that, will be done over there.
Still, pushed some optimizations in field_read_fields() / field_read_instances() for the "simpler cases" (which are also the most common) where we do have the id of the field/instance to fetch.
With those optims, and on the same setup as above, my ab tests seem to show that latest patch runs 12% *faster* than HEAD. Which, er, is very surprising, and probably requires some xhprof digging. Will try to do that tomorrow.
--> 8539a78 optimize field_read_(fields|instances)() for the "good" case
Updated patch, and interdiff for the commits mentioned in my three last posts.
Comment #155
effulgentsia CreditAttribution: effulgentsia commentedWe allow field names to be 32 characters, so that implicitly sets a maximum of a table prefix to 64 - 32 - strlen('field_revision_') = 17. The limit on the hash is therefore 32 - strlen('_deleted') = 24. So we can either up to 24 or remove the "_deleted" from the name and use the full 32 without needing to hash and truncate (but needing to remove the hyphens).
That's good enough for now. The above can be a follow up discussion.
Comment #156
Anonymous (not verified) CreditAttribution: Anonymous commentedre #154 - "I can't seem to reproduce @beejeebus' results on a fresh site hitting front page with no content, I can't see any significant impact between patch and HEAD." - were you logged in?
my results come from hitting the front page as an admin.
Comment #157
yched CreditAttribution: yched commentedNope, you're right, I was testing anon page views with ab.
Plus, the HEAD and patched setups I was testing were separate installs with different random content, so the page views were not strictly the same.
Redid my benches by:
- starting with the same fresh HEAD, creating random fields and nodes,
- duplicating that db + config over to a new site, applying patch, running upgrades.
- running ab both with and without a session cookie for uid 1
Results:
3 node types, 10 fields each, 100 nodes, frontpage showing 10 nodes
anon :
HEAD - 288ms (stddev 4.1)
patch - 290ms (stddev 5.2) --> + 0,7 %
user 1:
HEAD - 481ms (stddev 10.4)
patch - 489ms (stddev 8.6) --> + 1,6 %
(Note: the results for the patch are with the optimization commit mentioned in #154)
I won't have the time to delve into xhprof today, but the overhead seems more inline with what we'd expect by moving from "SQL + arrays" to "CMI + ConfigEntities + (temporary) ArrayAccess BC layer". Also, the difference is within stddev on my (pretty lame) setup.
Side note - pushed :
--> ff5af12 - fix HEAD to HEAD upgrade path
in order to be able to run the upgrade on an D8 setup (patch was renumbering updates, causing the "field to CMI" update to be skipped when starting from a D8 site)
+ merged latest HEAD
Updated patch attached.
Comment #158
xjms/id/ID/g
http://en.wikipedia.org/wiki/Id,_ego_and_super-ego
:)
Changes look good other than that. I'll review the rest of the patch once I get the chance.
Comment #159
yched CreditAttribution: yched commentedPushed a fix for #158:
194657b id --> ID
Working on @xjm's other remarks in #146, but will probably not get there before monday.
Comment #160
Anonymous (not verified) CreditAttribution: Anonymous commentedhere's some xhprof data for #157.
runs against the front page, anonymous, with content (article with some extra fields added), 1.4% regression with an aggregate of 30 runs.
same as above, but logged in as admin, 0.6% quicker.
definite improvement over the first set of runs :-)
i've attached the aggregated xhprof files if anyone wants to dig in.
Comment #161
yched CreditAttribution: yched commentedUpdated patch for @xjm's review in #146 - meaning we've caught up with the reviews so far :-)
- Point 1
was also in @eff's #130, those changes got reverted meanwhile.
- Points 2 - update bundle names in field_attach_rename_bundle()
The fact that the code operates on raw config and not at the entity level is intentional. This is only internal housekeeping as a reaction to external changes, and should not go through the entity hooks, it's not as if the field instance really changed. That's how it's done in D7 and current HEAD - direct updates on the {field_config_instance} table, not going through field_update_instance() & associated hooks.
Given that, it seemed awkward to have this piece of code go through $instance->id() to handle logic around ids - wrote it and reverted, just looked awkward.
It's housekeeping stuff dealing with CMI-level storage, so it has knowledge how storage is structured, that's fine IMO.
I did add an override for FieldInstance::id() to include the string concatenation logic, just not using it here.
--> 62d29b1 Cleanup field_attach_rename_bundle()
- Point 4 & 5
As mentioned in the issue summary, field_(create|update|delete)_(field|instance)() are totally up for removal in a followup, and are preserved only as an intermediary BC layer to avoid a 700k patch. Adjusted the phpdoc for field_update_(field|instance)() param, and cleaned the inline comments. The other fonctions are not affected by those mixed input considerations.
--> 6f4a59a clearer doc for mixed param accepted by field_update_(field|instance)()
- Points 6 to 15 - field_read_(field|instance)()
I had to google 'comma splice' to get what you meant, I'm now less ignorant than I was when I got up :-)
Other than than, tried to clean and comment best as I could, but keep in mind that f_r_(fields|instances)() functions are most likely on their way out, and are preserved here for temporary BC. Thus, making them better than what they currently are in HEAD is out of scope here. The goal is just to keep them working (OK, preferably with non-ugly code).
Especially, we won't be DRY-ing field_read_fields() & field_read_instances() here. DRY is called EntityFieldQuery, in this case. That's a followup.
Not changed by the patch, so not done for the reasons above.
Same. The phpdoc for the return value states this fact already, but added an inline comment with the actual reason.
--> 1a92e89 Cleanup field_read_(fields|instances()()
- Point 16 - "// @todo EFQ currently fails on conditions on comment bundle."
This will be fixed when the comment base table gets a column holding the exact values of comment bundle names. There was a dedicated issue for this, but it was superceded by #731724: Convert comment settings into a field to make them work with CMI and non-node entities, so added a link to that one.
[edit: fixed wrong issue link]
--> d5084ba add issue link for 'EFQ chokes on comments'
Comment #166
xjmOkay, I deleted all the ranty comments because I realized that my reactions were based on a previous experience and not this issue or the contributors working on it. :) Just please, two things:
@param
and@return
need to reflect that. Otherwise, there's not enough explanation for how callers should use the function, and people have to sift through all those switches and ternaries to try to decipher what is going on.Comment #167
xjmDown to field's
lib/
.I don't see an upgrade path test in the patch. Did I miss it or does it still need to be written?
Minor:
field_read_fields()
should have parens.I'm a little confused; why is it
'uuid'
when we read (and in the variable names) but just the'id'
when we write?Not quite a sentence; how about "Set fields with missing field type or storage modules to inactive."
Let's replace this link with a comment that actually explains whatever you're trying to explain with the link. :) @todos are one thing, because they're something we intend to change, but a link is a poor substitute for an inline comment. (I read that issue comment and still didn't quite understand.)
Comment #167.0
xjmUpdated issue summary.
Comment #168
swentel CreditAttribution: swentel commentedCreated the follow ups and updated the issue summary, we're getting real close, I can feel it! :)
Comment #169
yched CreditAttribution: yched commentedJust to be clear on possible misunderstandings above :
field_read_fields() / field_read_instances() behavior is *not* affected by the patch. If you get the feeling they are, it's possibly an effect of the patch diff format, but watching the code side-by-side should make that clear.
- The 'include_inactive' logic is just moved around, but was already present and is left unmodified.
- The format of the return values of field_read_fields() are unchanged from what they are in HEAD & D7 - "A field definition array, or FALSE" is the phpdoc for field_read_field() (singular - different function :-p)
#161 is most definitely not advocating to let unaccurate phpdocs hang around because we'll probably remove the functions later and are to lazy to update them - eww, no :-). It is merely advocating to not turn this 200k patch into perfecting docs for functions whose behavior is left unchanged from D7 - especially when those functions are identified for deprecation in followups.
Those followups have been identified and put in the issue summary like a week ago - but yes, not opened into actual issues so far.
Pasted the wrong node id for "// @todo EFQ currently fails on conditions on comment bundle" in #161. Correct issue is #731724: Convert comment settings into a field to make them work with CMI and non-node entities.
My bad, edited the post. The link in the committed code was correct though : d5084ba
Comment #170
yched CreditAttribution: yched commentedre @xjm's review part 3 (#167)
1. Right, patch has no explicit test for the upgrade path now.
It is de-facto tested by "custom block body to field" and "user signature to field" upgrade tests, since those updates run before field_update_8003(), so their fields and instances are created in the "old" storage, are then moved over to CMI, and are tested to work fine after the upgrade ran.
But sure, this is important enough to get a dedicated test, just not sure I'll be able to add it before a couple days.
Also, #1953418: Run "Field API : CMI" upgrade as early as possible. is about refining how we'll handle the upgrade path, so maybe add the tests over there (grin) ?
2. Fixed
3. Yup, wrong var name indeed. Fixed.
4. Fixed.
5. Actually that change should not be needed anymore.
It was an issue when the patch attempted to move all $instance['foo'] to $instance->foo (NULL->foo breaks). But now that we scaled back to supporting array syntax as BC, NULL['foo'] just "works" (as in "doesn't fail") silently, just like in current HEAD.
$instance being NULL is a problem for #1862758: [Followup] Implement entity access API for terms and vocabularies to solve (patch is RTBC over there), but we don't actually need to work around it here meanwhile.
Comment #171
swentel CreditAttribution: swentel commentedAddition to the upgrade path: there's a small test in FieldUpgradePathTest.php. Granted, it could probably do (a lot) more :)
As yched also said, the user upgrade indirectly tests the upgrade path of fields and works.
@yched - if you have some ideas during the day tomorrow, let them know here, or mail me, I have time tomorrow night, after that I'm offline though until april 7.
Change in UserPictureUpgradePathTest.php
The small Field API upgrade tests
Comment #172
yched CreditAttribution: yched commentedSimple reroll for a conflict with #1943494: Use batch in field_sql_storage_update_8000().
@swentel: ah, right, I spoke too soon, forgot about testFieldUpgradeToConfig().
So yeah, right now this just checks that we do get something not NULL for a body field on article nodes.
I'd suggest:
- check against the raw CMI store rather that field_info_(field|instance)(). We test the process of writing the correct CMI records, the field_info_*() functions are largely tested in other places.
- check that the content of the config arrays is the one we expect (well, other than the uuid, of course - but check that field_uuid is correct in the instance)
- cover the full range for body field :
config for field exists and is correct
config for instances on article & page exist and are correct
(maybe ?) no other instances exist for the field
- add records for a deleted field and instance in drupal-7.field.database.php, and check that they end up in state() and not config()
Way cool if you can move some of it, I'll pick up where you left.
[edit: oops, looks like I got my hours mixed up, too late for @swentel. Bleh, my bad. I'll try to take a crack at it tomorrow]
Comment #173
yched CreditAttribution: yched commentedForgot to attach the rerolled patch.
Comment #175
swentel CreditAttribution: swentel commentedTest failures are due to #1865206: Clean up ConfigFactory + ConfigStorageController::save/postsave/rename().
Fixed those + a small 80 chars fix + additional upgrade tests. This revealed we had a racing condition problem because field_sql_storage_update_8000 runs before the config upgrade. I've added extra update functions for sql storage, we should really revise this in #1953418: Run "Field API : CMI" upgrade as early as possible..
Interdiff attached, [edit - pushed to sandbox]
I have some unexpected time tomorrow evening as well, so I can dig in those further then.
[edit: hmm, looks like that 80 chars was not needed, need more sleep :)]
Comment #176
swentel CreditAttribution: swentel commentedComment #177
alexpottWhilst talking to @swentel in IRC noticed a couple of minor nits..
the field the field why hast thou forsaken me? :D
No need for this implementation.. according to @swentel already removed from the sandbox.
Comment #178
swentel CreditAttribution: swentel commentedFor 1: http://drupalcode.org/sandbox/yched/1736366.git/commit/3dced29720139124d...
For 2: http://drupalcode.org/sandbox/yched/1736366.git/commit/aebc0969b3a4599e8...
Comment #179
effulgentsia CreditAttribution: effulgentsia commentedI think the architecture has been thoroughly reviewed at this point, so removing that tag. Can we get a new patch for #178? Is there anything known that's left to do before this can be RTBC?
Comment #180
yched CreditAttribution: yched commented@swentel and I pushed a couple commits to enrich the upgrade tests.
Attached is the interdiff with #170 (which was the state of the code when @xjm asked for more tests) on the relevant files.
The tests are those listed in #172, plus:
- test field values on a node migrated from the D7 db
- test creating a new node in the final D8 state.
re #179 - "what's left to do ?"
- talked with @swentel, I intend to rework a bit the changes mentioned in #175 around field_sql_storage_update_8000() - in short, make it run after "field -> CMI" rather than before, and remove the need for the _update_8000_field_sql_storage_*() helper funcs that were added over there.
Hopefully, will tackle this tonight or tomorrow.
- I think @xjm still has an in-progress review on the code in field/lib (field / instance ConfigEntity classes and controllers).
Comment #181
yched CreditAttribution: yched commentedAlso, still a @todo in the code :
- generating the schema for deleted fields too would be a little more involved.
- discussing with various people (@effulgentsia, @chx, @fago,), it seems no-one sees the interest of exposing the schema of field_sql_storage dynamic data tables - deleted or not.
@DamZ pointed that ctools D7 uses the 'foreign keys' schema entries to auto-generate relationships - and it reads them from drupal_schema(), not from each individual field's hook_field_schema().
But even in this case, this is of no use for "deleted fields" data tables.
Thoughts ?
Comment #182
yched CreditAttribution: yched commentedMh, after a closer look :
That's actually not true - ctools uses drupal_schema() to read foreign keys on entity type base tables, but foreign keys for field data tables are read from hook_field_schema(), not the schema present in drupal_schema().
Comment #184
yched CreditAttribution: yched commentedStupid last minute change. randomString() is no good for a string that will run through check_markup()...
Comment #185
yched CreditAttribution: yched commentedPushed more changes around the upgrade path & its tests :
- 55365dd Run field_sql_storage_update_8000() after 'Field -> CMI' update.
As mentioned in #180, this removes the _update_8000_field_sql_storage_*() helpers that were added in #175.
- 50fbbf6 Missing in 'Field -> CMI' upgrade : rename tables for deleted fields.
- 5cb1a3a Add tests for "rename tables for deleted fields" in the upgrade tests
- 1ad2128 Make $entity_type param optional in Field(Instance()::__construct()
Comment #186
yched CreditAttribution: yched commentedRe: schema for deleted fields: ok, was a bit stupid of me make a debate of this, adding this was actually not that hard :-/.
Let's do this for now, and move the topic over to #1957204: Stop exposing dynamic field data tables in hook_schema() ?.
[edit: sorry, forgot to attach the interdiff. Related commit diffs are e14a9ef and 5642c47]
This means we're done here for now - aside from pending review from @xjm :-)
(also, oops, didn't mean to re-add the tag in #180)
Comment #187
yched CreditAttribution: yched commentedTurns out this
if (!defined('MAINTENANCE_MODE')) {
check in field_sql_storage_schema() was not needed anymore.+ cleaned up the deleted field & instance definitions used in the upgrade test.
Comment #188
xjmAlright, I went over the test coverage again closely to understand how the new API is used. Overall, the test coverage is nice and thorough. I did pick on the comments a little... sorry about that. :) There's also a few outstanding questions WRT to CMI in general (not in scope here) but I made note of them to explore in other issues.
I also have a couple questions/observations about the stored object format (near the end of the list below).
Presumably the second commented line ehre is a leftover from before config entities? Can we remove that?
Is there an issue for this
@todo
? I don't quite grok it, nor does the comment incdicate which test asserts this behavior.So, a question for this and the other tests. Why aren't we using the entity API when we update the label?
These comments are nice and clear. :) I think they're wrapping a bit early, though.
...So an interesting question is, say you enable a field-providing module in your test environment and it installs its default config. Then you either: synch your config to prod first and then enable the module in prod, or enable the module in prod and then synch, or enable the module in prod without synching. What happens in permutations of these scenarios? Probably something that will need to be a followup for #1776830: [META-1] Installation and uninstallation of configuration provided by a module that belongs to another module's API.
I can see it's copying files. What might be more helpful is from where to where and why. :)
Seems to me like CMI should have methods for adding/removing entries from the manifest. (Totally out of scope and unrelated; just an observation.)
Huh? It looks like it's just updating the manifests.
s/staging field/staged field/ ?
Nitpick: The docblock should be one line of 80 characters or fewer. Try:
"Test importing deletions"? Huh? Maybe "Tests import when a field and instance are deleted."? (Note that it should be "Tests" rather than "Test".)
"Confirm that the field exists" would be clearer. However, it looks like it's actually just loading the field? If it's checking whether it exists, there should be an assertion. (That actually is a good idea here anyway; if we're asserting later that it's gone, we should assert here that it exists before we delete it.)
This comment isn't quite a sentence; is it supposed to be "then" instead of "the"? Also, there are too many nouns in a row.
After reading the code, I'm thinking it's maybe:
Nitpick: These inline comments are kind of useless. I can see it's purging the batch from the next line. A more useful comment would tell me why. :)
Plethora of nouns again, or are they verbs?... Oh, I think I finally get it. (Note that I am reading this test from the bottom up.) "Test import field" is how we're referring to the field? Can we just explicitly say "The field_test_import field" or "the field_test_import isntance" for clarity?
Articles will really help clarify what's a verb here and what's a noun. E.g.:
I'd suggest s/check that/confirm that/gi.
Nitpick of all nitpicks: There should be a blank line between the last member and the closing curly for the class.
Some of my suggested clarifications for the other test apply to these as well (the docblocks, the name of the field in the assertions, etc.).
Tests importing what changes? Maybe:
Tests importing an updated field instance.
Do we have a method for generating field instance object names rather than composing them from the constituent parts by hand?
I think I commented on this already elsewhere, but: we're keying by UUID here instead of ID in case there are duplicate machine names for whatever reason? As far as I can tell the UUIDs are not stored as keys in the SQL storage, only in the configuration.
So far in the test data I've only seen 1:1 field/instance pairs. Do we have coverage for multiple instances with default config installation, confing import and synching, etc.? (Probably okay as a followup if not.)
active == 0 => disabled?
Some loose typing craziness going on here. In one field definition it's the string '0', and in another it's false. Maybe a CMI bug? Could be #1653026: [META] Use properly typed values in module configuration.
What's this, the allowed entity types? The entity types it currently uses? And why is this empty; is an empty value "any"? Hopefully if I look at the Field docs it will explain.
What goes in here? Serialized field settings or such? (I probably need to look at the schema.)
So the object name format for instances is
field.instance.entity_type.bundle_name.field_name
, and the field UUID, entity type, and bundle name are also stored explicitly as object keys. The only thing that's odd here is that the field definition ID is only in the object name and not in the object, unlike everything else.(This raises an interesting point. Are we doing anything with the UUID for config entities in CMI generally? Totally out of scope; just something I haven't thought about in awhile.)
...And then the field definition stores just its settings, storage information, machine name and UUID, and the machine name of the field type and the owner module. The object name is just
field.field.field_name.
The widget also stores both its type and owner module.
More shortly; just posting this much now so I don't lose it during my 10a phone call.
Comment #189
gdd#188.6 - I'm not a fan of doing this, ideally we should only be interacting with the manifests through saving/deleting configuration. That said, I can see where it might be useful for tests so maybe a test-only function?
#188.21 - The main usage for UUIDs is determining when config has been renamed vs new. So unless we're in an import scenario, it seems like the two are interchangeable with no ill effects. Its very possible I'm missing something though.
#188.24 - Until otherwise decided, all booleans should be quoted 0 or 1.
Comment #190
xjmElsewhere in D8, we've moved away from having spaces in array keys and standardized on underscores. See:
http://drupal.org/node/1235918
http://drupal.org/node/1827470
http://drupal.org/node/1796000
Why this API change, and why the space?
Comment #191
xjmSo I had the bright idea that I should review the existing codebase, to understand what's already there. And... yikes. Is what's in
Drupal\Core\Entity\Field
EntityNG fields, and completely unrelated to anything in this patch? Cause there's aDrupal\Core\Entity\Field\Type\Field
in there too. (Not confusing at ALL.)Alright, anyway. I gave up on all that, because
TypedData
is a very deep rabbit hole of absraction. Like, Alice in Wonderland style. Edit: and @berdir confirmed for me that it's completely unrelated to the current fields and instances that are stored with the entities in this patch.I started reviewing the
Field
andFieldInstance
config entity types provided by this patch. Then in the middle of a phone call I accidentally pasted part of the review, so I figure I'll post it. I've gone over most of the member variables and the constructors, not the rest of the classes yet.Okay, we DEFINITELY need to clarify this documentation. I just spent 15 mins writing up comments about these two values that, upon my inspection of the exported test field instances, proved to be utterly wrong. After another 30 minutes of looking it over:
$id
is the config object name, e.g.,field.field.my_field_name
? But it might not include the field module prefix? For the instance, it's presumablyentity_type.bundle.field_name
? Let's put an example of the formatsin a second paragraph of each docblock.$field_name
is supposed to be just the last part of this, the field machine namemy_field_name
, i.e., what the user sets when creating the field. Let's also document that and give an example.bar_field_name
not in synch with the actual config object nameentity_type.bundle.foo_field_name
. I have suggested this repeatedly in multiple config entity conversions; we need to have methods for the composition and decomposition of configuration object names. :)getField()
would be a good name for the method. (For one, we have this ongoing consistency issue with foo() vs. getFoo() for protected properties in the Entity API, but mostly, I would expectgetField()
to return an instantiated Field object. Unless that's what you mean?)Should these really be public?
These need more detailed documentation of what these members are for. The member docs on the base classes are our chance to explain what these values are for and how they're used, in detail. (Arrays in particular need to have their expected format documented, even if it's just a reference to another function or class or method...)
Also not sure about these being public. This flag should presumably only be set when a method is called on the object, right?
I feel like we should document what we expect to have in the
$values
array here.Field
implies that the field type andfield_name
are required, that the thefield_name
needs to be a valid machine name.FieldInstance
, they imply thatentity_type
,bundle
, and one offield_name
andfield_uuid
are required.Again with the
id
vs.field_name
conversion. Here it looks likeid
is canonical for the machine anme, and notfield_name
. But elsewhere, ID means the configuration object name, and the machine name isfield_name
. I don't get it. :)Okay, so the old one was a simple ArrayAccess wrapper for fields in the DB, and the new one is a proper config entity type. I'm assuming the ArrayAccess use is just for BC and that we'll remove it once our conversions are complete?
Nitpick: missing period. However, again, we could describe this much better, e.g., "A function that provides the default value for the field instance."
Nice, glad to see these here.
Interesting that we're calling out to
field_info_field()
here. Which, apparently returns the cached field definition, via a procedural wrapper that wraps theFieldInfo
class... presumably some day later in the conversion process this will all happen inside an$entity->load()
?Nitpick: We usually use the Oxford comma in core, so there should be a comma after "disabled" as well: "an instane of unknown, disabled, or deleted field @name".
We'll want to replace this @todo with a reference to our followup here as well.
This behavior also seems questionable. The field ID should be the config object name, correct, containing dots? It seems unstable to populate the machine name from that.
It would also be good to document the provided defaults in the docblock... Though actually, I feel like the defaults array we provide could also be a class member on its own.
This is some magic behavior. We default the field label to its machine name? Does this functionality exist in D7? Seems... odd.
Comment #192
yched CreditAttribution: yched commentedre @xjm's review #188:
1. Fixed.
2. I need to investigate that one a bit.
3 to 19: Reshaped the import tests quite a bit. In some places I took different options than the one you suggested.
3. "Why aren't we using the entity API when we update the label"
The tests place stuff in the staging directory, then run config_import(). You cannot write to the staging directory by manipulating objects at the ConfigEntity level, the controllers are hardwired to write to the active directory.
4:
"say you enable a field-providing module in your test environment and it installs its default config. Then you either: synch your config to prod first and then enable the module in prod, or enable the module in prod and then synch, or enable the module in prod without synching ?".
Right, if you import part of the config then enable the module, its default config will what's arleady been imported, that might have been altered from the initial default config already.
That's totally not specific to Field API ConfigEntities :-). I'm not even sure #1776830: [META-1] Installation and uninstallation of configuration provided by a module that belongs to another module's API is the place for that, because I don't think that's actually related to "...that belongs to another module's API" ?
But right, not for this issue to discuss.
10.
I Kept "Tests importing field and instance deletion" because, well, that's what this is ? The config changes that are imported can be creations, updates, deletions, here we're testing importing a deletion :-).
11. Kept "Check that X is foobared" formulation, that's used throughout all of core tests.
20. "Do we have a method for generating field instance object names rather than composing them from the constituent parts by hand?"
I wish there was, something like $entity->getConfigName() ?
AFAIK closest we have is entity_get_controller($entity->entityType())->getConfigPrefix() . '.' . $entity->id()...
Seems like a lot of hassle here. I did switch to using $entity->id() rather than concatenation of various parts.
21. "I think I commented on this already elsewhere, but:" we're keying by UUID here instead of ID in case there are duplicate machine names for whatever reason?"" As far as I can tell the UUIDs are not stored as keys in the SQL storage, only in the configuration"
This is a test storage, no SQL involved here, it stores field values in an array in a variable.
Field names are not unique, there can be several *deleted* fields with the same name, and a storage engine needs to keep the values around until they are purged. So this test storage keys the data by the only thing that's unique for a field : the $field['id'] in HEAD, the $field['uuid'] now.
That's all for tonight.
Comment #193
xjmThanks @yched!
Sorry, in the one I (thought I) commented on, the word "that" was missing, which results in ambiguous language.
Basically, the test comments were confusing as all getup, because there were long strings of words that could be either nouns or verbs. :) It's English's fault, but we can mitigate it. I'll re-read and see if it's clearer.I see what happened here... my sed made no sense in context. :D"Tests importing field and instance deletion" still sounds like nonsense, even if it's technically accurate... :P I certainly didn't understand that it meant what you just explained from reading it.
Comment #194
xjmJust a followup, I read the updated tests and they're tons clearer, so #193 is mostly irrelevant. :)
Comment #195
xjmMaybe "Tests importing the 'deleted' status for fields and instances"?
Comment #196
xjmAlright, I reviewed the rest of
Field
. Overall, it seems well-architected and robust. Love the thorough use of exceptions. My questions above about the unique identifiers above and which members should be public remain. Other than that:Once #1937600: Determine what services to register in the new Drupal class is in, we can convert this to the
moduleHandler()
method (but #1957154: Replace calls to drupal_container()->get('module_handler') service with Drupal::moduleHandler() should also catch it, so it doesn't matter what lands when).Okay, this is good to know.
The pattern
field.instance.entity_type.bundle_type.field_name
for the object name is then safely within the name length limits. (See #1709960: declare a maximum length for entity and bundle machine names, #1186034: Length of configuration object name can be too small, and #1888218: Default configuration entities provided by a module should include the module name in the machine name.)Edit: Can we add a constant to the class for that max length?
So I didn't quite understand this on first read. What this is saying is: we are about to create the storage tables based on this new or updated field definition. We need to make sure this particular field has all of the settings keys that are possible for this field type, so that the tables are created with all these columns.
This implies that the columns are created by inspecting the settings array's structure? It seems to me like it would be safer to explicitly use the field type definition when creating the tables, rather than implicitly, and then just set the defaults from the saved field. This might be out of scope here, but if so maybe a followup discussion? (I also haven't reviewed the storage controller yet, so there might be a reason for this I don't see.)
In either case, could you add a little more detail to the comment?
I don't see
field_storage_default
on #1775842: [meta] Convert all variables to state and/or config systems. Is this a mistake on that issue? If so let's update it.Grep of HEAD shows:
Assuming this is legacy and will be replaced later?
Is there already a constant for active storage vs. inactive storage? If not, let's file a followup issue to supply constants rather than using magic integers.
Weird comment, even outside of the trailing comma. Notify the storage backend? Notify it of what? This looks like a hook invocation, so let's document we're invoking
hook_field_storage_create_field()
.I'd put an inline comment above this
else
, something like, "Otherwise, the field is being updated." (Theif
is kind of long and so I had to look a bit to double-check whatif
it was matching.)Edit: Sorry, that is among the more useless dreditor snippets of all time. This is the else for
isNew()
inField::save()
.For extra bonus points, we could factor out protected
saveNew()
andsaveUpdated()
methods. (Could make a nice novice followup, too.)Okay, this is actually my first insight in the whole patch as to what
entity_types
is. It's... something you can set, maybe restrict, when creating the field, but not change on update? And it's empty in the exported config in the tests... I'm trying to fit this in to the idea of adding multiple instances of a single field to multiple entities and bundles, and it doesn't fit, so I must be guessing wrong about what this is.I had to think about this a second. Presumably this is to allow partial updates, so that omitting a setting from the object doesn't delete it on save? Can we add that to the comment here?
I don't quite understand the implications of this hunk; could you clarify?
Can we expand the comment here to indicate it's invoking either
hook_field_create_field()
orhook_field_update_field()
? (That's correct, right?)Can we add an inline comment for this line, the
$instance->delete(FALSE)
? I had to look up what FALSE meant, and once I did, my brain started recursing when I tried to figure this out. ;) Because recursion is exactly why we pass FALSE, right? That seems important.Excellent documentation, thanks!
So, this is where I think a
$this->setDeleted()
might be nice.Does this get called by the storage controller? ...I'll find out soon!
I'd add a comment here identifying what hook(s) are getting invoked here (
hook_field_schema()
).Hunh, why do we have to merge in these empty values? AFAIK the schema API doesn't blow up if they're not set... does it?
It looks like
field_reserved_columns()
returns only'deleted'
. A procedural wrapper to return that seems odd; followup?Yes, please, @todo comment indeed. Let's add that before commit. :)
Also, the fact that we have this getter also makes me question the member property being public.
Same note about adding a comment saying which hook is invoked, for both of these.
I actually don't really know how
ArrayAccess
works and I'm too lazy right now to read up on it, so I'm just going to have faith that these are implemented correctly. :)I still need to finish reviewing
FieldInstance
, and then look at the controller class. Almost done!Comment #198
tim.plunkettIssue for the public properties is over here #1959610: Remove public properties from entity classes
Comment #199
xjm#198 should be fine to address the global properties we're getting from the base class. Can we also reduce the visibility of the field-specific properties in this issue, or would that interfere with the temporary BC? @yched, @swentel, what do you think? :)
Comment #200
yched CreditAttribution: yched commentedSounds fine to me, should not interfere with the BC layer.
I'll remove public access to those properties in a next reroll.
Comment #201
yched CreditAttribution: yched commentedEr, sorry, I think I misunderstood the scope of the proposed change.
We're talking about removing *all* public properties from ConfigEntities ?
Like, $field->cardinality = 3; is forbidden, need to go through $field->set('cardinality', 3) ?
Big eeeew. Discoverability --.
Comment #202
xjmJust a few things about
FieldInstance
that aren't already covered by #198:Patch is in, so we can update these now if we want. :)
Aha. So, not setting
entity_types
means it can be anything, but setting it to an array of one or more entity types restricts it to those types. Cool. So let's document that on the property. :)ID, UUID (caps).
I don't see
$bundle_rename_allowed
documented on the class.Check what about the widget module?
"If no weight is specified, make sure the field sinks to the bottom."
Same as above, let's document the hooks inline for readability.
"Optional" is misspelled, and the coding standards specify that it should be lowercase here, IIRC. Also, the
@param
declaration needs a data type.Comment #203
xjmHm? No no, I meant only the ones I indicated. Stuff that callers have no business messing with, like the entity/bundle/field types, module, unique identifiers, internal flags. :)
Comment #204
BerdirWe'd lose discoverability with interfaces anyway, see #1391694-86: Use type-hinting for entity-parameters and the whole issue there
That said, I'd vote against making those properties protected here as well. Somehow config and normal entities started to drift apart here with protected properties and set/get behavior, even more so with EntityNG, which works quite differently anyway.
Comment #205
xjmI think folks are misunderstanding--I definitely think the configurable field settings should be public.
Comment #206
yched CreditAttribution: yched commentedOK, fine by me then. I had the impression that removing all public properties was what patch in #1959610: Remove public properties from entity classes was doing.
Comment #207
yched CreditAttribution: yched commentedKeeping up with HEAD practices :
moved to \Drupal methods for state(), config(), moduleHandler()
Comment #208
plachThe
Drupal
class is in the global namespace: justDrupal::state()
without the leading backslash will work in procedural code.Comment #208.0
plachUpdated issue summary.
Comment #209
yched CreditAttribution: yched commentedAdresses the rest of the comments in @xjm's #188 (from 22. on).
Mostly answers, minor code changes.
22.
Right, current tests do not check the case of "several instances for a field".
Opened #1960574: Add more tests for Field config import and added it to the issue summary.
23
Yes. A field is inactive when either its field type module or storage module is not available. Technically, this is derived (state) data rather than direct config. But figuring the proper way to do that will be a followup. This area is likely to change quite a bit when moving field types to plugins.
24.
false --> '0' in sample / default config files.
True, fixed.
Hum. A quick search shows core has tens of those :-/.
25. 26.
entity_types: { }
That's a list of entity types where the field is allowed to have instances in. Empty means no restriction.
This is used for cases like "avoid proposing site admins to add the 'comment_body' field to their nodes"...
settings: { }
That's a (non serialized) array of settings specific to the "plugin" at hand : field type or storage backend in the case of fields. fieldd_sql_storage happens to have no settings of its own.
27.
- The filename contains the human readable field ID because it's much easier to navigate. Technically, the info that's needed to reference the field is the field UUID though, and that's critically important for deleted fields and instances (there can be several deleted fields with the same "name").
Definitions of deleted fields and instances are stored in state rather than config, but the exact same underlying format is used.
There might be a way to be more human friendly in config files and store the field ID there (if an instance is in config, it's necessarily an instance of a non-deleted field, and referring to it by it's ID is unambiguous). If that's deemed important, I'd much rather explore that in a followup, since that would mean two different formats for when the definition is in config() or state().
- 'entity type' and 'bundle' are needed in the filename to serve as namespaces and avoid clashes, and also as properties in the file for EFQ queriability.
UUIDs are the base information for answering "is this an 'update of existing' or a 'delete existing and create new' ?" during imports.
AFAIK, this is handled at the config_import level, though, not for ConfigEntities to bother with.
28. 29.
True, but I'm not sure there's a question ;-)
Comment #210
yched CreditAttribution: yched commented@plach #208 : ah, true. Fixed.
Comment #211
xjmWowza. Commented on that issue. :)
Okay, so: Field just uses the default config storage controller, and its deletion method also deletes instance definitions for the field's instances. ATM this class just exists to ensure that config doesn't try to re-delete the instances based on the fact that they were also missing on the import. Makes sense.
I worried a bit about something horrible happening if someone accidentally stages a corrupt manifest and then manages to explode all their field data that way. Do we have any mechanism in the Configuration system for warning someone that action X deletes data Y? Or maybe it's up to the module to implement an import hook to provide this warning? Might be worth a followup.
Per http://drupal.org/node/608152, we should always typehint an interface, not a class.
Alright... I'm done! :D I'll give yched a chance to catch up with all my remarks, and meanwhile I'm going to test this patch manually. :) Thereafter, I'll try to distill what remaining followups might be needed, what the API changes are, and a summary of the patch for committers.
Comment #212
xjmAlright, this makes sense, even if it's not obvious. I'd definitely agree on doing a followup and not attempting to address that here.
Comment #213
yched CreditAttribution: yched commentedI think the config import UI screen presents you with a list of what's going to be created / updated / deleted. Not sure what additional notification might be proposed on top of that. Maybe deletes should be made more prominent / red-ish.
Differenciating several kinds of deletes seems a bit tricky - like : deletion of an image style is not deemed important enough that we nag the user about it, but deletion of a field goes through an additional confirm form ? On what basis
I guess an extra safety layer could be : when importing a set of config files where an entry is absent in the manifest but the config file is still present, assume something's wrong and stop there ? But the manifest is precisely what lists config files, so it's a bit of a catch-22...
Probably not a discussion for this thread :-)
Comment #214
xjmLooks like this and #1374116: Move bundle CRUD API out of Field API will conflict.
Comment #215
yched CreditAttribution: yched commented- #188 point 2 was left unanswered
Double checked, that was a leftover. Code and tests are correct. The field definition can specify custom storage indexes for some of the field 'columns', and those are merged with the set of indexes hardcoded by the field type's hook_field_schema().
The definition only stores the "custom" ones, and the tests check that the merge happens correctly.
Removed the @todo.
- #190
In current HEAD, this info is collected when the field definition is loaded, and added in a 'details' entry in the middle of $field['storage'], whose content otherwise comes straight from the field definition.
Patch changes that:
- That info is accessed through a $field->getStorageDetails() method. Cleaner separation from the 'definition' (this is data derived from the definition, not part of the definition itself), and only computed when/if actually needed (most page views don't need it).
- For code that hasn't been converted to the object syntax yet, the ArrayAccess BC layer accepts $field['storage details'] and routes it to $field->getStorageDetails(). The name of the property itself is not too important, since everything will switch to the method syntax in the end, but right, 'storage_details' (without a space) makes more sense. Fixed.
- #211
That would be an issue for ConfigStorageController, this is an override of its method, and needs to respect the same signature. All other existing overrides of import[Create|Change|Delete]() in core typehint this way too. Besides, Config itself implements no interface that could be used for hints.
Opened #1961684: ConfigEntityStorage::import[Create|Change|Delete]() type hint on Config class.
- Additionally, changed some code in CrudTest::testCreateField(): read from raw config (as the test currently does in HEAD) rather than from a loaded ConfigEntity, and added a word of explanation.
Moving on to #191.
Comment #217
xjmI did a bunch of manual testing. It works!! It totally works. I started by changing some basic things and then deploying them:
All of the above worked great. Next, I started trying to break things the best way I knew how:
So, uh, the TLDR for that list is, the taxonomy field needs to also export the UUID for its default value so that taxonomy can sanity-check and maybe even correct the default term for the field on import. I'm trying to decide if that's in scope here--on the one hand, it's a bug that doesn't exist before this patch. On the other, though, it's also not a regression, and a casual test seemed to indicate ER does not have the same bug, so with #1847596: Remove Taxonomy term reference field in favor of Entity reference in the works... We'll probably want a maintainer temperature check on whether this can be a followup, since it's after feature freeze.
On a roll breaking things with default term values, I tried to see how #1140188: Fatal errors during or after adding default values for autocomplete widgets would manifest with this patch. This was really neat:
Not sure why it keeps disappearing: http://drupal.org/files/wat.png
I also tested the following scenario:
This (properly) throws this exception:
This is doing exactly what it should--preventing data loss by disallowing us from importing a change we shouldn't be able to make--but it's not too user-friendly. We might want to consider how best to handle these situations in CMI from a UX standpoint.
The failed import also resulted in the bad field getting "stuck" on the config synch page, and I was even able to get a fatal and then a permanent warning stuck on my site by deleting the same field in dev and staging the manifest for it. I was able to solve the problem by just clearing out the staging directory (which I imagine is going to be up there with "clear your cache"), but I'll do further testing to see if there's something Field API or CMI could handle in the situation where the manifest and staged config entities don't match.
Okay phew! That's all for now. I need a break. :) I'll try some more thorough testing later.
Comment #218
webchickSo first of all, HOLY FLAMING MONKEY BALLS that everything that was tested in #217 and worked actually worked. That's AMAZING!! :D
xjm and larowlan and I talked the part that didn't work some on IRC. It sounds like parts of fixing this might be easy, other parts not. xjm brain-dumped the following:
Additionally, larowlan found #1757452: Support config entities in entity reference fields which is a pre-existing bug, not the fault of this patch.
So the end result is I think that getting Field API => CMI conversion into HEAD so it can be more widely tested, even if * Reference fields aren't perfect, is probably more critical than holding up this entire patch on making everything perfect. I've escalated the other bug to major in response (though it might be critical). Lee said he'd take a look.
In other words, carry on. ;)
Comment #219
yched CreditAttribution: yched commentedReference to a taxo tid inside "default value" being broken after deployment on a different environment :
Sure - that is also true of a view that includes a filter configured on a specific term ;-).
This is a much larger problem space than just Fields: config entities referencing content entities - i.e cross-references between things that are not deployed (if deployed at all) through the same mechanisms.
And right, I'm not sure whether we have plans for how to address this - this, and stuff like "how do we react when we do find a reference is broken ? Cancel the whole import ?" (which would be very hard)
More generally, one of the expectations on this "Field / CMI" task (aside from, well, getting it done) was precisely that once it's in, it would serve as a life-size test-proof for the CMI APIs and workflows, and identify rough edges on non theorical use cases. Now that the patch nears a final state, this is precisely what's happening, great :-). But once such issues are identified, let's make sure the discussions happen in the proper place and not buried here :-)
Comment #220
yched CreditAttribution: yched commentedAdded more thoughts on #1757452: Support config entities in entity reference fields
Comment #221
xjmRight, of course. :) But it's also important to raise the issues before commit here, even when we decide to file them as followups. That's why I posted the manual testing review I did in #217 and then immediately pinged @webchick. That comment should lead to probably three separate followup issues that I was too burned out to file after testing CMI for 4 hours.
Comment #222
yched CreditAttribution: yched commented- Fixed test failures (FieldSqlStorageTest was a leftover in the 'storage details' / 'storage_details' rename, the others for CustomBlock look like non-reproducible hiccups)
- Rerolled after #1374116: Move bundle CRUD API out of Field API
No useful interdiff, due to the merge.
Comment #224
xjm#222: field_CMI-1735118-222.patch queued for re-testing.
Comment #224.0
xjmadd followup issue
Comment #225
yched CreditAttribution: yched commentedStill working on #191, but moving slowly, limited time :-/.
Posting what I have for now.
I started by moving/adapting the existing docs about the former $field array struct that was provided in field.module so far.
Still need to do the same for $instance properties.
Then I'll recheck @xjm feedback and adjust accordingly if needed.
[edit: and "patch size equals number of comments" still holds :-p]
Comment #225.0
yched CreditAttribution: yched commentedAdded 1946404 in followup
Comment #225.1
swentel CreditAttribution: swentel commentedUpdated issue summary.
Comment #226
yched CreditAttribution: yched commented@swentel and I moved / adjusted the documentation for $instance properties.
Didn't process #191 yet.
Sad that this is moving so slow, but client-busy schedule right now, and addressing all the review items / getting a task this size right *in one patch* is daunting - and somewhat soul crushing, TBH.
Comment #228
xjmThank you thank you thank you. The docs in #226 looks excellent, and answer a bunch of my questions from #191 and elsewhere.
Also, @yched, please don't hesitate to spin off followup issues for anything you think should be out of scope, like the public/protected thing, or the bit about the defaults for the Field and FieldInstance constructor. I'll file issues for the things I found in #217, which @webchick agreed can be non-blocking. Finally, let me know if you'd like me to take care of some of the minor docs and code style fixes I listed above, and if so, when would be good to change them. If you give me access to the sandbox, I can push those fixes to a separate branch for you to merge in whenever you want. Please don't be soul-crushed. :)
Comment #229
xjm@swentel gave me sandbox access, so going to try to reroll #226 and then get some of the dumb cleanups out of the way.
Comment #230
xjmMerged 8.x. Let's see if it works. :)
Comment #230.0
xjmUpdated issue summary.
Comment #230.1
swentel CreditAttribution: swentel commentedUpdated issue summary.
Comment #230.2
swentel CreditAttribution: swentel commentedUpdated issue summary.
Comment #231
xjmAlright, I pushed the attached docs polish and nitpicking to a
field-configentity-xjm
branch. Here's what's been addressed of my reviews:Comment #191
getField()
followup.Comment #196
ArrayAccess
dealies and inCrudTest
. Uh. And probably in other things once the BC is removed?Comment #202
I'll file followups for #211 and #217, as well as the spectacular ways I managed to break my test site this weekend. ;)
Comment #232
xjmBasically, #231 is the Cliff's notes, so don't bother reading anything I posted above that. ;)
Comment #233
xjmAhem, search and replace fail. I'll fix.
Comment #234
xjmComment #235
swentel CreditAttribution: swentel commentedAnswers on #231 - part one
Comment #191
So as far I can see, everything is covered, moving on to the next now.
Comment #235.0
swentel CreditAttribution: swentel commentedUpdated issue summary.
Comment #236
swentel CreditAttribution: swentel commentedAnswers on #231 - part two
Comment #196
if (isset())
statements. I'd leave this as is for now.@yched I couldn't nicely address point 3 from #196 - there has been shuffling around of the code, I guess you have a better insight in this one.
Other than that, this one is tackled too imo, up to the last one!
Comment #237
swentel CreditAttribution: swentel commentedAnswers on #231 - part three
Comment #202
So, I guess there are 2 (maybe 1?) things we still need to figure out, hoping to get @yched's thoughts on those.
Comment #238
swentel CreditAttribution: swentel commentedHere's an updated patch with the latest changes.
Comment #238.0
swentel CreditAttribution: swentel commentedUpdated issue summary.
Comment #239
yched CreditAttribution: yched commentedThanks a ton for summarizing the remaining points, and pushing the simplest fixes yourself, @xjm. This was *extremely* helpful :-).
Cannot access code tonight, but here are some answers in addition to @swentel's latest comments :
- #191 1.
Created #1967106: FieldInstance::getField() method, and added it to the issue summary.
Still todo : add links to that issue next to the "@todo Revisit that in favor of a getField()" comments in FieldInstance.php (two places IIRC)
- #191 10. - field_info_field() call in FieldInstance.php
Right. Additionally, #1967106: FieldInstance::getField() method should reformulate things a bit here.
- #191 6. & 13.
Yeah, I know this $id / $field_name dance is a bit confusing, but this really just is temporary BC.
The two guiding lines in the patch are:
- No API breaks, to keep the patch "small" (<700Kb). This means existing (D7) properties on $field and $instance are still available via the ArrayAcces BC layer.
- No BC compromise on the structure of the CMI files, we aim for the "final" version (well, as far as we currently can tell), to minimize later changes in there, which would be painful for everyone. This means renaming a couple properties, notably :
field_name -> id, following the trend for most ConfigEntities in core to settle on "id" for "the machine name"
field_id (numeric) -> uuid.
So the notion of "field name" will disappear in favor of "field id". But while we have the BC layer, there are some unfortunate collisions between the old names and new names.
- #191 15 - Instance label defaulting to field_name (well, ID ;-)
Yes, this exists in D7. No real-life use cases, but very useful for instances created in tests.
- #196 3
- Absolutely, the schema for the storage tables, defined in hook_field_schema(), depends on $field['settings'], that's totally by design. This merge of default settings is here so that the code in hook_field_schema() doesn't need to do isset($field['settings']['foo']) checks in case the incoming $field definition doesn't specify all the settings. That's nothing new compared to D7 here.
- #196 6.
This is actually a boolean, so I'd say a constant is overkill. Code should probably be fixed to say TRUE / FALSE instead of 0 / 1, through.
- #196 18.
- This is not just about the Schema API (hook_schema(), db_create_table()...) but about any code that might need to access the "field schema" - and there's a lot. Avoid 'undefined index' warnings in client code. Here as well, this is just code moved around, nothing new wrt HEAD / D7.
Comment #240
yched CreditAttribution: yched commentedTo be clearer on the last point :
The "field schema" (as returned by hook_field_schema()) and "the db schema for the storage tables created by the SQL storage backend" are two related but different things. The latter is pure Schema API stuff, and is only consumed for DB DDL stuff, while the former is used by much more various code that needs to know stuff about "what field values look like".
Comment #241
yched CreditAttribution: yched commentedForgot:
#196 19.
Yeah, I actually have no clue what the logic around field_reserved_columns() is. This was introduced by the EFQv2 patch, and the issue has no explanations about that.
But yeah, I tried to see if it could be moved to a method on Field.php, but the consuming code won't allow that.
Comment #242
catchI went through this, not in nearly as much depth as I'd like but overall it looks good. There's the BC layer in there etc. but already follow-ups to remove that and that's much nicer than a 700kb patch.
With the 4% performance regression, that should be handled by #1880766: Preload configuration objects, either way not the fault of this patch in itself.
xjm asked for 'committer temperature check' on deploying a field where the terms are different on dev/staging - IMO that's fine as a follow-up and/or deferred to the taxonomy->entity reference conversion - that's a general problem with CMI files referencing serial entity IDs - things like the front page variable will suffer from it too (i.e. if they reference node/678 and that's different on production or whatever).
One thing that stuck out - I couldn't tell if this is BC layer or a conscious decision to keep the behaviour, not sure why we're not just requiring people to load/change/save for these.
There's going to be loads of followups from this but the sooner we can get it in and start working on those the better I think.
Comment #243
yched CreditAttribution: yched commented@catch: that's BC. field_create_field() should go away in favor of the regular entity_create() syntax (which works already - see code sample in the OP). #1953410: [Meta] Remove field_create_*(), field_update_*() and field_delete_*() in favor of just using the ConfigEntity API
Comment #244
yched CreditAttribution: yched commentedAlso @catch: patch got optimized after the 4% perf hit was reported.
According to subsequent benches, we should be more around 1.5%. See #157 / #160 above.
Comment #245
swentel CreditAttribution: swentel commentedReroll after #1957148: Replace entity_query() with Drupal::entityQuery() got in, trivial merge in options.module
- edit - haha patch has wrong issue number, oh well ..
Comment #246
swentel CreditAttribution: swentel commentedMore documentation updates
Comment #247
swentel CreditAttribution: swentel commentedOk, last one ...
$bundle_rename_allowed
property and aallowBundleRename()
method so we can allow bundle renames.Can we get an RTBC, pretty please ? :)
Comment #249
swentel CreditAttribution: swentel commentedIt's in the details.
Comment #250
swentel CreditAttribution: swentel commentedOne small tweak after talking to xjm on IRC. This should be the final one people! :)
Comment #251
xjmAlright, doing a final review and filing some followups. :)
Comment #252
webchickComment #253
xjmFiled some novice followups:
Non-novice followups coming next. :)
Comment #253.0
xjmAdd followup
Comment #253.1
xjmUpdated issue summary.
Comment #253.2
xjmUpdated issue summary.
Comment #254
xjmOh, and one docs tweak @swentel and I discussed this morning. Also filed #1969136: Move field_reserved_columns() to a static method.
Comment #254.0
xjmUpdated issue summary.
Comment #255
yched CreditAttribution: yched commentedSorry, been kept afk longer than I thought. Thanks for the tidying up & triaging work, folks :-)
Pushed some more clarifications on documentation items pointed by @xjm.
(thus, closing #1969072: Clarify documentation around field storage and schema in Field::save())
Comment #256
xjmLet's ship it. :)
Comment #257
alexpottCommitted to a820153 and pushed to 8.x. Thanks!
Comment #258
yched CreditAttribution: yched commentedWOOOOOOOOOOHOO !
Huge congrats to @swentel, @alexpott & @xjm for the amazing effort !
Now, on to the followups :-), + reviving "field types as plugins".
Comment #258.0
yched CreditAttribution: yched commentedUpdated issue summary.
Comment #259
yched CreditAttribution: yched commentedSo, not sure what to document in the change notice right now, vs. what should wait for the followups / removal of the BC layers.
Potential hitlist :
- field_[CUD]_(field|instance)() deprecated in favor of entity_create() / $entity->save() / $entity->delete()
- $field and $instance are now ConfigEntity objects, properties are accessed through OO syntax, some of them renamed.
(might be affected by #1966008: Decide which field and instance properties should be public/protected)
- New way of doing CRUD on field / instance definitions within update hooks
(needs #1969662: Provide new _update_8000_*() CRUD helpers for Field and Instance definitions)
I guess we want to encourage / enforce newly added code to use the new APIs from now on, so having a complete change notice asap would make sense. OTOH, as pointed above, some of the API changes will be refined in followups.
Comment #260
yched CreditAttribution: yched commentedSide note :
- deleted the temporary field-configentity-xjm branch [edit: and the old 1735118-field-configentity-alexpott branch] from the sandbox.
- keeping the main field-configentity-BC branch around for a while
- @swentel: I'll let you decide whether the previous field-configentity-swentel-properties or field-configentity-swentel can be deleted too ?
Comment #261
xjmFiled #1969698: ConfigEntity::save() should disallow saving ID/UUID conflicts (Field UUID changes can badly corrupt field data). Note that this is not caused by the field conversion; it's a pre-existing bug in HEAD with blocks and views. However, fields make it extra exciting, maybe because of related config entity bug #1944368: React to / force order of imports?
Comment #261.0
xjmnew followup
Comment #262
yched CreditAttribution: yched commentedTrivial followup, should be an easy RTBC : #1969714: Stale class names for FieldInstance in phpdocs
(thus, not adding it to the list in the OP)
Comment #263
swentel CreditAttribution: swentel commented@yched There's an existing change notice out there that just lists the config entity conversions, just adding this one to that should be ok. Also, there's another issue that lists the 'Which tables to drop'.
Can't find the exact node id's at this very moment, big party here ;)
Comment #264
xjmWe also definitely need to document the API change from serial numerical IDs to UUIDs, since I spent about 15 comments and half a phone call with half of OCTO before I figured that bit out.
Also, #1969698: ConfigEntity::save() should disallow saving ID/UUID conflicts (Field UUID changes can badly corrupt field data) has been retitled; turns out it's a field-only bug after all. =/ The pre-existing HEAD featurebug of exported config not including UUIDs just makes it easier to kill your site with that bug.
Comment #265
andypostFiled another major #1970110: Should ConfigEntity::isNew() be based on id or uuid ?
Comment #266
swentel CreditAttribution: swentel commentedAdded to http://drupal.org/node/1818734 and #1860986: Drop left-over tables from 8.x. I'm seeing specific change notices per config entity, so yeah, we'll need a dedicated one for field api too.
Comment #267
andypostSuppose better to have one change notice for field and instance so filed https://drupal.org/node/2012896
It needs to be edited about API changes (removed functions) and ArrayAccess shim
Comment #268
swentel CreditAttribution: swentel commentedUpdated some stuff, also filed https://drupal.org/node/2013431 (file_usage.id column change).
What else do we need to add more ? We still have issues to kill the BC layer and remove functions so we'll have additional change notices after I think (or update this one), so I think we're good now.
Comment #269
yched CreditAttribution: yched commentedYay, thanks !
Adjusted a couple things in https://drupal.org/node/2012896, I think we should be good.
(leaving to critical, since it was the former priority level already)
Comment #270
YesCT CreditAttribution: YesCT commentedhm. tag didn't come off. trying again.
Comment #271
andypost@yched probably 'field_name' in examples should be changed to 'id', is not it?
Comment #272
yched CreditAttribution: yched commented@andypost:
Actually no, the name of the incoming property is still 'field_name' ATM, not 'field_id'.
This was kept that way because the BC layer preserves $instance['field_id'] as "the field UUID".
I feared that the same property name meaning one thing in BC and one other thing in the new API would be confusing.
But true, that's not ideal, and back then I thought we would be able to get rid of the BC layer sooner, and then move to 'field_id' as "the value of $field->id()".
We definitely don't want to be stuck with this "field_name" property for the whole D8 cycle.
Thoughts ?
Comment #273
andypost@yched so here's inconsistency with
_update_8003_field_create_field()
that usesDrupal::config('field.field.' . $field_config['id'])
and no field_name at allComment #274
yched CreditAttribution: yched commented@andypost: yes, the notion of 'field_name' has disappeared from the CMI files, where it's consistently 'id', 'field_id'.
Most config entities in core have settled on 'id' for their id key (accessed by the ->id() method).
No problem here.
The issue is in entity_create('field_instance', $values), where we take "the machine name of the field of which we're creating an instance" as $values['field_name'], while stricly speaking it should be $values['field_id']. This was done for the reasons explained in #272, and maybe it wasn't such a good idea :-/
I think maybe we'd be better off switching to $values['field_id'] in the code that calls entity_create('field_instance').
Could be done as part of the various "get rid of field_CRUD_*() calls" issues, that would mean an intermediate step where FieldInstance::__construct() would accept both 'field_name' and 'field_id'...
Comment #275
yched CreditAttribution: yched commentedFYI: opinions welcome in #2019601: Rename config Field / FieldInstance structures ? :-/
Comment #277
yched CreditAttribution: yched commentedCross linking to #2056405: Let field types provide their support for "default values" and associated input UI, which will be needed to fix "field CMI files for taxo / entity_ref contain local numeric entity ids and break on deploy", raised by @xjm / @webchick in #217 / #218 (sorry, couldn't find a more specific issue for this ?)
Comment #277.0
yched CreditAttribution: yched commentedUpdated issue summary.