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 images Fixed in the patch by changing the file_managed.id column to a varchar

Follow ups

Related configuration issues

Related

CommentFileSizeAuthor
#255 field_CMI-1735118-255.patch235.37 KByched
#255 interdiff.txt1.48 KByched
#254 field-cmi-1735118-254.patch235.26 KBxjm
#254 field-name-interdiff.txt846 bytesxjm
#250 1735118-250.patch235.17 KBswentel
#250 interdiff.txt814 bytesswentel
#249 1735118-249.patch235.06 KBswentel
#249 interdiff.txt636 bytesswentel
#247 1735118-247.patch235.06 KBswentel
#247 interdiff.txt2.47 KBswentel
#246 1735118-246.patch234.79 KBswentel
#246 interdiff.txt6 KBswentel
#245 1957148-245.patch235.32 KBswentel
#238 1735118-238.patch235.33 KBswentel
#238 interdiff.txt2.08 KBswentel
#234 field_cmi-1735118-234.patch235.32 KBxjm
#234 interdiff-234.txt1.22 KBxjm
#231 field_cmi-1735118-231.patch235.25 KBxjm
#231 xjm-field-interdiff.txt11.72 KBxjm
#230 field_cmi-1735118-230.patch233.64 KBxjm
#226 field_CMI-1735118-226.patch419.28 KByched
#226 interdiff.txt14.92 KByched
#225 field_CMI-1735118-225.patch224.26 KByched
#225 interdiff.txt8.29 KByched
#222 field_CMI-1735118-222.patch215.24 KByched
#215 field_CMI-1735118-215.patch215.83 KByched
#215 interdiff.txt8.29 KByched
#210 field_CMI-1735118-210.patch215.26 KByched
#210 interdiff.txt5.37 KByched
#209 field_CMI-1735118-208.patch215.28 KByched
#209 interdiff.txt1.05 KByched
#207 field_CMI-1735118-207.patch215.26 KByched
#207 interdiff.txt17.18 KByched
#192 field_CMI-1735118-192.patch214.97 KByched
#192 interdiff.txt16.72 KByched
#186 field_CMI-1735118-186.patch214.72 KByched
#187 field_CMI-1735118-187.patch214.7 KByched
#187 interdiff.txt3.46 KByched
#185 field_CMI-1735118-185.patch214.64 KByched
#185 interdiff.txt22.33 KByched
#184 field_CMI-1735118-182.patch206.85 KByched
#184 interdiff.txt653 bytesyched
#180 field_CMI-1735118-180.patch206.86 KByched
#180 test-interdiff.txt9.2 KByched
#175 field_CMI-1735118-175.patch203.35 KBswentel
#175 interdiff.txt10.07 KBswentel
#173 field_CMI-1735118-172.patch198 KByched
#170 field_CMI-1735118-170.patch198.22 KByched
#170 interdiff.txt1.89 KByched
#161 field_CMI-1735118-161.patch198.86 KByched
#161 interdiff.txt14.09 KByched
#160 runs.tar_.gz295.53 KBAnonymous (not verified)
#157 field_CMI-1735118-157.patch195.62 KByched
#154 field_CMI-1735118-154.patch195.77 KByched
#154 interdiff.txt10.53 KByched
#149 xhprof-fields.png132.44 KBAnonymous (not verified)
#149 cmi-fields.tar_.gz122.36 KBAnonymous (not verified)
#148 field_CMI-1735118-148.patch197.73 KBswentel
#147 field_CMI-1735118-147.patch202.9 KBswentel
#147 interdiff.txt1.53 KBswentel
#143 field_CMI-1735118-143.patch196.88 KByched
#142 interdiff.txt12.04 KByched
#136 field_CMI-1735118-136.patch196.86 KByched
#134 field_CMI-1735118-134.patch196.96 KByched
#127 field_CMI-1735118-127.patch204.13 KByched
#124 field_CMI-1735118-124.patch203.59 KByched
#124 interdiff.txt16.97 KByched
#121 field_CMI-1735118-121.patch202.18 KByched
#120 field_CMI-1735118-120.patch202.12 KByched
#118 field_CMI-1735118-118.patch201.8 KByched
#117 field_CMI-1735118-117.patch202.88 KByched
#111 field_CMI-1735118-111.patch202.52 KByched
#111 interdiff.txt1.37 KByched
#109 field_CMI-1735118-109.patch202.42 KByched
#102 1735118-102.patch86.27 KBswentel
#102 interdiff.txt4.27 KBswentel
#101 1735118-101.patch86.09 KBswentel
#101 interdiff.txt1.64 KBswentel
#99 1735118-99.patch84.13 KBswentel
#88 1735118-88.patch83.79 KBswentel
#88 interdiff.txt9.89 KBswentel
#87 1735118-87.patch87.16 KBswentel
#87 interdiff.txt2.14 KBswentel
#84 1735118-84.patch85.61 KBswentel
#84 interdiff.txt14.23 KBswentel
#80 1735118-80.patch97.29 KBswentel
#80 interdiff.txt10 KBswentel
#76 74-76.patch6.77 KBalexpott
#76 1735118-76.patch91.29 KBalexpott
#74 1735118-74.patch88.62 KBalexpott
#74 71-74-interdiff.txt30.53 KBalexpott
#71 1735118-71.patch75.54 KBswentel
#70 1735118-70.patch70.33 KBswentel
#69 1735118-69.patch67.68 KBswentel
#67 1735118-67.patch68.14 KBswentel
#65 1735118-65.patch67.98 KBswentel
#62 1735118-62.patch63.8 KBswentel
#59 1735118-59.patch62.41 KBswentel
#58 1735118-58.patch0 bytesswentel
#56 1735118-56.patch61.46 KBswentel
#49 1735118-49.patch58.1 KBswentel
#41 1735118-41.patch54.55 KBswentel
#35 1735118-34.patch45.94 KBswentel
#29 1735118-29.patch50.42 KBswentel
#27 field-api-to-cmi-1735118.26.patch50.59 KBalexpott
#17 field-api-to-cmi-1735118.17.patch45.73 KBlarowlan
#15 field-api-to-cmi-1735118.15.patch45.38 KBlarowlan
#13 field-api-to-cmi-1735118.13.patch45.27 KBlarowlan
#11 drupal-1735118-11.patch43.92 KBtim.plunkett
#9 1735118-9.patch43.21 KBswentel
#7 1735118-7.patch43.17 KBswentel
#1 1735118-1.patch37.72 KBswentel
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

swentel’s picture

Issue tags: +Configuration system
FileSize
37.72 KB

Adding patch - it will absolutely not turn green ever as field deletions need to be converted as well.

chx’s picture

Status: Needs review » Needs work

IMO 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.

yched’s picture

First 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

chx’s picture

You 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.

yched’s picture

Sandbox 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).

swentel’s picture

Status: Needs work » Needs review
FileSize
43.17 KB

Field tests turn green, rerunning to see what the rest says.

swentel’s picture

FileSize
43.21 KB

Expected that, this should fix the installation

tim.plunkett’s picture

FileSize
43.92 KB

#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.

larowlan’s picture

This 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.

larowlan’s picture

This 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'.

larowlan’s picture

Last patch went backwards in terms of test-fails, issues with file field widget.
Attempting alternate approach.

larowlan’s picture

Status: Needs review » Needs work

Ok, there are 35 failed tests in the File Field Widget Test but these tests pass locally - any ideas?

larowlan’s picture

Status: Needs work » Needs review
Issue tags: -Configuration system

#13: field-api-to-cmi-1735118.13.patch queued for re-testing.

larowlan’s picture

Issue tags: +Configuration system

#11: drupal-1735118-11.patch queued for re-testing.

larowlan’s picture

Yep, 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.

vlad.ilyich’s picture

Status: Needs review » Needs work

.

Anonymous’s picture

tried 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:

  field_delete_field('comment_body');

  // Remove variables.
  variable_del('comment_block_count');
  $node_types = array_keys(node_type_get_types());
  foreach ($node_types as $node_type) {
    field_attach_delete_bundle('comment', 'comment_node_' . $node_type);
    ...
  }

those field_attach_delete_bundle() calls never delete any instances, because we do this in field_read_instances():

  if (!$include_deleted) {
    $query->condition('fc.deleted', 0);
    $query->condition('fci.deleted', 0);
  }

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.

swentel’s picture

Hey 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

Anonymous’s picture

Issue summary: View changes

update so that others don't waste their time on this issue

gdd’s picture

Issue summary: View changes

Updated issue summary.

alexpott’s picture

alexpott’s picture

Status: Needs work » Needs review
FileSize
50.59 KB

The 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 :)

Anonymous’s picture

Issue summary: View changes

Added default images

swentel’s picture

FileSize
50.42 KB

The 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)

webchick’s picture

"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!

swentel’s picture

Status: Needs review » Needs work

Ah man, forgot the push some stashes, I'll do that tomorrow.

yched’s picture

Punting 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.

swentel’s picture

Status: Needs work » Needs review

Pushed everything, here goes.

swentel’s picture

FileSize
45.94 KB

And with the patch ...

gdd’s picture

Status: Needs review » Needs work

Its 3:00 am right now but here are some comments based on a quick scan.

+++ b/core/modules/field/field.attach.incundefined
@@ -1276,20 +1276,27 @@ function field_attach_create_bundle($entity_type, $bundle) {
+  $instances = field_read_instances();
+  foreach ($instances as $id => $instance) {
+    if ($instance['entity_type'] == $entity_type && $instance['bundle'] == $bundle_old) {
+      $old_config_identifier = 'field.instance.' . $instance['entity_type'] . '.' . $bundle_old . '.' . $instance['field_name'];
+      $new_config_identifier = 'field.instance.' . $instance['entity_type'] . '.' . $bundle_new . '.' . $instance['field_name'];
+      $config_instance = config($old_config_identifier)->get();
+      $config_instance['bundle'] = $bundle_new;
+      config($old_config_identifier)->delete();
+      field_create_instance($config_instance);
+    }

We have config_rename() committed now so we can clean this up a little.

+++ b/core/modules/field/field.crud.incundefined
@@ -171,23 +172,43 @@ function field_create_field($field) {
+  // @todo temporary (cruel) hack to get the tests green.
+  // if you uninstall the comment module and the forum after being enabled
+  // field_delete_field removes the comment_body and renames the data tables.
+  // Currently, the config file is not deleted so the uuid will be overwritten
+  // because the prior_field check above doesn't care about the deleted ones.
+  // So yes we need to store the deleted status somewhere else so we can
+  // delete the yml file immediately. Either still keep a table with

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.

+++ b/core/modules/field/field.crud.incundefined
@@ -475,6 +497,10 @@ function field_create_instance($instance) {
+  // Generate UUID. We md5 it so the creation of deleted tables can work.

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.

+++ b/core/modules/field/field.installundefined
@@ -381,15 +231,51 @@ function _update_7000_field_create_instance($field, &$instance) {
+  db_drop_table('field_config');
+  db_drop_table('field_config_instance');

<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.

gdd’s picture

Status: Needs work » Needs review
cosmicdreams’s picture

I will attempt to itemize every file that would be effected by this change tonight.

cosmicdreams’s picture

core/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.

swentel’s picture

Assigned: Unassigned » swentel

Working on getting rid of the extremely ugly hacks now re: the deletion of fields/instances.

swentel’s picture

FileSize
54.55 KB

Ok, new patch. Changes:

  • Use the rename() methods
  • Move updates from field_sql_storage_update_8000() to field_update_8001() because that drops the field_config tables (that was actually an exception that didn't got caught until a few days ago).
  • Move the creation of the {config} table very early in the update/upgrade path to get rid of the field_system_info_alter() hack. system_update_8003 is now gone as well
swentel’s picture

Issue summary: View changes

Updated issue summary.

Anonymous’s picture

quick review, overall looking really good.

- maybe something like this for exploding on the config name:

function field_config_import_create($name, $new_config, $old_config) {
  list($module, $type) = explode('.', $name);
  if ($module != 'field') {
    return;
  }
  
  switch ($type) {
    case 'field':
      field_create_field($new_config->get());
      break;
    case 'instance':
      field_create_instance($new_config->get());
      break;
  }
}

- there's lots of variations on this:

config('field.instance.' . $instance->entity_type . '.' . $instance->bundle . '.' . $instance->field_name) ...

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:

/**
 * Refreshes the 'active' and 'storage_active' columns for fields.
 */
function field_sync_field_status() {

  $fields = field_read_fields(array(), array('included_deleted' => 1 ,'include_inactive' => 1));
  // Refresh the 'active' and 'storage_active' columns according to the current
  // set of enabled modules.
  $modules = module_list();
  foreach ($modules as $module_name) {
    $fields = field_associate_fields($module_name, $fields);
  }

  foreach ($fields as $id => $field) {
    $updated = FALSE;
    if (!in_array($field['module'], $modules)) {
      $fields[$id]['active'] = 0;
      $updated = TRUE;
    }
    if (!in_array($field['storage_module'], $modules)) {
      $fields[$id]['storage_active'] = 0;
      $updated = TRUE;
    }
    if ($updated) {
      $updated_ids[] = $id;
    }
  } 
  
  foreach ($updated_ids as $id) {
    // We can not use field_update_field because the prior_field does not
    // check whether a field is really there or not.
    config('field.field.' . $fields[$id]['field_name'])->setData($fields[$id])->save();
  } 

  field_cache_clear(TRUE);
}

- references to 'row' needs to be updated in tests and elsewhere

+    $this->assertFalse($field, 'A field_config row for the field does not exist.');
chx’s picture

i 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.

aspilicious’s picture

It is banned for security reasons but also for just encoding stuff?

chx’s picture

Doesn'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 :(

aspilicious’s picture

So what function can we use to just encode stuff (like needed in this issue)

effulgentsia’s picture

hash('sha256', $string_to_encode)

KarenS’s picture

swentel’s picture

FileSize
58.1 KB

New patch, changes:

  • Mostly chasing HEAD and cleanups (also, thank you file storage patch!).
  • replace md5 with the function in #47 and moved it info a helper function.
  • Clean up the explode() functionality in the hook_config_import_* hooks addressed in #42.
  • Tests (!) for CMI's hook_config_import_create(), hook_config_import_change() and hook_config_import_delete().
swentel’s picture

Issue summary: View changes

Remove the 'rename' and 'field_system_info_alter' hack blockers'

chx’s picture

Status: Needs review » Needs work

First 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:

+++ b/core/modules/field/field.moduleundefined
@@ -768,6 +860,21 @@ function field_cache_clear() {
+  return substr(hash('sha256', $uuid->generate()), 0, 6);

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?

+++ b/core/modules/field/modules/field_sql_storage/field_sql_storage.installundefined
@@ -12,15 +12,15 @@ function field_sql_storage_schema() {
+  module_load_include('module', 'field');

Why are you using module_load_include to load field.module? Just two lines below this, you are using drupal_load.

+++ b/core/modules/file/file.installundefined
@@ -232,3 +232,29 @@ function file_requirements($phase) {
+    'description' => 'The primary key of the object using the file.',

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.

xjm’s picture

// @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.

So 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.

swentel’s picture

I 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.

swentel’s picture

Status: Needs work » Needs review
FileSize
61.46 KB

Checking 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.

swentel’s picture

Status: Needs review » Needs work
FileSize
0 bytes

No clue why ModulesDisabledUpgradePathTest fails on the bot, works fine on my local machine, patch attached fixes the rest though.

swentel’s picture

Status: Needs work » Needs review
FileSize
62.41 KB

meh, nice going.

swentel’s picture

Status: Needs review » Needs work

So both failures don't happen on my local machine, no clue what's going on here. Anyway, focusing on the deleting of fields now.

swentel’s picture

Status: Needs work » Needs review
FileSize
63.8 KB

So, let's see what this one does:

  • Removes all (cruel) hacks I needed previously to get it green (!)
  • Uses the state API to store the deleted fields

Upgrade tests and field api tests are green on my machine, I have a good feeling the rest will follow.

cosmicdreams’s picture

Status: Needs review » Needs work

It looks like the exceptions in other tests are related to some stale code that is either in the EnableDisableTest or related to it.

swentel’s picture

Status: Needs work » Needs review
FileSize
67.98 KB

This 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().

Status: Needs review » Needs work

The last submitted patch, 1735118-65.patch, failed testing.

swentel’s picture

Status: Needs work » Needs review
FileSize
68.14 KB

Forgot 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 :)

swentel’s picture

Status: Needs review » Needs work
FileSize
67.68 KB

Posting 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.

swentel’s picture

FileSize
70.33 KB

More storage cleanup - no review though, still fails on the same test.

swentel’s picture

Status: Needs work » Needs review
FileSize
75.54 KB

Ok, 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.

alexpott’s picture

FileSize
30.53 KB
88.62 KB

First 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.

Anonymous’s picture

Issue summary: View changes

Add link to 1785560

alexpott’s picture

FileSize
91.29 KB
6.77 KB

Here you go... testbot test. :)

alexpott’s picture

So... thank you testbot!

There are a couple of issues that have been highlighted by this conversion. Having drupal_get_schema() depend on entity_get_info() is messy to say the least. (It's dependent because field_sql_storage_schema() uses field_read_fields() to build the schemas)

One of the really tricky issues is due to this code in module_enable():

      $module_config
        ->set("enabled.$module", $weight)
        ->set('enabled', module_config_sort($module_config->get('enabled')))
        ->save();
      $disabled_config
        ->clear($module)
        ->save();
      // Load the module's code.
      drupal_load('module', $module);
      module_load_install($module);

      // Refresh the module list to include it.
      system_list_reset();
      module_implements_reset();
      _system_update_bootstrap_status();
      // Update the kernel to include it.
      // @todo The if statement is here because install_begin_request() creates
      //   a container without a kernel. It probably shouldn't.
      if ($kernel = drupal_container()->get('kernel', ContainerInterface::NULL_ON_INVALID_REFERENCE)) {
        $kernel->updateModules(module_list());
      }
      // Refresh the schema to include it.
      drupal_get_schema(NULL, TRUE);
      // Update the theme registry to include it.
      drupal_theme_rebuild();

      // Allow modules to react prior to the installation of a module.
      module_invoke_all('modules_preinstall', array($module));

      // Clear the entity info cache before importing new configuration.
      entity_info_cache_clear();

      // Now install the module if necessary.
      if (drupal_get_installed_schema_version($module, TRUE) == SCHEMA_UNINSTALLED) {
        drupal_install_schema($module);

Due to this code during the drupal_get_schema() call hook_entity_info_alter()'s are fired before modules are enabled. That's why the patch does:

+++ b/core/modules/node/node.moduleundefined
@@ -194,6 +194,11 @@ function node_cron() {
 function node_entity_info_alter(&$info) {
+  if (!db_table_exists('node_type')) {
+    // entity_get_info() is called in drupal_get_schema now this means this can
+    // be called before the schema is installed.
+    return;
+  }

+++ b/core/modules/taxonomy/taxonomy.moduleundefined
@@ -109,6 +109,11 @@ function taxonomy_permission() {
  */
 function taxonomy_entity_info_alter(&$info) {
+  if (!db_table_exists('taxonomy_vocabulary')) {
+    // entity_get_info() is called in drupal_get_schema now this means this can
+    // be called both the schema is installed.
+    return;
+  }
   foreach (taxonomy_vocabulary_get_names() as $machine_name => $vocabulary) {

+++ b/core/modules/rdf/rdf.moduleundefined
@@ -390,6 +390,11 @@ function rdf_modules_uninstalled($modules) {
 function rdf_entity_info_alter(&$entity_info) {
+  if (!db_table_exists('rdf_mapping')) {
+    // entity_get_info() is called in drupal_get_schema now this means this can
+    // be called before the schema is installed.
+    return;
+  }

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 in field_system_info_alter() and field_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.

alexpott’s picture

One more thing....

+++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/FieldEntity.phpundefined
@@ -0,0 +1,60 @@
+class FieldEntity extends ConfigEntityBase {

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.

alexpott’s picture

andypost’s picture

I 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

+++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/Bundle.phpundefined
@@ -0,0 +1,60 @@
+ *   id = "bundle",
...
+class Bundle extends ConfigEntityBase {

+++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/FieldEntity.phpundefined
@@ -0,0 +1,60 @@
+ *   id = "fieldentity",
...
+class FieldEntity extends ConfigEntityBase {

+++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/Instance.phpundefined
@@ -0,0 +1,60 @@
+ *   id = "instance",
...
+class Instance extends ConfigEntityBase {

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

+++ b/core/modules/field/tests/modules/field_test_config/config/field.field.field_test_import.ymlundefined
@@ -0,0 +1,53 @@
+id: field_test_import

We have field_test module and field so any reason to make another one?

+++ b/core/modules/taxonomy/taxonomy.moduleundefined
@@ -109,6 +109,11 @@ function taxonomy_permission() {
+  if (!db_table_exists('taxonomy_vocabulary')) {

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

andypost’s picture

Issue summary: View changes

Adding Entity translation UI problem.

swentel’s picture

FileSize
10 KB
97.29 KB

Chasing 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

swentel’s picture

So 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

sun’s picture

Sorry, but no, the manifest file is not to be abused as an config object index or lookup facility.

yched’s picture

@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.

swentel’s picture

FileSize
14.23 KB
85.61 KB

New 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

moshe weitzman’s picture

Status: Needs review » Needs work

This 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.

+++ b/core/modules/system/lib/Drupal/system/Tests/Upgrade/FieldUpgradePathTest.php
@@ -0,0 +1,46 @@
+/**
+ * Tests upgrade of system variables.
+ */

Wrong Doxygen

moshe weitzman’s picture

Issue summary: View changes

We are converting to config entity already in the patch

swentel’s picture

Status: Needs work » Needs review
FileSize
2.14 KB
87.16 KB

This 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.

swentel’s picture

FileSize
9.89 KB
83.79 KB

Chasing HEAD, especially with the user picture conversion. Removed all references to '@fielcmi' as well.

mbrett5062’s picture

Status: Needs review » Needs work

Not 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'?

yched’s picture

Right, but I'd vote for 'field' & 'field_instance'.

mbrett5062’s picture

Sounds 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'.

Berdir’s picture

field and field_instance sounds good to me as well, +1.

swentel’s picture

field would be nice yes, but that currently clashes with hook_field_load, etc, see #78

mbrett5062’s picture

@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.

yched’s picture

@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.

yched’s picture

Issue summary: View changes

Entity UI translation problem is gone.

chx’s picture

What happened to this issue...?

swentel’s picture

We'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.

swentel’s picture

Status: Needs work » Needs review
FileSize
84.13 KB

Let's see how this one turns out, the upgrade path should be working - sorry for not having an interdiff.

Status: Needs review » Needs work

The last submitted patch, 1735118-99.patch, failed testing.

swentel’s picture

Status: Needs work » Needs review
FileSize
1.64 KB
86.09 KB

Should 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.

swentel’s picture

FileSize
4.27 KB
86.27 KB

Green - 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 ?

swentel’s picture

Issue summary: View changes

Updated issue summary.

Status: Needs review » Needs work

The last submitted patch, 1735118-102.patch, failed testing.

Anonymous’s picture

Issue summary: View changes

Updated issue summary.

sun’s picture

+++ b/core/modules/field/tests/modules/field_test_config/config/field.field.field_test_import.yml

id: field_test_import
...
name: ''
...
  field_name: field_test_import

1) 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.

+++ b/core/modules/field/tests/modules/field_test_config/config/field.field.field_test_import.yml

+  storage:
...
+    active: 1
+    details:
+      sql:
+        FIELD_LOAD_CURRENT:
+          field_data_field_test_import:
...
+  active: 1
+  columns:
+    value:
+      type: varchar
...
+  'foreign keys':
...
+  indexes:

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.

+++ b/core/modules/field/tests/modules/field_test_config/config/field.instance.node.test_import.field_test_import.yml

+name: ''
+label: node.test_import.field_test_import
+data:
+  field_name: field_test_import
+  entity_type: node
+  bundle: test_import
+  label: 'Test import field'
+  widget:
...
+  settings:
...
+  required: 0
+  description: ''

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?

swentel’s picture

@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 ....

aspilicious’s picture

I would make it a 2 step process. First get the cmi conversion in and than put everything again in a sane position.

andypost’s picture

I think bout uri() for Instance - and sun's idea is great

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.

Working 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

webchick’s picture

Priority: Normal » Major

This feels like at least major; until this is done, we haven't really battle-tested the API.

yched’s picture

Status: Needs work » Needs review
FileSize
202.42 KB

So, 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 :

entity_create('field_instance', array(
    'field_name' => 'field_foo',
    'entity_type' => 'node',
    'bundle' => 'article',
    'required' => TRUE,
  ))
  ->save();

- 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 :-)

Status: Needs review » Needs work

The last submitted patch, field_CMI-1735118-109.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
1.37 KB
202.52 KB

Drats. As could be expected, my last minute refactoring in the update function was broken :-/
Should be better now.

yched’s picture

Pushed minor commits:
98d3a3a Fix possible bug on importDelete()
b3824f0 Add comment about why importCreate() works as is

alexpott’s picture

Awesome work guys!

+++ b/core/modules/field/field.attach.incundefined
@@ -1549,11 +1549,17 @@ function field_attach_create_bundle($entity_type, $bundle) {
+  $instances = field_read_instances();
+  foreach ($instances as $id => $instance) {
+    if ($instance['entity_type'] == $entity_type && $instance['bundle'] == $bundle_old) {
+      $new_instance_id = $instance['entity_type'] . '.' . $bundle_new . '.' . $instance['field_name'];
+      config('field.instance.' . $instance['entity_type'] . '.' . $bundle_old . '.' . $instance['field_name'])->rename('field.instance.' . $new_instance_id);
+      config('field.instance.' . $instance['entity_type'] . '.' . $bundle_new . '.' . $instance['field_name'])
+        ->set('bundle', $bundle_new)
+        ->set('id', $new_instance_id)
+        ->save();
+    }
+  }

This should be using config's rename function. Atm it seems you might leave the old config around.

+++ b/core/modules/field/field.api.phpundefined
@@ -1055,7 +1055,7 @@ function hook_field_attach_delete_revision(\Drupal\Core\Entity\EntityInterface $
 function hook_field_attach_purge(\Drupal\Core\Entity\EntityInterface $entity, $field, $instance) {
...
-  if ($entity->entityType() == 'node' && $field->field_name == 'my_field_name') {
+  if ($entity->entityType() == 'node' && $field['field_name'] == 'my_field_name') {

Very minor nit... this change is unnecessary

+++ b/core/modules/field/field.crud.incundefined
@@ -542,103 +262,26 @@ function field_create_instance(&$instance) {
+    // Override settings
+    foreach ($instance as $key => $value) {
+      if (is_array($value)) {
+        $instance_loaded[$key] = $value;
+      }
+      else {
+        $instance_loaded[$key] = $value;
+      }
+    }
+    $instance = $instance_loaded;

This looks like it can be simplified. No need for the if (is_array($value)) { :)

+++ b/core/modules/field/field.info.ymlundefined
@@ -3,6 +3,4 @@ description: 'Field API to add fields to entities like nodes and users.'
-dependencies:
-  - field_sql_storage

Awesome work to remove this very weird circular dependency...

+++ b/core/modules/field/field.installundefined
@@ -229,12 +81,16 @@ function _update_7000_field_create_field(&$field) {
+  field_sql_storage_field_storage_create_field($field_entity);

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()

yeah, I don't think people want to be wondering "oh, are fields on CMI yet when my upgrade runs"
@yched

+++ b/core/modules/field/field.installundefined
@@ -382,15 +237,14 @@ function _update_7000_field_create_instance($field, &$instance) {
+function field_update_dependencies() {
+  // Convert to config after SQL storage has been updated.
+  $dependencies['field_sql_storage'][8000] = array(
+    'field' => 8002,
+  );
+  return $dependencies;
 }

Think we need to declare that block_update_8008 and user_update_8011 need to run before field_update_8002

+++ b/core/modules/field/field.installundefined
@@ -484,6 +338,113 @@ function field_update_8002() {
+        ->setData($config)

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.

+++ b/core/modules/field/field.installundefined
@@ -484,6 +338,113 @@ function field_update_8002() {
+        ->setData($config)

As above... this a tricky issue that would be resolved if we didn't cast everything to strings!

+++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/Field.phpundefined
@@ -0,0 +1,464 @@
+      $prior_field = field_read_field($this->field_name, array('include_inactive' => TRUE));

Hmmm... can't we use config here just to check whether or not we have a configuration object already?

+++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/Field.phpundefined
@@ -0,0 +1,464 @@
+          field_delete_instance($instance, FALSE);

This should be $instance->delete(FALSE);

+++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/FieldInstance.phpundefined
@@ -0,0 +1,377 @@
+      if (field_read_instance($this->entity_type, $this->field_name, $this->bundle)) {

Hmmm... can't we use config here just to check whether or not we have a configuration object already?

+++ b/core/modules/field/lib/Drupal/field/Tests/CrudTest.phpundefined
@@ -219,12 +218,6 @@ function testReadFields() {
-    $fields = field_read_fields(array('entity_type' => $instance_definition['entity_type'], 'bundle' => $instance_definition['bundle']));
-    $this->assertTrue(count($fields) == 1 && isset($fields[$field_definition['field_name']]), 'The field was properly read.');
-    $fields = field_read_fields(array('entity_type' => $instance_definition['entity_type'], 'field_name' => $instance_definition['field_name']));
-    $this->assertTrue(count($fields) == 1 && isset($fields[$field_definition['field_name']]), 'The field was properly read.');

Does this not work now? Or are these test pointless?

+++ b/core/modules/field/lib/Drupal/field/Tests/CrudTest.phpundefined
@@ -303,8 +300,8 @@ function testDeleteField() {
-    $field = field_read_field($this->field['field_name'], array('include_deleted' => TRUE));
-    $this->assertTrue(!empty($field['deleted']), 'A deleted field is marked for deletion.');
+    $deleted_fields = state()->get('field.field.deleted');
+    $this->assertTrue(isset($deleted_fields[$field['uuid']]), 'A deleted field is marked for deletion.');

Not sure why this change is necessary...

+++ b/core/modules/field/lib/Drupal/field/Tests/CrudTest.phpundefined
@@ -351,17 +348,6 @@ function testDeleteField() {
-  function testUpdateNonExistentField() {
-    $test_field = array('field_name' => 'does_not_exist', 'type' => 'number_decimal');
-    try {
-      field_update_field($test_field);
-      $this->fail(t('Cannot update a field that does not exist.'));
-    }
-    catch (FieldException $e) {
-      $this->pass(t('Cannot update a field that does not exist.'));
-    }
-  }

This test still seems valid looking at field_update_field(). Do we need to remove it?

+++ b/core/modules/field/lib/Drupal/field/Tests/FieldImportCreateTest.phpundefined
@@ -0,0 +1,86 @@
+use Symfony\Component\Yaml\Parser;

Unnecessary

+++ b/core/modules/field/lib/Drupal/field/Tests/FieldImportDeleteTest.phpundefined
@@ -0,0 +1,87 @@
+    config_install_default_config('module', 'field_test_config');

Should use DrupalUnitTestBase::installConfig()

+++ b/core/modules/field/lib/Drupal/field/Tests/FieldInfoTest.phpundefined
@@ -136,22 +136,17 @@ function testFieldPrepare() {
+    config('field.field.' . $field_definition['field_name'])
+      ->set('settings', array())
+      ->save();
+    field_info_cache_clear();

Using config and not Config entity system... whereas below...

+++ b/core/modules/field/lib/Drupal/field/Tests/FieldInfoTest.phpundefined
@@ -173,25 +168,18 @@ function testInstancePrepare() {
+    $instance = entity_load('field_instance', $instance_definition['entity_type'] . '.' . $instance_definition['bundle'] . '.' . $instance_definition['field_name']);
+    $instance['settings'] = array();
+    $instance['widget']['settings'] = 'unavailable_widget';
+    $instance['widget']['settings'] = array();
+    field_update_instance($instance);

Uses config entity system... seems inconsistent

+++ b/core/modules/field/lib/Drupal/field/Tests/FieldInstanceCrudTest.phpundefined
@@ -48,26 +49,24 @@ function setUp() {
-    // Read the raw record from the {field_config_instance} table.
-    $result = db_query('SELECT * FROM {field_config_instance} WHERE field_name = :field_name AND bundle = :bundle', array(':field_name' => $this->instance_definition['field_name'], ':bundle' => $this->instance_definition['bundle']));
-    $record = $result->fetchAssoc();
-    $record['data'] = unserialize($record['data']);
+    // Read the configuration.
+    $instance = entity_load('field_instance', $this->instance_definition['entity_type'] . '.' . $this->instance_definition['bundle'] . '.' . $this->instance_definition['field_name']);

Should we be using config here... to read the raw config?

+++ b/core/modules/field/lib/Drupal/field/Tests/TranslationTest.phpundefined
@@ -249,9 +249,9 @@ function testTranslatableFieldSaveLoad() {
-    $field = $this->field;
+    $field = $this->field_definition;

This might be clearer if it was $field_definition = $this->field_definition;

+++ b/core/modules/simpletest/lib/Drupal/simpletest/Tests/DrupalUnitTestBaseTest.phpundefined
@@ -108,10 +108,7 @@ function testEnableModulesInstall() {
-    // @todo field_sql_storage and field should technically not be necessary
-    //   for an entity query.

Need to keep part of this @todo only the field stuff is being removed.

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Tests/TermFieldTest.phpundefined
@@ -53,7 +53,7 @@ function setUp() {
+    $this->field = field_create_field($this->field);

should be $this->field = field_create_field($this->field_defintion); to be consistent with other tests.

+++ b/core/modules/field/lib/Drupal/field/FieldInstanceStorageController.phpundefined
@@ -0,0 +1,30 @@
+ * Contains Drupal\field\FieldInstanceStorageController.

Minor. Should have a leading \ as in \Drupal\field\FieldInstanceStorageController

+++ b/core/modules/field/lib/Drupal/field/FieldStorageController.phpundefined
@@ -0,0 +1,17 @@
+ * Contains Drupal\field\FieldStorageController.

As above

+++ b/core/modules/field/lib/Drupal/field/Tests/FieldImportChangeTest.phpundefined
@@ -0,0 +1,68 @@
+ * Definition of Drupal\field\Tests\FieldImportChangeTest.

Should be Contains \Drupal\field\Tests\FieldImportChangeTest

+++ b/core/modules/field/lib/Drupal/field/Tests/FieldImportCreateTest.phpundefined
@@ -0,0 +1,86 @@
+ * Definition of Drupal\field\Tests\FieldImportCreateTest.

Should be Contains \Drupal\field\Tests\FieldImportCreateTest

+++ b/core/modules/field/lib/Drupal/field/Tests/FieldImportDeleteTest.phpundefined
@@ -0,0 +1,87 @@
+ * Definition of Drupal\field\Tests\FieldImportDeleteTest.

Should be Contains \Drupal\field\Tests\FieldImportDeleteTest

+ * Note: the class take no special care about importing instances after their
+ * field in importCreate(), since this is guaranteed by the alphabetical order
+ * (field.field.* entries are processed before field.instance.* entries).

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 :)

alexpott’s picture

Issue summary: View changes

Updated issue summary.

Berdir’s picture

Ok, 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:

$ ls -l . | wc -l
1606
$ ls -l field.* | wc -l
859
$ ls -l field.field.* | wc -l
290

:)

Berdir’s picture

Issue summary: View changes

Added list of follow ups.

swentel’s picture

Thanks Alex and Berdir!

Quick question for you Alex:

This should be using config's rename function. Atm it seems you might leave the old config around.

We are using rename() there no ? Or am I completely overlooking something ?

alexpott’s picture

Doh! You're right... somehow just missed the rename()... so it's me doing the overlooking...

yched’s picture

FileSize
202.88 KB

For now, just reroll after #1942000: Node NG broke Edit module (conflicts in Drupal\edit\Tests\EditTestBase)

yched’s picture

FileSize
201.8 KB

@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.

Status: Needs review » Needs work

The last submitted patch, field_CMI-1735118-118.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
202.12 KB

Silly 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 :

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.

This sounds like a bug in setData() to me ?
$config->setData($array) !== foreach ($array as $key => $value) {$config->set($key, $value);} sure is misleading...

yched’s picture

FileSize
202.18 KB

Pushed 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.

yched’s picture

Actually 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 !

Berdir’s picture

Will 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.

yched’s picture

FileSize
16.97 KB
203.59 KB

Pushed 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.

yched’s picture

One last change is being worked on in #1906218: Helper issue for "Field CMI", and after that we should be good to go !

webchick’s picture

Priority: Major » Critical

According 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.

yched’s picture

FileSize
204.13 KB

Ok, 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.

yched’s picture

Issue summary: View changes

Updated issue summary.

yched’s picture

Issue summary: View changes

Update for "ready to review" patch.

yched’s picture

As 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]

yched’s picture

Issue summary: View changes

typo

xjm’s picture

I reviewed up through file.install (more later). Few comments:

  1. +++ b/core/modules/file/file.installundefined
    @@ -234,6 +234,21 @@ function file_requirements($phase) {
    +  // The update hook to convert the id column in the column table needs to be
    +  // the first one because user update hooks will be called during system
    +  // update hooks due to other dependencies and the user picture conversion
    +  // might need to save a default image and the field it's stored in, does
    +  // not have an integer id anymore.
    

    Holy run-on sentence Batman. Also id => ID.

  2. +++ b/core/modules/file/file.installundefined
    @@ -245,3 +260,17 @@ function file_update_8000() {
    + * Convert the id column in file_usage to store UUID's.
    + */
    +function file_update_8001() {
    

    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.

  3. +++ b/core/modules/file/lib/Drupal/file/Tests/FileFieldWidgetTest.phpundefined
    @@ -205,7 +205,7 @@ function testMultiValuedWidget() {
    -    $field_name = strtolower($this->randomName());
    +    $field_name = 'test_file_field';
    
    +++ b/core/modules/file/lib/Drupal/file/Tests/FilePrivateTest.phpundefined
    @@ -38,7 +38,7 @@ public function setUp() {
    -    $field_name = strtolower($this->randomName());
    +    $field_name = 'test_file_field';
    
    +++ b/core/modules/file/lib/Drupal/file/Tests/FileTokenReplaceTest.phpundefined
    @@ -31,7 +31,7 @@ function testFileTokenReplacement() {
    -    $field_name = 'field_' . strtolower($this->randomName());
    +    $field_name = 'test_file_field';
    

    Why? (And elsewhere.)

  4. +++ b/core/modules/forum/forum.installundefined
    @@ -22,10 +22,6 @@ function forum_install() {
    -  // If we enable forum at the same time as taxonomy we need to call
    -  // field_associate_fields() as otherwise the field won't be enabled until
    -  // hook modules_enabled is called which takes place after hook_enable events.
    -  field_associate_fields('taxonomy');
    

    Why?

  5. +++ b/core/modules/forum/forum.installundefined
    @@ -55,7 +51,7 @@ function forum_enable() {
    -  if (!field_info_field('taxonomy_forums')) {
    +  if (!field_read_field('taxonomy_forums', array('include_inactive' => TRUE))) {
    

    There isn't an API change with include_inactive from D7; why is this change necessary?

  6. +++ b/core/modules/translation_entity/translation_entity.admin.incundefined
    @@ -6,20 +6,21 @@
    - * @param \Drupal\Field\FieldInstance $instance
    + * @param Drupal\field\Plugin\Core\Entity\FieldInstance $instance
    

    Missing preceding slash.

  7. +++ b/core/modules/user/user.installundefined
    @@ -716,7 +712,7 @@ function user_update_8011() {
    -  _update_7000_field_create_field($field);
    +  $field = _update_7000_field_create_field($field);
    

    Why aren't we also deleting the previous definition of $field here?

  8. +++ b/core/modules/views/lib/Drupal/views/Tests/Plugin/RelationshipJoinTestBase.phpundefined
    @@ -27,7 +27,6 @@
         $this->installSchema('user', array('users', 'users_roles', 'role_permission'));
    -    $this->installSchema('field', array('field_config', 'field_config_instance'));
         $this->installConfig(array('user'));
         parent::setUpFixtures();
     
    

    Mmmm, does this test still provide the intended coverage? Shouldn't we be installConfig()ing? Ditto may places elsewhere in the patch.

effulgentsia’s picture

Here's some minor stuff that jumped out at me:

+++ b/core/modules/block/block.install
@@ -263,7 +263,7 @@ function block_update_8008() {
-    _update_7000_field_create_field($body_field);
+    $body_field = _update_7000_field_create_field($body_field);

Should this be $body_field_definition?

+++ b/core/modules/comment/comment.install
@@ -16,7 +16,7 @@ function comment_uninstall() {
-    field_attach_delete_bundle('comment', 'comment_node_' . $node_type);
+    field_attach_delete_bundle('comment', 'comment_node_' . $node_type, FALSE);

Why are we passing FALSE here? This can leave unused fields around: do we want that?

+++ b/core/modules/datetime/lib/Drupal/datetime/Tests/DateTimeFieldTest.php
@@ -281,13 +281,15 @@ function testDatelistWidget() {
+    $this->field = $this->field_definition;

Huh? Don't we want to distinguish between a field definition array and a field object? Doesn't this blur that distinction?

+++ b/core/modules/field/field.crud.inc
@@ -48,157 +50,16 @@
+  $field = entity_create('field_entity', $field);

Should the 2nd $field be $field_definition instead?

+++ b/core/modules/field/field.crud.inc
@@ -342,68 +143,58 @@ function field_read_field($field_name, $include_additional = array()) {
+    foreach ($deleted_fields as $id => $config) {
+      $fields[$id] = new Field($config, 'field_entity');
+    }

Do we have a follow up for removing the hard-coded class name here?

+++ b/core/modules/field/field.crud.inc
@@ -686,82 +330,87 @@ function field_read_instance($entity_type, $field_name, $bundle, $include_additi
+    foreach ($deleted_instances as $id => $instance) {
+      $instances[$id] = new FieldInstance($instance, 'field_instance');
+    }

Do we have a follow up for removing the hard-coded class name here?

+++ b/core/modules/field/field.module
@@ -784,6 +819,24 @@ function field_cache_clear() {
+ * When a field is a deleted, the tables are renamed to {field_data_field_id}
+ * and {field_revision_field_id}. To make sure we don't end up with table

Should this be {field_data_FIELD_UUID} and {field_revision_FIELD_UUID}?

+++ b/core/modules/field/field.module
@@ -784,6 +819,24 @@ function field_cache_clear() {
+ * names longer than 64 characters, we hash the uuid and return the first
+ * 6 characters so we end up with a short unique id.

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?

+++ b/core/modules/field/lib/Drupal/field/FieldInfo.php
@@ -181,7 +188,7 @@ public function getFields() {
-        $this->fieldsById[$field['id']] = $this->prepareField($field);
+        $this->fieldsById[$field['uuid']] = $this->prepareField($field);
       }
 
       // Store in persistent cache.
@@ -191,7 +198,7 @@ public function getFields() {

@@ -191,7 +198,7 @@ public function getFields() {
     // Fill the name/ID map.
     foreach ($this->fieldsById as $field) {
       if (!$field['deleted']) {
-        $this->fieldIdsByName[$field['field_name']] = $field['id'];
+        $this->fieldIdsByName[$field['id']] = $field['uuid'];

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.

swentel’s picture

Some answers - but not all.

This should probably have its own separate upgrade path test.

The user picture upgrade path proves that this works, so can we potentially defer this to a follow up if really needed ?

- field_associate_fields('taxonomy');

See http://drupal.org/node/1735118#comment-6366070

+ if (!field_read_field('taxonomy_forums', array('include_inactive' => TRUE))) {

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.

Mmmm, does this test still provide the intended coverage? Shouldn't we be installConfig()ing?

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:

  • HEAD: query -> order by id (primary key)
  • Config: listAll() -> alphabetical order

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.

Why only 6? (re: deleted tables

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.

Why are we passing FALSE here? This can leave unused fields around: do we want that?

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.)

Do we have a follow up for removing the hard-coded class name here?

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.

fago’s picture

I like where this is going! Just one thing that bothers me:

+++ b/core/modules/datetime/lib/Drupal/datetime/Tests/DateTimeFieldTest.php
@@ -281,13 +281,15 @@ function testDatelistWidget() {
+    $this->field = $this->field_definition;

Huh? Don't we want to distinguish between a field definition array and a field object? Doesn't this blur that distinction?

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.

yched’s picture

Will reply to @xjm & @eff review when I get a chance but,

re #132

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.

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.

yched’s picture

Status: Needs work » Needs review
FileSize
196.96 KB

Thanks 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.

Status: Needs review » Needs work

The last submitted patch, field_CMI-1735118-134.patch, failed testing.

yched’s picture

FileSize
196.86 KB

Drats. Reroll.

podarok’s picture

#136 nice work
+1 RTBC

amateescu’s picture

This is not rtbc yet, what are you +1ing exactly?

podarok’s picture

#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 )))

podarok’s picture

few more bits to review

+++ b/core/modules/field/field.crud.incundefined
@@ -48,157 +50,16 @@
-function field_create_field($field) {
+++ b/core/modules/field/field.crud.incundefined
@@ -48,157 +50,16 @@
+function field_create_field(array $field) {

this has to be described in change notice and better documented

+++ b/core/modules/field/field.crud.incundefined
@@ -467,60 +237,16 @@ function field_delete_field($field_name) {
-function field_create_instance(&$instance) {
+++ b/core/modules/field/field.crud.incundefined
@@ -467,60 +237,16 @@ function field_delete_field($field_name) {
+function field_create_instance(array $instance) {

this has to be described in change notice and better documented

+++ b/core/modules/field/field.crud.incundefined
@@ -686,82 +330,87 @@ function field_read_instance($entity_type, $field_name, $bundle, $include_additi
-function field_delete_instance($instance, $field_cleanup = TRUE) {
+++ b/core/modules/field/field.crud.incundefined
@@ -686,82 +330,87 @@ function field_read_instance($entity_type, $field_name, $bundle, $include_additi
+function field_delete_instance(FieldInstance $instance, $field_cleanup = TRUE) {

this has to be described in change notice

+++ b/core/modules/field/field.installundefined
@@ -172,7 +23,7 @@ function field_schema() {
-function _update_7000_field_create_field(&$field) {
+function _update_7000_field_create_field($field) {

not easy to grok why

+++ b/core/modules/field/lib/Drupal/field/FieldInfo.phpundefined
@@ -377,27 +384,40 @@ public function getBundleInstances($entity_type, $bundle) {
+    // Do not return anything for unknown entity types.
+    if (entity_get_info($entity_type)) {

+ if (entity_get_info($entity_type)) {

http://drupal.org/node/1929006 deprecated

+++ b/core/modules/field/lib/Drupal/field/Tests/FieldImportCreateTest.phpundefined
@@ -0,0 +1,84 @@
+    config_import();
...
+    // Assert the staging field is there.
+    $field = entity_load('field_entity', 'field_test_import_staging');
+    $this->assertTrue($field, 'Test import field from staging exists');
+    $instance = entity_load('field_instance', 'test_entity.test_bundle.field_test_import_staging');
+    $this->assertTrue($instance, 'Test import field instance from staging exists');
+  }
+}
diff --git a/core/modules/field/lib/Drupal/field/Tests/FieldImportDeleteTest.php 

Edit: not nessesary as it loads only one instance...
skip...
http://drupal.org/node/1553180 entity_load renamed

swentel’s picture

Those 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.

yched’s picture

FileSize
12.04 KB

Oops, somehow forgot to post the interdiff in #134.
Here it is.

yched’s picture

FileSize
196.88 KB

Reroll.

xjm’s picture

+++ b/core/modules/file/lib/Drupal/file/Tests/FileFieldWidgetTest.phpundefined
@@ -88,7 +88,13 @@
+    // Use explicit names instead of random names for those fields, because of a
+    // bug in drupalPost() with multiple file uploads in one form, where the
+    // order of uploads depends on the order in which the upload elements are
+    // added to the $form (which, in the current implementation of
+    // FileStorage::listAll(), comes down to the alphabetical order on field
+    // names).
+    $field_name = 'test_file_field_1';
     $field_name2 = 'test_file_field_2';

@@ -205,7 +211,7 @@
-    $field_name = 'test_file_field';
+    $field_name = strtolower($this->randomName());

Aha! 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 and test_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.

Status: Needs review » Needs work

The last submitted patch, field_CMI-1735118-143.patch, failed testing.

xjm’s picture

Round 2, from the top down to just before field.install. :)

  1. +++ b/core/modules/comment/comment.installundefined
    @@ -16,7 +16,7 @@ function comment_uninstall() {
    -    field_attach_delete_bundle('comment', 'comment_node_' . $node_type);
    +    field_attach_delete_bundle('comment', 'comment_node_' . $node_type, FALSE);
    
    +++ b/core/modules/field/field.attach.incundefined
    @@ -1582,14 +1588,18 @@ function field_attach_rename_bundle($entity_type, $bundle_old, $bundle_new) {
    + * @param $field_cleanup
    + *   If TRUE, the field will be deleted as well if its last instance is being
    + *   deleted. If FALSE, it is the caller's responsibility to handle the case of
    + *   fields left without instances. Defaults to TRUE.
    ...
    -function field_attach_delete_bundle($entity_type, $bundle) {
    +function field_attach_delete_bundle($entity_type, $bundle, $field_cleanup = TRUE) {
    

    Note to self: API change to add to the summary (also look for why).

  2. +++ b/core/modules/field/field.attach.incundefined
    @@ -1549,11 +1549,17 @@ function field_attach_create_bundle($entity_type, $bundle) {
    +  foreach ($instances as $id => $instance) {
    +    if ($instance['entity_type'] == $entity_type && $instance['bundle'] == $bundle_old) {
    +      $new_instance_id = $instance['entity_type'] . '.' . $bundle_new . '.' . $instance['field_name'];
    +      config('field.instance.' . $instance['entity_type'] . '.' . $bundle_old . '.' . $instance['field_name'])->rename('field.instance.' . $new_instance_id);
    +      config('field.instance.' . $instance['entity_type'] . '.' . $bundle_new . '.' . $instance['field_name'])
    +        ->set('bundle', $bundle_new)
    +        ->set('id', $new_instance_id)
    +        ->save();
    +    }
    

    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 is entity_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.

  3. +++ b/core/modules/field/field.crud.incundefined
    @@ -48,157 +50,16 @@
    +function field_create_field(array $field) {
    +  $field = entity_create('field_entity', $field);
    +  $field->save();
       return $field;
     }
    

    This is lovely. :)

  4. +++ b/core/modules/field/field.crud.incundefined
    @@ -224,78 +85,21 @@ function field_create_field($field) {
    +  // Module developers can still pass in an array of properties
    +  // into field_update_field().
    +  if (is_array($field)) {
    

    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.

  5. +++ b/core/modules/field/field.crud.incundefined
    @@ -224,78 +85,21 @@ function field_create_field($field) {
    +    // Override settings
    +    foreach ($field as $key => $value) {
    +      $field_loaded[$key] = $value;
    +    }
    

    This hunk could use a clearer comment; something like "Update the field with the passed-in values." (Also, comments should end in a period.)

  6. +++ b/core/modules/field/field.crud.incundefined
    @@ -325,11 +129,8 @@ function field_read_field($field_name, $include_additional = array()) {
      * @param array $params
    

    Out of scope, but I would dearly love to call this $conditions instead of $params so that the function was more human-readable.

  7. +++ b/core/modules/field/field.crud.incundefined
    @@ -325,11 +129,8 @@ function field_read_field($field_name, $include_additional = array()) {
    + *   An array of conditions to match against. Keys are names of properties found
    + *   in field configuration files, values are conditions to match.
    

    Ahhh, comma splice! (Yes, I know it already was there.) :)

  8. +++ b/core/modules/field/field.crud.incundefined
    @@ -325,11 +129,8 @@ function field_read_field($field_name, $include_additional = array()) {
      * @param array $include_additional
      *   The default behavior of this function is to not return fields that are
      *   inactive or have been deleted. Setting
    

    We need to update this parameter description to include all the possible flags and what they mean. I suggest a nice list.

  9. +++ b/core/modules/field/field.crud.incundefined
    @@ -342,68 +143,58 @@ function field_read_field($field_name, $include_additional = array()) {
    +  $include_inactive = isset($include_additional['include_inactive']) && $include_additional['include_inactive'];
    +  $include_deleted = (isset($include_additional['include_deleted']) && $include_additional['include_deleted']) || (isset($params['deleted']) && $params['deleted']);
    

    wat

    (Translation: Uh, let's add a comment for all this handsome inline logic.)

  10. +++ b/core/modules/field/field.crud.incundefined
    @@ -342,68 +143,58 @@ function field_read_field($field_name, $include_additional = array()) {
    +  // Get configuration fields.
    +  $fields = entity_load_multiple('field_entity');
    

    "Configuration fields"? Maybe "fields stored in the active configuration" or something? I don't actually know what the comment is trying to say.

  11. +++ b/core/modules/field/field.crud.incundefined
    @@ -342,68 +143,58 @@ function field_read_field($field_name, $include_additional = array()) {
    +  // Merge deleted fields if needed..
    

    Nitpick: double period.

  12. +++ b/core/modules/field/field.crud.incundefined
    @@ -342,68 +143,58 @@ function field_read_field($field_name, $include_additional = array()) {
    +  // Add active and storage active parameters.
    +  if (!$include_inactive) {
    +    $params['active'] = 1;
    +    $params['storage.active'] = 1;
       }
    ...
    +  // Collect matching fields.
    +  $matching_fields = array();
    +  foreach ($fields as $field) {
    +    // Conditions.
    +    foreach ($params as $key => $value) {
    +      switch ($key) {
    +        case 'storage.active':
    +          $checked_value = $field->storage['active'];
    +          break;
    +
    +        case 'field_name';
    +          $checked_value = $field->id;
    +          break;
    +
    +        default:
    +          $checked_value = $field->$key;
    +          break;
    +      }
    +
    +      if ($checked_value != $value) {
    +        continue 2;
    +      }
    +    }
    

    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?

  13. +++ b/core/modules/field/field.crud.incundefined
    @@ -342,68 +143,58 @@ function field_read_field($field_name, $include_additional = array()) {
    +    // Invoke read field.
         module_invoke_all('field_read_field', $field);
    

    Maybe:
    // Invoke hook_field_read_field().

  14. +++ b/core/modules/field/field.crud.incundefined
    @@ -342,68 +143,58 @@ function field_read_field($field_name, $include_additional = array()) {
    -    $fields[$field_name] = $field;
    +    $key = $include_deleted ? $field->uuid : $field->id;
    +    $matching_fields[$key] = $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.

  15. +++ b/core/modules/field/field.crud.incundefined
    @@ -686,82 +330,87 @@ function field_read_instance($entity_type, $field_name, $bundle, $include_additi
     function field_read_instances($params = array(), $include_additional = array()) {
    

    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?)

  16. +++ b/core/modules/field/field.crud.incundefined
    @@ -858,6 +507,10 @@ function field_purge_batch($batch_size) {
    +    // @todo EFQ currently fails on conditions on comment bundle.
    +    if ($entity_type == 'comment') {
    +      continue;
    +    }
    

    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.

swentel’s picture

Status: Needs work » Needs review
FileSize
1.53 KB
202.9 KB

Reroll - 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

swentel’s picture

FileSize
197.73 KB

Better patch

Anonymous’s picture

FileSize
122.36 KB
132.44 KB

i 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.

XHProf summary

i've attached the xhrpof aggregate files if anyone else wants to dig in.

Anonymous’s picture

Issue summary: View changes

typo

yched’s picture

@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.]

amateescu’s picture

@swentel found the same thing here: #1931900: EFQ calls field_info_field() too liberally

yched’s picture

A 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:

// Force taxonomy fields to be re-activated *now*.
field_associate_fields('taxonomy');
// Create 'taxonomy_forums' field if the field is not active.
if (!field_info_field('taxonomy_forums')) {
  field_create_field(... 'taxonomy_forums' ...);
}

Patch does a more reasonable:

// Create 'taxonomy_forums' field if it does not exist (active or not).
if (!field_read_field('taxonomy_forums', array('include_inactive'))) {
  field_create_field(... 'taxonomy_forums' ...);
}

- @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

-        $this->fieldsById[$field['id']] = $this->prepareField($field);
+        $this->fieldsById[$field['uuid']] = $this->prepareField($field);

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.

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.

yched’s picture

- @eff #130 point 8

substr(hash('sha256', $field['uuid']), 0, 6);

Why only 6? (...) Perhaps raising to 32 (the length field ids are allowed to be) would be safer?

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

yched’s picture

FileSize
10.53 KB
195.77 KB

Lastly 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.

effulgentsia’s picture

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 ?

We 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).

@swentel pushed from 6 to 10 chars

That's good enough for now. The above can be a follow up discussion.

Anonymous’s picture

re #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.

yched’s picture

FileSize
195.62 KB

Nope, 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.

xjm’s picture

+++ b/core/modules/field/field.crud.incundefined
@@ -147,7 +147,14 @@
+    // Optimize for the most frequent case where we do have a specific id.
...
+    // No specific id, we need to examine all existing fields.

@@ -333,7 +340,14 @@
+    // Optimize for the most frequent case where we do have a specific id.
...
+    // No specific id, we need to examine all existing instances.

+++ b/core/modules/field_sql_storage/field_sql_storage.moduleundefined
@@ -55,6 +60,11 @@
+ * 10 characters so we end up with a short unique id.

s/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.

yched’s picture

Pushed a fix for #158:
194657b id --> ID

Working on @xjm's other remarks in #146, but will probably not get there before monday.

Anonymous’s picture

FileSize
295.53 KB

here'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.

yched’s picture

FileSize
14.09 KB
198.86 KB

Updated 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.

We need to update this parameter description to include all the possible flags and what they mean. I suggest a nice list

Not changed by the patch, so not done for the reasons above.

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.

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'

xjm’s picture

Okay, 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:

  1. File the followups you propose and link them here. There's a lot of "that can be a followup" above and no issues for them.
  2. In general, updating the documentation block for a function is part of the documentation gate when you change the behavior of the function, even if the previous documentation was also insufficient. That's the case here. When array keys have special meanings, the @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.
xjm’s picture

Down to field's lib/.

  1. +++ b/core/modules/field/field.installundefined
    @@ -484,6 +352,116 @@ function field_update_8002() {
    +function field_update_8003() {
    

    I don't see an upgrade path test in the patch. Did I miss it or does it still need to be written?

  2. +++ b/core/modules/field/field.moduleundefined
    @@ -334,7 +335,8 @@ function field_cron() {
    +  // It's not safe to call field_read_fields during maintenance mode.
    

    Minor: field_read_fields() should have parens.

  3. +++ b/core/modules/field/field.moduleundefined
    @@ -528,48 +530,81 @@ function field_modules_disabled($modules) {
    +  foreach (config_get_storage_names_with_prefix('field.field') as $uuid) {
    +    $field = config($uuid)->get();
    +    $fields[$field['uuid']] = $field;
       }
    ...
    +      config('field.field.' . $field['id'])
    

    I'm a little confused; why is it 'uuid' when we read (and in the variable names) but just the 'id' when we write?

  4. +++ b/core/modules/field/field.moduleundefined
    @@ -528,48 +530,81 @@ function field_modules_disabled($modules) {
    +  // Set fields which field type module or storage module is absent to
    +  // 'inactive'.
    

    Not quite a sentence; how about "Set fields with missing field type or storage modules to inactive."

  5. +++ b/core/modules/field/field.multilingual.incundefined
    @@ -200,7 +200,10 @@ function field_content_languages() {
    +  // See http://drupal.org/node/1862758#comment-7035586
    +  if (!empty($field)) {
    +    return $field['translatable'] && field_has_translation_handler($entity_type);
    +  }
    

    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.)

xjm’s picture

Issue summary: View changes

Updated issue summary.

swentel’s picture

Created the follow ups and updated the issue summary, we're getting real close, I can feel it! :)

yched’s picture

Just 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

yched’s picture

FileSize
1.89 KB
198.22 KB

re @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.

swentel’s picture

Addition 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

-    $this->assertEqual(1, $usage['image']['default_image'][$field['id']]);
+    $this->assertTrue(isset($usage['image']['default_image'][$field['uuid']]));

The small Field API upgrade tests

+  /**
+   * Tests upgrade of fields and instances to config.
+   */
+  function testFieldUpgradeToConfig() {
+    $this->assertTrue($this->performUpgrade(), t('The upgrade was completed successfully.'));
+
+    // Assert the body field and instance on the article are converted.
+    $body_field = field_info_field('body');
+    $this->assertNotNull($body_field, 'The body field has been found.');
+    $body_field_instance = field_info_instance('node', 'body', 'article');
+    $this->assertNotNull($body_field_instance, 'The body field instance on the article content type has been found.');
+  }
yched’s picture

Simple 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]

yched’s picture

Forgot to attach the rerolled patch.

Status: Needs review » Needs work

The last submitted patch, field_CMI-1735118-172.patch, failed testing.

swentel’s picture

FileSize
10.07 KB
203.35 KB

Test 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 :)]

swentel’s picture

Status: Needs work » Needs review
alexpott’s picture

Whilst talking to @swentel in IRC noticed a couple of minor nits..

+++ b/core/modules/field/lib/Drupal/field/FieldInstanceStorageController.phpundefined
@@ -0,0 +1,36 @@
+    // If the field the field has been deleted in the same import, the instance

the field the field why hast thou forsaken me? :D

+++ b/core/modules/field/lib/Drupal/field/FieldStorageController.phpundefined
@@ -0,0 +1,17 @@
+<?php
+
+/**
+ * @file
+ * Contains \Drupal\field\FieldStorageController.
+ */
+
+namespace Drupal\field;
+
+use Drupal\Core\Config\Entity\ConfigStorageController;
+
+/**
+ * Controller class for fields.
+ */
+class FieldStorageController extends ConfigStorageController {
+
+}

No need for this implementation.. according to @swentel already removed from the sandbox.

effulgentsia’s picture

I 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?

yched’s picture

@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).

yched’s picture

Also, still a @todo in the code :

function field_sql_storage_schema() {
    // (... schema for non-deleted field tables ...)
    // @todo we also need deleted fields, they have a schema.
}

- 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 ?

yched’s picture

Mh, after a closer look :

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().

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().

Status: Needs review » Needs work

The last submitted patch, field_CMI-1735118-180.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
653 bytes
206.85 KB

Stupid last minute change. randomString() is no good for a string that will run through check_markup()...

yched’s picture

FileSize
22.33 KB
214.64 KB

Pushed 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()

yched’s picture

FileSize
214.72 KB

Re: 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)

yched’s picture

Turns 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.

xjm’s picture

Alright, 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).

  1. +++ b/core/modules/field/lib/Drupal/field/Tests/CrudTest.phpundefined
    @@ -172,9 +170,10 @@ function testCreateFieldFail() {
    +    // The field does not exist.
    +    // $field = config('field.field.' . $field_name)->get();
    +    $field = entity_load('field_entity', $field_name);
    +    $this->assertFalse($field, 'The field does not exist.');
    

    Presumably the second commented line ehre is a leftover from before config entities? Can we remove that?

  2. +++ b/core/modules/field/lib/Drupal/field/Tests/CrudTest.phpundefined
    @@ -238,8 +231,10 @@ function testFieldIndexes() {
    -    $this->assertEqual($field['indexes'], $expected_indexes, 'Field type indexes saved by default');
    +    // @todo This is a tested behavior... indexes are supposed to be saved in the field.
    +    $this->assertEqual($schema['indexes'], $expected_indexes, 'Field type indexes saved by default');
    

    Is there an issue for this @todo? I don't quite grok it, nor does the comment incdicate which test asserts this behavior.

  3. +++ b/core/modules/field/lib/Drupal/field/Tests/FieldImportChangeTest.phpundefined
    @@ -0,0 +1,68 @@
    +    // Change label.
    +    $active = $this->container->get('config.storage');
    +    $staging = $this->container->get('config.storage.staging');
    +    $manifest = $active->read($this->instance_manifest);
    +    $instance = $active->read($this->instance_name);
    +    $new_label = 'Test update import field';
    +    $instance['label'] = $new_label;
    +    $staging->write($this->instance_name, $instance);
    +    $staging->write($this->instance_manifest, $manifest);
    

    So, a question for this and the other tests. Why aren't we using the entity API when we update the label?

  4. +++ b/core/modules/field/lib/Drupal/field/Tests/FieldImportCreateTest.phpundefined
    @@ -0,0 +1,84 @@
    +    // Enable field_test_config module and assert the test import
    +    // field and instance is available on the Test content type.
    +    // This tests creating fields and instances that are provided
    +    // by a module.
    ...
    +    // Copy another field and instance to the staging directory
    +    // on the Test content type and run config_import() to test
    +    // importing from the staging directory.
    

    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.

  5. +++ b/core/modules/field/lib/Drupal/field/Tests/FieldImportCreateTest.phpundefined
    @@ -0,0 +1,84 @@
    +    // Copy the files.
    

    I can see it's copying files. What might be more helpful is from where to where and why. :)

  6. +++ b/core/modules/field/lib/Drupal/field/Tests/FieldImportCreateTest.phpundefined
    @@ -0,0 +1,84 @@
    +    $field_manifest['field_test_import_staging'] = array('name' => $this->field_test_import_staging);
    +    $instance_manifest['node.test_import.field_test_import_staging'] = array('name' => $this->instance_test_import_staging);
    

    Seems to me like CMI should have methods for adding/removing entries from the manifest. (Totally out of scope and unrelated; just an observation.)

  7. +++ b/core/modules/field/lib/Drupal/field/Tests/FieldImportCreateTest.phpundefined
    @@ -0,0 +1,84 @@
    +    // Write to manifest and new config.
    +    $staging->write($this->field_manifest, $field_manifest);
    +    $staging->write($this->instance_manifest, $instance_manifest);
    

    Huh? It looks like it's just updating the manifests.

  8. +++ b/core/modules/field/lib/Drupal/field/Tests/FieldImportCreateTest.phpundefined
    @@ -0,0 +1,84 @@
    +    // Assert the staging field is there.
    

    s/staging field/staged field/ ?

  9. +++ b/core/modules/field/lib/Drupal/field/Tests/FieldImportDeleteTest.phpundefined
    @@ -0,0 +1,87 @@
    + * Tests the functionality of deleting fields and instances
    + * on hook_config_import_delete().
    

    Nitpick: The docblock should be one line of 80 characters or fewer. Try:

    Tests field and instance deletion on	hook_config_import_delete().
    
  10. +++ b/core/modules/field/lib/Drupal/field/Tests/FieldImportDeleteTest.phpundefined
    @@ -0,0 +1,87 @@
    +   * Test importing deletions.
    

    "Test importing deletions"? Huh? Maybe "Tests import when a field and instance are deleted."? (Note that it should be "Tests" rather than "Test".)

  11. +++ b/core/modules/field/lib/Drupal/field/Tests/FieldImportDeleteTest.phpundefined
    @@ -0,0 +1,87 @@
    +    // Check the field exists.
    +    $field_test_import = field_info_field('field_test_import');
    

    "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.)

  12. +++ b/core/modules/field/lib/Drupal/field/Tests/FieldImportDeleteTest.phpundefined
    @@ -0,0 +1,87 @@
    +    // Delete body field and instance, the test import instance
    +    // from the manifest.
    +    $active = $this->container->get('config.storage');
    +    $staging = $this->container->get('config.storage.staging');
    +    $field_manifest = $active->read($this->field_manifest);
    +    unset($field_manifest['field_test_import']);
    +    $instance_manifest = $active->read($this->instance_manifest);
    +    unset($instance_manifest['test_entity.test_bundle.field_test_import']);
    +    $staging->write($this->field_manifest, $field_manifest);
    +    $staging->write($this->instance_manifest, $instance_manifest);
    

    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:

    Delete the body field and its instance, then remove it from the manifest.
    
  13. +++ b/core/modules/field/lib/Drupal/field/Tests/FieldImportDeleteTest.phpundefined
    @@ -0,0 +1,87 @@
    +    // Import.
    +    config_import();
    ...
    +    // Run purge_batch().
    +    field_purge_batch(10);
    

    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. :)

  14. +++ b/core/modules/field/lib/Drupal/field/Tests/FieldImportDeleteTest.phpundefined
    @@ -0,0 +1,87 @@
    +    // Assert the field and instance are gone.
    +    $field = entity_load('field_entity', 'field_test_import', TRUE);
    +    $this->assertFalse($field, 'Test import field is removed.');
    +    $instance = entity_load('field_instance', 'test_entity.test_bundle.field_test_import', TRUE);
    +    $this->assertFalse($instance, 'Test import field instance is removed.');
    

    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?

  15. +++ b/core/modules/field/lib/Drupal/field/Tests/FieldImportDeleteTest.phpundefined
    @@ -0,0 +1,87 @@
    +    // Check that import field is in state of deleted fields.
    

    Articles will really help clarify what's a verb here and what's a noun. E.g.:

    Check that the field is stored in the deleted field list in state().
    
  16. +++ b/core/modules/field/lib/Drupal/field/Tests/FieldImportDeleteTest.phpundefined
    @@ -0,0 +1,87 @@
    +    // Check that the deleted fields are removed from state.
    ...
    +    // Check all config files are gone.
    

    I'd suggest s/check that/confirm that/gi.

  17. +++ b/core/modules/field/lib/Drupal/field/Tests/FieldImportDeleteTest.phpundefined
    @@ -0,0 +1,87 @@
    +  }
    +}
    

    Nitpick of all nitpicks: There should be a blank line between the last member and the closing curly for the class.

  18. +++ b/core/modules/field/lib/Drupal/field/Tests/FieldImportChangeTest.phpundefined
    @@ -0,0 +1,68 @@
    +/**
    + * Tests the functionality of deleting fields and instances
    + * on hook_config_import_change().
    + */
    +class FieldImportChangeTest extends FieldUnitTestBase {
    
    +++ b/core/modules/field/lib/Drupal/field/Tests/FieldImportCreateTest.phpundefined
    @@ -0,0 +1,84 @@
    +/**
    + * Tests the functionality of creating fields and instances
    + * on hook_config_import_create().
    + */
    +class FieldImportCreateTest extends FieldUnitTestBase {
    

    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.).

  19. +++ b/core/modules/field/lib/Drupal/field/Tests/FieldImportChangeTest.phpundefined
    @@ -0,0 +1,68 @@
    +  /**
    +   * Test importing changes.
    +   */
    +  function testImportChange() {
    

    Tests importing what changes? Maybe:
    Tests importing an updated field instance.

  20. +++ b/core/modules/field/lib/Drupal/field/Tests/FieldInfoTest.phpundefined
    @@ -173,25 +168,19 @@ function testInstancePrepare() {
    +    config('field.instance.' . $instance_definition['entity_type'] . '.' . $instance_definition['bundle'] . '.' . $instance_definition['field_name'])
    
    +++ b/core/modules/field/lib/Drupal/field/Tests/FieldInstanceCrudTest.phpundefined
    @@ -48,26 +49,21 @@ function setUp() {
    +    $config = config('field.instance.' . $this->instance_definition['entity_type'] . '.' . $this->instance_definition['bundle'] . '.' . $this->instance_definition['field_name'])->get();
    

    Do we have a method for generating field instance object names rather than composing them from the constituent parts by hand?

  21. +++ b/core/modules/field/tests/modules/field_test/field_test.storage.incundefined
    @@ -89,7 +89,7 @@ function field_test_field_storage_load($entity_type, $entities, $age, $fields, $
    -    $field_data = $data[$field['id']];
    +    $field_data = $data[$field['uuid']];
    

    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.

  22. +++ b/core/modules/field/tests/modules/field_test_config/config/field.field.field_test_import.ymlundefined
    @@ -0,0 +1,20 @@
    +id: field_test_import
    
    +++ b/core/modules/field/tests/modules/field_test_config/config/field.instance.test_entity.test_bundle.field_test_import.ymlundefined
    @@ -0,0 +1,20 @@
    +id: test_entity.test_bundle.field_test_import
    

    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.)

  23. +++ b/core/modules/field/tests/modules/field_test_config/config/field.field.field_test_import.ymlundefined
    @@ -0,0 +1,20 @@
    +active: 1
    

    active == 0 => disabled?

  24. +++ b/core/modules/field/tests/modules/field_test_config/config/field.field.field_test_import.ymlundefined
    @@ -0,0 +1,20 @@
    +locked: false
    
    +++ b/core/modules/field/tests/modules/field_test_config/staging/field.field.field_test_import_staging.ymlundefined
    @@ -0,0 +1,20 @@
    +locked: '0'
    

    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.

  25. +++ b/core/modules/field/tests/modules/field_test_config/staging/field.field.field_test_import_staging.ymlundefined
    @@ -0,0 +1,20 @@
    +entity_types: {  }
    

    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.

  26. +++ b/core/modules/field/tests/modules/field_test_config/staging/field.field.field_test_import_staging.ymlundefined
    @@ -0,0 +1,20 @@
    +  settings: {  }
    

    What goes in here? Serialized field settings or such? (I probably need to look at the schema.)

  27. +++ b/core/modules/field/tests/modules/field_test_config/staging/field.instance.test_entity.test_bundle.field_test_import_staging.ymlundefined
    @@ -0,0 +1,20 @@
    +id: test_entity.test_bundle.field_test_import_staging
    +uuid: ea711065-6940-47cd-813d-618f64095481
    +langcode: und
    +field_uuid: 0bf654cc-f14a-4881-b94c-76959e47466b
    +entity_type: test_entity
    +bundle: test_bundle
    

    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.)

  28. +++ b/core/modules/field/tests/modules/field_test_config/staging/field.field.field_test_import_staging.ymlundefined
    @@ -0,0 +1,20 @@
    +id: field_test_import_staging
    +uuid: 0bf654cc-f14a-4881-b94c-76959e47466b
    ...
    +type: text
    ...
    +module: text
    

    ...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.

  29. +++ b/core/modules/field/tests/modules/field_test_config/staging/field.instance.test_entity.test_bundle.field_test_import_staging.ymlundefined
    @@ -0,0 +1,20 @@
    +widget:
    +  type: text_textfield
    ...
    +  module: text
    

    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.

gdd’s picture

#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.

xjm’s picture

+++ b/core/modules/field/lib/Drupal/field/Plugin/views/field/Field.phpundefined
@@ -169,8 +169,8 @@ public function query($use_groupby = FALSE) {
+        if (isset($this->field_info['storage details']['sql'][$rkey][$this->table][$column])) {
+          $name = $this->field_info['storage details']['sql'][$rkey][$this->table][$column];

Elsewhere 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?

xjm’s picture

So 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 a Drupal\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 and FieldInstance 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.

  1. +++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/Field.phpundefined
    @@ -0,0 +1,463 @@
    +  /**
    +   * The field ID (config name).
    +   *
    +   * @var string
    +   */
    
    +++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/FieldInstance.phpundefined
    @@ -0,0 +1,423 @@
    +  /**
    +   * The field ID (config name).
    +   *
    +   * @var string
    +   */
    +  public $id;
    ...
    +  /**
    +   * The field name.
    +   * @todo Revisit that in favor of a getField() method.
    +   *
    +   * @var string
    +   */
    ...
    +  /**
    +   * Overrides \Drupal\Core\Entity\Entity::id().
    +   */
    +  public function id() {
    +    return $this->entity_type . '.' . $this->bundle . '.' . $this->field_name;
    +  }
    

    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:

    • It says "The field ID (config name)" on the field INSTANCE. I think that's inaccurate? For Field,
      $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 presumably entity_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 name my_field_name, i.e., what the user sets when creating the field. Let's also document that and give an example.
    • On FieldInstance, we should also clarify that these refer to the main field object for which these are instances.
    • I think something like the proposed method in the @todo sounds like a good idea, because we could get unexpected behavior if something sets a bar_field_name not in synch with the actual config object name entity_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. :)
    • I don't think 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 expect getField() to return an instantiated Field object. Unless that's what you mean?)
    • Nitpick: @todos should be the last thing in any docblock other than an annotation, and there should be a blank line between them and other sections of the docblock.
    • Let's definitely make sure the followup issue for this gets filed.
  2. +++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/Field.phpundefined
    @@ -0,0 +1,463 @@
    +
    +  /**
    +   * The field module.
    +   *
    +   * @var string
    +   */
    
    +++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/FieldInstance.phpundefined
    @@ -0,0 +1,423 @@
    +  /**
    +   * The instance entity type.
    +   *
    +   * @var string
    +   */
    +  public $entity_type;
    +
    +  /**
    +   * The instance bundle.
    +   *
    +   * @var string
    +   */
    +  public $bundle;
    

    Should these really be public?

  3. +++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/Field.phpundefined
    @@ -0,0 +1,463 @@
    +  /**
    +   * The field active state.
    +   *
    +   * @var bool
    +   */
    +  public $active;
    +
    +  /**
    +   * The field storage locked state.
    +   *
    +   * @var bool
    +   */
    ...
    +  /**
    +   * The field settings.
    +   *
    +   * @var array
    +   */
    ...
    +  /**
    +   * The field entity types.
    +   *
    +   * @var array
    +   */
    +  public $entity_types;
    ...
    +  /**
    +   * The field indexes.
    +   *
    +   * @var array
    +   */
    ...
    +  /**
    +   * The field storage.
    +   *
    +   * @var array
    +   */
    ...
    +  /**
    +   * The field schema.
    +   *
    +   * @var array
    +   */
    

    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...)

  4. +++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/Field.phpundefined
    @@ -0,0 +1,463 @@
    +  /**
    +   * The field deleted state.
    +   *
    +   * @var bool
    +   */
    
    +++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/FieldInstance.phpundefined
    @@ -0,0 +1,423 @@
    +  /**
    +   * The instance deleted state.
    +   *
    +   * @var bool
    +   */
    +  public $deleted;
    

    Also not sure about these being public. This flag should presumably only be set when a method is called on the object, right?

  5. +++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/Field.phpundefined
    @@ -0,0 +1,463 @@
    +  /**
    +   * Overrides \Drupal\Core\Config\Entity\ConfigEntityBase::__construct().
    +   */
    
    +++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/FieldInstance.phpundefined
    @@ -0,0 +1,423 @@
    +  /**
    +   * Overrides \Drupal\Core\Config\Entity\ConfigEntityBase::__construct().
    +   */
    +  public function __construct(array $values, $entity_type = 'field_instance') {
    

    I feel like we should document what we expect to have in the $values array here.

    • E.g., the exception handling for Field implies that the field type and field_name are required, that the the field_name needs to be a valid machine name.
    • For FieldInstance, they imply that entity_type, bundle, and one of field_name and field_uuid are required.
  6. +++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/Field.phpundefined
    @@ -0,0 +1,463 @@
    +    if (empty($values['id'])) {
    +      $values['id'] = $values['field_name'];
    +      unset($values['field_name']);
    +    }
    

    Again with the id vs. field_name conversion. Here it looks like id is canonical for the machine anme, and not field_name. But elsewhere, ID means the configuration object name, and the machine name is field_name. I don't get it. :)

  7. +++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/FieldInstance.phpundefined
    @@ -0,0 +1,423 @@
    +class FieldInstance extends ConfigEntityBase implements \ArrayAccess {
    
    +++ b/core/modules/field/lib/Drupal/field/Plugin/Type/Widget/WidgetInterface.phpundefined
    @@ -8,7 +8,7 @@
    -use Drupal\field\FieldInstance;
    +use Drupal\field\Plugin\Core\Entity\FieldInstance;
    

    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?

  8. +++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/FieldInstance.phpundefined
    @@ -0,0 +1,423 @@
    +   * Default value function
    

    Nitpick: missing period. However, again, we could describe this much better, e.g., "A function that provides the default value for the field instance."

  9. +++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/FieldInstance.phpundefined
    @@ -0,0 +1,423 @@
    +    // Check required properties.
    +    if (empty($values['entity_type'])) {
    +      throw new FieldException(format_string('Attempt to create an instance of field @field_name without an entity type.', array('@field_name' => $values['field_name'])));
    +    }
    +    if (empty($values['bundle'])) {
    +      throw new FieldException(format_string('Attempt to create an instance of field @field_name without a bundle.', array('@field_name' => $values['field_name'])));
    +    }
    

    Nice, glad to see these here.

  10. +++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/FieldInstance.phpundefined
    @@ -0,0 +1,423 @@
    +      $field = field_info_field($values['field_name']);
    

    Interesting that we're calling out to field_info_field() here. Which, apparently returns the cached field definition, via a procedural wrapper that wraps the FieldInfo class... presumably some day later in the conversion process this will all happen inside an $entity->load()?

  11. +++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/FieldInstance.phpundefined
    @@ -0,0 +1,423 @@
    +        throw new FieldException(format_string('Attempt to create an instance of unknown, disabled or deleted field @name', array('@name' => $values['field_name'])));
    

    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".

  12. +++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/FieldInstance.phpundefined
    @@ -0,0 +1,423 @@
    +    // @todo Revisit that in favor of a getField() method.
    

    We'll want to replace this @todo with a reference to our followup here as well.

  13. +++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/FieldInstance.phpundefined
    @@ -0,0 +1,423 @@
    +      if ($field) {
    +        $values['field_name'] = $field->id;
    +      }
    

    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.

  14. +++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/FieldInstance.phpundefined
    @@ -0,0 +1,423 @@
    +    // Provide defaults.
    +    $values += array(
    

    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.

  15. +++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/FieldInstance.phpundefined
    @@ -0,0 +1,423 @@
    +      'label' => $values['field_name'],
    

    This is some magic behavior. We default the field label to its machine name? Does this functionality exist in D7? Seems... odd.

yched’s picture

FileSize
16.72 KB
214.97 KB

re @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.

xjm’s picture

Thanks @yched!

Check that X is foobared" formulation, that's used throughout all of core tests.

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.

xjm’s picture

Just a followup, I read the updated tests and they're tons clearer, so #193 is mostly irrelevant. :)

xjm’s picture

Maybe "Tests importing the 'deleted' status for fields and instances"?

xjm’s picture

Alright, 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:

  1. +++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/Field.phpundefined
    @@ -0,0 +1,463 @@
    +    $module_handler = \Drupal::service('module_handler');
    ...
    +      $module_handler = \Drupal::service('module_handler');
    

    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).

  2. +++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/Field.phpundefined
    @@ -0,0 +1,463 @@
    +      // Field name cannot be longer than 32 characters. We use drupal_strlen()
    

    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?

  3. +++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/Field.phpundefined
    @@ -0,0 +1,463 @@
    +      // Create all per-field-type properties (needed here as long as we have
    +      // settings that impact column definitions).
    +      $this->settings += $field_type['settings'];
    

    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?

  4. +++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/Field.phpundefined
    @@ -0,0 +1,463 @@
    +      // Provide default storage.
    +      $this->storage += array(
    +        'type' => variable_get('field_storage_default', 'field_sql_storage'),
    +        'settings' => array(),
    +      );
    

    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:

    [tesla:drupal | Tue 15:29:45] $ grep -r "field_storage_default" *
    core/modules/edit/lib/Drupal/edit/Tests/EditTestBase.php:    variable_set('field_storage_default', $this->default_storage);
    core/modules/field/field.attach.inc: * 'field_storage_default' identifies the storage backend used by default.
    core/modules/field/field.crud.inc: *     - type: the storage backend specified in the 'field_storage_default'
    core/modules/field/field.crud.inc:    'type' => variable_get('field_storage_default', 'field_sql_storage'),
    core/modules/field/lib/Drupal/field/Tests/CrudTest.php:    $this->assertEqual($record['storage_type'], variable_get('field_storage_default'), 'The field type is properly saved.');
    core/modules/field/lib/Drupal/field/Tests/FieldTestBase.php:    variable_set('field_storage_default', $this->default_storage);
    core/modules/field/lib/Drupal/field/Tests/FieldUnitTest
    
  5. +++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/Field.phpundefined
    @@ -0,0 +1,463 @@
    +      $storage_type = field_info_storage_types($this->storage['type']);
    

    Assuming this is legacy and will be replaced later?

  6. +++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/Field.phpundefined
    @@ -0,0 +1,463 @@
    +      $this->storage['active'] = 1;
    

    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.

  7. +++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/Field.phpundefined
    @@ -0,0 +1,463 @@
    +      // Notify the storage backend,
    +      $module_handler->invoke($this->storage['module'], 'field_storage_create_field', array($this));
    

    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().

  8. +++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/Field.phpundefined
    @@ -0,0 +1,463 @@
    +    else {
    

    I'd put an inline comment above this else, something like, "Otherwise, the field is being updated." (The if is kind of long and so I had to look a bit to double-check what if it was matching.)

    Edit: Sorry, that is among the more useless dreditor snippets of all time. This is the else for isNew() in Field::save().

    For extra bonus points, we could factor out protected saveNew() and saveUpdated() methods. (Could make a nice novice followup, too.)

  9. +++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/Field.phpundefined
    @@ -0,0 +1,463 @@
    +        throw new FieldException("Cannot change an existing field's entity_types property.");
    

    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.

  10. +++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/Field.phpundefined
    @@ -0,0 +1,463 @@
    +      // Make sure all settings are present.
    +      $this->settings += $original->settings;
    

    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?

  11. +++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/Field.phpundefined
    @@ -0,0 +1,463 @@
    +      // Tell the storage engine to update the field. Do this before saving the
    +      // new definition since it still might fail.
    +      $module_handler->invoke($this->storage['module'], 'field_storage_update_field', array($this, $original, $has_data));
    

    I don't quite understand the implications of this hunk; could you clarify?

  12. +++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/Field.phpundefined
    @@ -0,0 +1,463 @@
    +    // Invoke external hooks after the cache is cleared for API consistency.
    +    $module_handler->invokeAll($hook, $hook_args);
    

    Can we expand the comment here to indicate it's invoking either hook_field_create_field() or hook_field_update_field()? (That's correct, right?)

  13. +++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/Field.phpundefined
    @@ -0,0 +1,463 @@
    +        $instance->delete(FALSE);
    
    +++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/FieldInstance.phpundefined
    @@ -0,0 +1,423 @@
    +  public function delete($field_cleanup = TRUE) {
    

    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.

  14. +++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/Field.phpundefined
    @@ -0,0 +1,463 @@
    +      // Delete the configuration of this field and save the field configuration
    +      // in the key_value table so we can use it later during
    +      // field_purge_batch(). This makes sure a new field can be created
    +      // immediately with the same name.
    

    Excellent documentation, thanks!

  15. +++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/Field.phpundefined
    @@ -0,0 +1,463 @@
    +      $deleted_fields = state()->get('field.field.deleted') ?: array();
    +      $config = $this->getExportProperties();
    +      $config['deleted'] = TRUE;
    

    So, this is where I think a $this->setDeleted() might be nice.

  16. +++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/Field.phpundefined
    @@ -0,0 +1,463 @@
    +  public function getSchema() {
    

    Does this get called by the storage controller? ...I'll find out soon!

  17. +++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/Field.phpundefined
    @@ -0,0 +1,463 @@
    +      $schema = (array) $module_handler->invoke($this->module, 'field_schema', array($this));
    

    I'd add a comment here identifying what hook(s) are getting invoked here (hook_field_schema()).

  18. +++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/Field.phpundefined
    @@ -0,0 +1,463 @@
    +      $schema += array('columns' => array(), 'indexes' => array(), 'foreign keys' => array());
    

    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?

  19. +++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/Field.phpundefined
    @@ -0,0 +1,463 @@
    +      if (array_intersect(array_keys($schema['columns']), field_reserved_columns())) {
    

    It looks like field_reserved_columns() returns only 'deleted'. A procedural wrapper to return that seems odd; followup?

  20. +++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/Field.phpundefined
    @@ -0,0 +1,463 @@
    +  /**
    +   * Returns the storage details for the field.
    +   *
    +   * @return array
    +   *   The storage details. @todo document.
    +   */
    +  public function getStorageDetails() {
    

    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.

  21. +++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/Field.phpundefined
    @@ -0,0 +1,463 @@
    +      $details = (array) $module_handler->invoke($this->storage['module'], 'field_storage_details', array($this));
    +      $module_handler->alter('field_storage_details', $details, $this);
    

    Same note about adding a comment saying which hook is invoked, for both of these.

  22. +++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/Field.phpundefined
    @@ -0,0 +1,463 @@
    +  public function offsetExists($offset) {
    ...
    +  public function &offsetGet($offset) {
    ...
    +  public function offsetSet($offset, $value) {
    ...
    +  public function offsetUnset($offset) {
    

    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!

tim.plunkett’s picture

Issue for the public properties is over here #1959610: Remove public properties from entity classes

xjm’s picture

#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? :)

yched’s picture

Sounds fine to me, should not interfere with the BC layer.
I'll remove public access to those properties in a next reroll.

yched’s picture

Er, 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 --.

xjm’s picture

Just a few things about FieldInstance that aren't already covered by #198:

  1. +++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/FieldInstance.phpundefined
    @@ -0,0 +1,423 @@
    +    $module_handler = \Drupal::service('module_handler');
    

    Patch is in, so we can update these now if we want. :)

  2. +++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/FieldInstance.phpundefined
    @@ -0,0 +1,423 @@
    +      // Check that the field can be attached to this entity type.
    +      if (!empty($field->entity_types) && !in_array($this->entity_type, $field->entity_types)) {
    

    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. :)

  3. +++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/FieldInstance.phpundefined
    @@ -0,0 +1,423 @@
    +      // Assign the id.
    ...
    +      // Set the field uuid.
    

    ID, UUID (caps).

  4. +++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/FieldInstance.phpundefined
    @@ -0,0 +1,423 @@
    +      // Renaming a bundle on the instance is allowed when an entity's bundle
    +      // is renamed and when field_attach_rename_bundle() does internal
    +      // housekeeping.
    +      if ($this->bundle != $original->bundle && !isset($this->bundle_rename_allowed)) {
    

    I don't see $bundle_rename_allowed documented on the class.

  5. +++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/FieldInstance.phpundefined
    @@ -0,0 +1,423 @@
    +    // Check widget module.
    

    Check what about the widget module?

  6. +++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/FieldInstance.phpundefined
    @@ -0,0 +1,423 @@
    +    // If no weight specified, make sure the field sinks at the bottom.
    

    "If no weight is specified, make sure the field sinks to the bottom."

  7. +++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/FieldInstance.phpundefined
    @@ -0,0 +1,423 @@
    +    // Invoke external hooks after the cache is cleared for API consistency.
    +    $module_handler->invokeAll($hook, $hook_args);
    ...
    +      // Let modules react to the deletion of the instance.
    +      $module_handler->invokeAll('field_delete_instance', array($this));
    ...
    +      drupal_alter(array('field_widget_properties', 'field_widget_properties_' . $this->entity_type), $widget_properties, $context);
    

    Same as above, let's document the hooks inline for readability.

  8. +++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/FieldInstance.phpundefined
    @@ -0,0 +1,423 @@
    +   * @param $field_cleanup
    +   *   (Optionnal) If TRUE, the field will be deleted as well if its last
    +   *   instance is being deleted. If FALSE, it is the caller's responsibility to
    +   *   handle the case of fields left without instances. Defaults to TRUE.
    

    "Optional" is misspelled, and the coding standards specify that it should be lowercase here, IIRC. Also, the @param declaration needs a data type.

xjm’s picture

Er, 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 --.

Hm? 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. :)

Berdir’s picture

We'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.

xjm’s picture

I think folks are misunderstanding--I definitely think the configurable field settings should be public.

yched’s picture

OK, 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.

yched’s picture

FileSize
17.18 KB
215.26 KB

Keeping up with HEAD practices :
moved to \Drupal methods for state(), config(), moduleHandler()

plach’s picture

The Drupal class is in the global namespace: just Drupal::state() without the leading backslash will work in procedural code.

plach’s picture

Issue summary: View changes

Updated issue summary.

yched’s picture

FileSize
1.05 KB
215.28 KB

Adresses 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

+++ b/core/modules/field/tests/modules/field_test_config/config/field.field.field_test_import.ymlundefined
@@ -0,0 +1,20 @@
+active: 1

active == 0 => disabled?

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.

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.

- 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.

Are we doing anything with the UUID for config entities in CMI generally?

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 ;-)

yched’s picture

FileSize
5.37 KB
215.26 KB

@plach #208 : ah, true. Fixed.

xjm’s picture

+++ b/core/modules/field/lib/Drupal/field/FieldInstanceStorageController.phpundefined
@@ -0,0 +1,36 @@
+ * Note: the class take no special care about importing instances after their
+ * field in importCreate(), since this is guaranteed by the alphabetical order
+ * (field.field.* entries are processed before field.instance.* entries).
+ * @todo Revisit after http://drupal.org/node/1944368.

Wowza. Commented on that issue. :)

+++ b/core/modules/field/lib/Drupal/field/FieldInstanceStorageController.phpundefined
@@ -0,0 +1,36 @@
+class FieldInstanceStorageController extends ConfigStorageController {
...
+    // If the field has been deleted in the same import, the instance will be
+    // deleted by then, and there is nothing left to do. Just return TRUE so
+    // that the file does not get written to active store.
+    if (!$old_config->get()) {
+      return TRUE;
+    }
+    return parent::importDelete($name, $new_config, $old_config);
+  }
+

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.

+++ b/core/modules/field/lib/Drupal/field/FieldInstanceStorageController.phpundefined
@@ -0,0 +1,36 @@
+  /**
+   * Overrides \Drupal\Core\Config\Entity\ConfigStorageController::importDelete().
+   */
+  public function importDelete($name, Config $new_config, Config $old_config) {

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.

xjm’s picture

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().

Alright, this makes sense, even if it's not obvious. I'd definitely agree on doing a followup and not attempting to address that here.

yched’s picture

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.

I 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 :-)

xjm’s picture

Looks like this and #1374116: Move bundle CRUD API out of Field API will conflict.

yched’s picture

FileSize
8.29 KB
215.83 KB

- #188 point 2 was left unanswered

@todo This is a tested behavior... indexes are supposed to be saved in the field.
+    $this->assertEqual($schema['indexes'], $expected_indexes, 'Field type indexes saved by default');

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

-          $name = $this->field_info['storage']['details']['sql'][$rkey][$this->table][$column];
+          $name = $this->field_info['storage details']['sql'][$rkey][$this->table][$column];

Why this API change, and why the space?

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

+ public function importDelete($name, Config $new_config, Config $old_config) {
Per http://drupal.org/node/608152, we should always typehint an interface, not a class.

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.

Status: Needs review » Needs work

The last submitted patch, field_CMI-1735118-215.patch, failed testing.

xjm’s picture

I did a bunch of manual testing. It works!! It totally works. I started by changing some basic things and then deploying them:

  • Changing the order of fields on articles.
  • Adding fields to an article.
  • Deleting fields from an article.
  • Adding an existing field on articles (Tags) to pages.

All of the above worked great. Next, I started trying to break things the best way I knew how:

  1. I set a default value for the tags field on the dev site and then deployed the field to production. Since the default value is stored using the auto-increment ID rather than a UUID, the field happily set the default value of the field to a totally different term on the production site that had the same integer ID.
  2. I set the default value for the tags field to a term with an ID that did not exist on the production site. Thankfully, this did not cause any icky errors; both editing content and editing the content type simply gave me no default value in the field. It did however have the same "feature" as above--when I created a brand new term on the production site, it magically became the default value for my existing field! :)

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:
Only local images are allowed.
Not sure why it keeps disappearing: http://drupal.org/files/wat.png

I also tested the following scenario:

  1. Create a field.
  2. Deploy it.
  3. Add content to the field in production.
  4. Make a change to the field in dev that would not be allowed with data (reducing the max length).
  5. Stage the changed field instance and try to synch.

This (properly) throws this exception:

Drupal\field\FieldUpdateForbiddenException: field_sql_storage cannot change the schema for an existing field with data. in field_sql_storage_field_update_forbid() (line 270 of /Applications/MAMP/htdocs/prod/core/modules/field_sql_storage/field_sql_storage.module).
The website has encountered an error. Please try again later

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.

webchick’s picture

So 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:

1. stick the UUID in the field settings with hte tid (probably trivial)
2. implement hooks in taxonomy to check the UUId when saving the field definition
and either match it to a known uuid, or just dump the default value since it's not relevant
or keep the default UUID and unset the ID, actually

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. ;)

yched’s picture

Reference 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 :-)

yched’s picture

xjm’s picture

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 :-)

Right, 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.

yched’s picture

Status: Needs work » Needs review
FileSize
215.24 KB

- 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.

Status: Needs review » Needs work
Issue tags: -Configuration system

The last submitted patch, field_CMI-1735118-222.patch, failed testing.

xjm’s picture

Status: Needs work » Needs review
Issue tags: +Configuration system

#222: field_CMI-1735118-222.patch queued for re-testing.

xjm’s picture

Issue summary: View changes

add followup issue

yched’s picture

FileSize
8.29 KB
224.26 KB

Still 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]

yched’s picture

Issue summary: View changes

Added 1946404 in followup

swentel’s picture

Issue summary: View changes

Updated issue summary.

yched’s picture

FileSize
14.92 KB
419.28 KB

@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.

Status: Needs review » Needs work

The last submitted patch, field_CMI-1735118-226.patch, failed testing.

xjm’s picture

Thank 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. :)

xjm’s picture

@swentel gave me sandbox access, so going to try to reroll #226 and then get some of the dumb cleanups out of the way.

xjm’s picture

Status: Needs work » Needs review
FileSize
233.64 KB

Merged 8.x. Let's see if it works. :)

xjm’s picture

Issue summary: View changes

Updated issue summary.

swentel’s picture

Issue summary: View changes

Updated issue summary.

swentel’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Alright, 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

  1. Addressed by #226, aside from getField() followup.
  2. Addressed by #1966008: Decide which field and instance properties should be public/protected
  3. Addressed by #226.
  4. Addressed by #1966008: Decide which field and instance properties should be public/protected
  5. Can be a followup. (Needs issue.)
    +++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/Field.phpundefined
    @@ -0,0 +1,463 @@
    +  /**
    +   * Overrides \Drupal\Core\Config\Entity\ConfigEntityBase::__construct().
    +   */
    
    +++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/FieldInstance.phpundefined
    @@ -0,0 +1,423 @@
    +  /**
    +   * Overrides \Drupal\Core\Config\Entity\ConfigEntityBase::__construct().
    +   */
    +  public function __construct(array $values, $entity_type = 'field_instance') {

    I feel like we should document what we expect to have in the $values array here.

    E.g., the exception handling for Field implies that the field type and field_name are required, that the the field_name needs to be a valid machine name.
    For FieldInstance, they imply that entity_type, bundle, and one of field_name and field_uuid are required.

  6. Not yet addressed:
    +++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/Field.phpundefined
    @@ -0,0 +1,463 @@
    +    if (empty($values['id'])) {
    +      $values['id'] = $values['field_name'];
    +      unset($values['field_name']);
    +    }

    Again with the id vs. field_name conversion. Here it looks like id is canonical for the machine anme, and not field_name. But elsewhere, ID means the configuration object name, and the machine name is field_name. I don't get it. :)

  7. The answer to this is "yes" and the followup is #1953408: Remove ArrayAccess BC layer from field config entities.
  8. Resolved in #226.
  9. N/A
  10. May warrant an explicit followup?

    Interesting that we're calling out to field_info_field() here. Which, apparently returns the cached field definition, via a procedural wrapper that wraps the FieldInfo class... presumably some day later in the conversion process this will all happen inside an $entity->load()?

  11. Resolved in attached.
  12. As in #1.
  13. Not yet addressed:
    +++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/FieldInstance.phpundefined
    @@ -0,0 +1,423 @@
    +      if ($field) {
    +        $values['field_name'] = $field->id;
    +      }

    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.

  14. Can be a followup:
    +++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/FieldInstance.phpundefined
    @@ -0,0 +1,423 @@
    +    // Provide defaults.
    +    $values += array(

    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.

  15. Not yet addressed:
    
    +++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/FieldInstance.phpundefined
    @@ -0,0 +1,423 @@
    +      'label' => $values['field_name'],

    This is some magic behavior. We default the field label to its machine name? Does this functionality exist in D7? Seems... odd.

Comment #196

  1. Resolved in #207.
  2. Resolved in attached.
  3. Not yet addressed:
     +++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/Field.phpundefined
    @@ -0,0 +1,463 @@
    +      // Create all per-field-type properties (needed here as long as we have
    +      // settings that impact column definitions).
    +      $this->settings += $field_type['settings'];
     

    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.)

  4. I added the variable to #1775842: [meta] Convert all variables to state and/or config systems. Probably could use an explicit issue.
  5. Maybe could use an explicit followup:
    +++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/Field.phpundefined
    @@ -0,0 +1,463 @@
    +      $storage_type = field_info_storage_types($this->storage['type']);

    Assuming this is legacy and will be replaced later?

  6. Needs followup, or to be changed to a Boolean (I think):
    +++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/Field.phpundefined
    @@ -0,0 +1,463 @@
    +      $this->storage['active'] = 1;
    

    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.

  7. Resolved in attached.
  8. Resolved in attached.
  9. Resolved in #226.
  10. Resolved in attached.
  11. Not yet addressed:
    +++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/Field.phpundefined
    @@ -0,0 +1,463 @@
    +      // Tell the storage engine to update the field. Do this before saving the
    +      // new definition since it still might fail.
    +      $module_handler->invoke($this->storage['module'], 'field_storage_update_field', array($this, $original, $has_data));
    

    I don't quite understand the implications of this hunk; could you clarify?

  12. Fixed in attached.
  13. Fixed in attached.
  14. N/A
  15. Addressed by #1966008: Decide which field and instance properties should be public/protected.
  16. No, it's used in the ArrayAccess dealies and inCrudTest. Uh. And probably in other things once the BC is removed?
  17. Addressed in attached.
  18. Not yet addressed:
    +++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/Field.phpundefined
    @@ -0,0 +1,463 @@
    +      $schema += array('columns' => array(), 'indexes' => array(), 'foreign keys' => array());

    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?

  19. Could use followup:
    +++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/Field.phpundefined
    @@ -0,0 +1,463 @@
    +      if (array_intersect(array_keys($schema['columns']), field_reserved_columns())) {
    

    It looks like field_reserved_columns() returns only 'deleted'. A procedural wrapper to return that seems odd; followup?

  20. Resolved in #226.
  21. Resolved in attached.
  22. N/A

Comment #202

  1. Resolved in #207.
  2. Resolved in #226.
  3. Fixed in attached.
  4. Not yet addressed:
    +++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/FieldInstance.phpundefined
    @@ -0,0 +1,423 @@
    +      // Renaming a bundle on the instance is allowed when an entity's bundle
    +      // is renamed and when field_attach_rename_bundle() does internal
    +      // housekeeping.
    +      if ($this->bundle != $original->bundle && !isset($this->bundle_rename_allowed)) {
    

    I don't see $bundle_rename_allowed documented on the class.

  5. Not yet addressed:
    +++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/FieldInstance.phpundefined
    @@ -0,0 +1,423 @@
    +    // Check widget module.

    Check what about the widget module?

  6. Fixed in attached.
  7. Fixed in attached.
  8. Fixed in attached.

I'll file followups for #211 and #217, as well as the spectacular ways I managed to break my test site this weekend. ;)

xjm’s picture

Basically, #231 is the Cliff's notes, so don't bother reading anything I posted above that. ;)

xjm’s picture

+++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/Field.phpundefined
@@ -271,11 +278,11 @@ public function save() {
+        throw new FieldException(format_string('Attempt to create a field with an ID longer than static::ID_MAX_LENGTH characters: %id', array('%id' => $this->id)));

Ahem, search and replace fail. I'll fix.

xjm’s picture

swentel’s picture

Answers on #231 - part one

Comment #191

  • 5. field_create_*(), field_update_*() document those. Will be addressed in #1953410: [Meta] Remove field_create_*(), field_update_*() and field_delete_*() in favor of just using the ConfigEntity API
  • 6. We left 'field_name' as a property that can be inserted in field_create_field() because if we'd remove that (so rename all 'field_name' to 'id' everywhere in core), we'd get an 700kb patch. Will be removed once the BC layer is removed. Note that the Field class does not have field_name property anymore.
  • 10. We have an issue to move FieldInfo class to the DIC and ultimately killing field.info.inc, at least, that's our dream :) See #1950088: Move FieldInfo class to the DIC
  • 13. This is actually a nice DX behavior we introduced :) In any case, people can pass in either the UUID of the field or the field_name. In this case, field_create_instance was supplied with the UUID and we need to know the field_name, which happens to be on the id property now. Also, this is not the config name with dots, but the effective id, e.g. 'body', or 'field_image'.
  • 14. see 5.
  • 15. This is the label of the instance, which defaults to the field name in case field_create_field is not supplied with a label property, as that is not required. Same behavior as in D7, see _field_write_instance().

So as far I can see, everything is covered, moving on to the next now.

swentel’s picture

Issue summary: View changes

Updated issue summary.

swentel’s picture

Answers on #231 - part two

Comment #196

@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!

swentel’s picture

Answers on #231 - part three

Comment #202

  • 4. So yes, this is all alexpott's fault! ;) Configuration used to have rename() method, but that's gone now so we had to call $instance->set('bundle', $new_bundle)->save() now (also, that one updates the manifest, so it's actually good). Normally we don't allow to update the bundle through field_update_instance() (or $instance->save()), unless the bundle is renamed from the UI or programmatically, say a node bundle (article to article_2). We used to simply update this by a simple query in field_attach_rename_bundle() , but that is now also moved by #1374116: Move bundle CRUD API out of Field API - so I've updated the comment so it reflects the right function (field_entity_bundle_rename) for now. It's internal housekeeping, so it's a property that nobody should even use, maybe move this to #1966008: Decide which field and instance properties should be public/protected ?
  • 5. Yes, changed to "Get the widget module and settings from the widget type." Ping me on IRC in case the English can be better, then I can fix it quickly :)

So, I guess there are 2 (maybe 1?) things we still need to figure out, hoping to get @yched's thoughts on those.

swentel’s picture

FileSize
2.08 KB
235.33 KB

Here's an updated patch with the latest changes.

swentel’s picture

Issue summary: View changes

Updated issue summary.

yched’s picture

Thanks 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

- @xjm: Interesting that we're calling out to field_info_field() here. Which, apparently returns the cached field definition, via a procedural wrapper that wraps the FieldInfo class... presumably some day later in the conversion process this will all happen inside an $entity->load()?
- @swentel: We have an issue to move FieldInfo class to the DIC and ultimately killing field.info.inc, at least, that's our dream :) See #1950088: Move FieldInfo class to DIC

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

+++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/Field.phpundefined
@@ -0,0 +1,463 @@
+      // Create all per-field-type properties (needed here as long as we have
+      // settings that impact column definitions).
+      $this->settings += $field_type['settings'];

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.

- 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.

+++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/Field.phpundefined
@@ -0,0 +1,463 @@
+      $this->storage['active'] = 1;

- @xjm: 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.
- @swentel: Sure why not, actually a nice Novice follow up ;) See #1966998: Add a constant OR use TRUE/FALSE for active storage vs. inactive storage and other properties

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.

+++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/Field.phpundefined
@@ -0,0 +1,463 @@
+      $schema += array('columns' => array(), 'indexes' => array(), 'foreign keys' => array());

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?

- 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.

yched’s picture

To 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".

yched’s picture

Forgot:

#196 19.

+++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/Field.phpundefined
@@ -0,0 +1,463 @@
+      if (array_intersect(array_keys($schema['columns']), field_reserved_columns())) {

- @xjm: It looks like field_reserved_columns() returns only 'deleted'. A procedural wrapper to return that seems odd; followup?
- @swentel: Not for now, this function is also used in _field_sql_storage_columnname() for instance which does not get a $field object

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.

catch’s picture

I 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.

+++ b/core/modules/field/field.crud.incundefined
@@ -212,90 +73,33 @@ function field_create_field($field) {
+ * @param mixed $field
+ *   Either the \Drupal\field\Plugin\Core\Entity\Field object to update, or a
+ *   field array structure. If the latter, $field['field_name'] must provided;
+ *   it identifies the field that will be updated to match this structure. Any
+ *   other properties of the field that are not specified in $field will be left
+ *   unchanged, so it is not necessary to pass in a fully populated $field
+ *   structure.

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.

yched’s picture

@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

yched’s picture

Also @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.

swentel’s picture

FileSize
235.32 KB

Reroll after #1957148: Replace entity_query() with Drupal::entityQuery() got in, trivial merge in options.module

- edit - haha patch has wrong issue number, oh well ..

swentel’s picture

FileSize
6 KB
234.79 KB

More documentation updates

swentel’s picture

FileSize
2.47 KB
235.06 KB

Ok, last one ...

Can we get an RTBC, pretty please ? :)

Status: Needs review » Needs work

The last submitted patch, 1735118-247.patch, failed testing.

swentel’s picture

Status: Needs work » Needs review
FileSize
636 bytes
235.06 KB

It's in the details.

swentel’s picture

FileSize
814 bytes
235.17 KB

One small tweak after talking to xjm on IRC. This should be the final one people! :)

xjm’s picture

Assigned: swentel » xjm

Alright, doing a final review and filing some followups. :)

webchick’s picture

xjm’s picture

Issue summary: View changes

Add followup

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Oh, and one docs tweak @swentel and I discussed this morning. Also filed #1969136: Move field_reserved_columns() to a static method.

xjm’s picture

Issue summary: View changes

Updated issue summary.

yched’s picture

FileSize
1.48 KB
235.37 KB

Sorry, 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())

xjm’s picture

Assigned: xjm » alexpott
Status: Needs review » Reviewed & tested by the community

Let's ship it. :)

alexpott’s picture

Title: Convert Field API to CMI » Change notice: Convert Field API to CMI
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

Committed to a820153 and pushed to 8.x. Thanks!

yched’s picture

WOOOOOOOOOOHOO !

Huge congrats to @swentel, @alexpott & @xjm for the amazing effort !

Now, on to the followups :-), + reviving "field types as plugins".

yched’s picture

Issue summary: View changes

Updated issue summary.

yched’s picture

So, 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.

yched’s picture

Side 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 ?

xjm’s picture

Filed #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?

xjm’s picture

Issue summary: View changes

new followup

yched’s picture

Trivial followup, should be an easy RTBC : #1969714: Stale class names for FieldInstance in phpdocs
(thus, not adding it to the list in the OP)

swentel’s picture

So, not sure what to document in the change notice right now, vs. what should wait for the followups / removal of the BC layers.

@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 ;)

xjm’s picture

We 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.

andypost’s picture

swentel’s picture

Added 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.

andypost’s picture

Suppose 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

swentel’s picture

Status: Active » Needs review

Updated 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.

yched’s picture

Title: Change notice: Convert Field API to CMI » Convert Field API to CMI
Status: Needs review » Fixed
Issue tags: -Needs change record

Yay, 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)

YesCT’s picture

hm. tag didn't come off. trying again.

andypost’s picture

@yched probably 'field_name' in examples should be changed to 'id', is not it?

yched’s picture

@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 ?

andypost’s picture

@yched so here's inconsistency with _update_8003_field_create_field() that uses
Drupal::config('field.field.' . $field_config['id']) and no field_name at all

yched’s picture

@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'...

yched’s picture

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

yched’s picture

Cross 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 ?)

yched’s picture

Issue summary: View changes

Updated issue summary.