Ignore this one for reviews, this is just to post new periodic patches for the conversion of field api to cmi, now starting with adding more properties to the config entity objects. Not for the faint of heart ... This happens also in the sandbox, but in a separate branch - http://drupalcode.org/sandbox/yched/1736366.git/shortlog/refs/heads/fiel...

Related #1735118: Convert Field API to CMI

CommentFileSizeAuthor
#165 field_CMI_BC-1906218-165.patch215.24 KByched
#163 field_CMI_BC-1906218-163.patch214.71 KByched
#161 field_CMI_BC-1906218-161.patch214.67 KByched
#160 field_CMI_BC-1906218-160.patch214.6 KByched
#157 field_CMI_BC-1906218-157.patch195.03 KByched
#157 interdiff.txt1.93 KByched
#156 field_CMI_BC-1906218-156.patch204.13 KByched
#156 interdiff.txt11.64 KByched
#154 1906218-154.patch204.45 KBswentel
#154 interdiff.txt644 bytesswentel
#153 1906218-153.patch204.47 KBswentel
#153 interdiff.txt1.9 KBswentel
#149 field_CMI_BC-1906218-149.patch203.59 KBswentel
#149 interdiff.txt999 bytesswentel
#148 field_CMI_BC-1906218-147.patch202.93 KBswentel
#148 interdiff.txt921 bytesswentel
#143 field_CMI_BC-1906218-142.patch203.68 KByched
#142 field_CMI_BC-1906218-142.patch202.76 KBswentel
#142 interdiff.txt4.62 KBswentel
#138 field_CMI_BC-1906218-138.patch203.63 KByched
#138 interdiff.txt15.27 KByched
#133 field_CMI_BC-1906218-135.patch201.62 KByched
#133 interdiff.txt7.84 KByched
#132 field_CMI_BC-1906218-133.patch201.56 KByched
#132 interdiff.txt7.31 KByched
#131 field_CMI_BC-1906218-133.patch202.97 KByched
#131 interdiff.txt7.1 KByched
#129 field_CMI_BC-1906218-129.patch196.1 KByched
#129 interdiff.txt2.4 KByched
#126 field_CMI_BC-1906218-126.patch195.6 KByched
#126 interdiff.txt12.85 KByched
#124 field_CMI_BC-1906218-123.patch188.22 KByched
#124 interdiff.txt4.88 KByched
#121 field_CMI_BC-1906218-121.patch188.68 KByched
#121 interdiff.txt14.46 KByched
#119 field_CMI_BC-1906218-119.patch187.56 KByched
#118 field_CMI_BC-1906218-118.patch175.36 KByched
#115 efq.interdiff.txt2.03 KBswentel
#113 field_CMI_BC-1906218-113.patch159.51 KBswentel
#113 interdiff.txt1.32 KBswentel
#111 field_CMI_BC-1906218-111.patch159.35 KBswentel
#111 interdiff.txt1.16 KBswentel
#109 field_CMI_BC-1906218-109.patch158.87 KByched
#109 interdiff.txt999 bytesyched
#107 field_CMI_BC-1906218-107.patch159.19 KByched
#107 interdiff.txt1.09 KByched
#105 field_CMI_BC-1906218-105.patch158.5 KByched
#105 interdiff.txt1.02 KByched
#103 field_CMI_BC-1906218-103.patch159.25 KByched
#102 field_CMI_BC-1906218-102.patch160.09 KByched
#100 field_CMI_BC-1906218-100.patch159.71 KByched
#99 field_CMI_BC-1906218-99.patch160.56 KByched
#98 1906218-BC-layer.patch156.62 KBswentel
#96 1906218-BC-layer.patch154.63 KBswentel
#91 1906218-BC-layer.patch155.36 KBswentel
#89 1906218-BC-layer.patch154.99 KBswentel
#83 1906218-BC-layer.patch206.36 KBswentel
#81 1906218-BC-layer.patch207.13 KBswentel
#79 1906218-BC-layer.patch207.13 KBswentel
#77 1906218-BC-layer.patch246.2 KBswentel
#75 1906218-BC-layer.patch262.03 KBswentel
#73 1906218-BC-layer.patch284.45 KBswentel
#71 1906218-fieldapi-cmi-71.patch611.42 KBswentel
#69 1906218-fieldapi-cmi-69.patch606.2 KBswentel
#68 fields-instances-do-not-test.patch207.07 KBswentel
#67 1906218-fieldapi-cmi-67.patch580.84 KBswentel
#65 1906218-fieldapi-cmi-65.patch579.37 KBswentel
#63 1906218-fieldapi-cmi-63.patch579.73 KBswentel
#59 1906218-fieldapi-cmi-59.patch578.87 KBswentel
#57 1906218-fieldapi-cmi-57.patch577.97 KBswentel
#55 1906218-fieldapi-cmi-55.patch580 KBswentel
#53 1906218-fieldapi-cmi-53.patch572.65 KBswentel
#51 1906218-fieldapi-cmi-51.patch569.6 KBswentel
#49 1906218-fieldapi-cmi-49.patch566.26 KBswentel
#47 1906218-fieldapi-cmi-47.patch565.42 KBswentel
#45 1906218-fieldapi-cmi-45.patch566.65 KBswentel
#43 1906218-fieldapi-cmi-43.patch566.42 KBswentel
#41 1906218-fieldapi-cmi-41.patch564.72 KBswentel
#39 1906218-fieldapi-cmi-39.patch521.74 KBswentel
#37 1906218-fieldapi-cmi-37.patch411.48 KBswentel
#35 1906218-fieldapi-cmi-34.patch411.47 KBswentel
#33 1906218-fieldapi-cmi-32.patch411.47 KBswentel
#32 1906218-fieldapi-cmi-31.patch411.33 KBswentel
#30 1906218-fieldapi-cmi-29.patch411.3 KBswentel
#27 1906218-fieldapi-cmi-27.patch410.87 KBswentel
#27 interdiff.txt1.61 KBswentel
#24 1906218-fieldapi-cmi-24.patch410.58 KBswentel
#24 interdiff.txt688 bytesswentel
#22 1906218-fieldapi-cmi-22.patch410.43 KBswentel
#20 1906218-fieldapi-cmi-20.patch410.47 KBswentel
#18 1906218-fieldapi-cmi-18.patch410.08 KBswentel
#16 1906218-fieldapi-cmi-16.patch409.73 KBswentel
#14 1906218-fieldapi-cmi-14.patch404.17 KBswentel
#13 1906218-fieldapi-cmi-13.patch393.7 KBswentel
#11 1906218-fieldapi-cmi-11.patch393.7 KBswentel
#9 1906218-fieldapi-cmi-9.patch388.48 KBswentel
#7 1906218-fieldapi-cmi-7.patch377.64 KBswentel
#5 1906218-fieldapi-cmi-7.patch376.69 KBswentel
#3 1906218-fieldapi-cmi-3.patch378.58 KBswentel
#1 1906218-fieldapi-cmi.patch335.7 KBswentel
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

swentel’s picture

Status: Active » Needs review
FileSize
335.7 KB
swentel’s picture

FileSize
378.58 KB

Common bot, login ! :)

swentel’s picture

FileSize
376.69 KB

Should fix lot of tests, including upgrade path.

swentel’s picture

FileSize
377.64 KB

Fixes fatal error in Field UI - more tests free to run - but potentially more exceptions ;)

swentel’s picture

FileSize
388.48 KB

More fixes

swentel’s picture

Fixes in entity reference and forum

swentel’s picture

Field deletion works fine now too

swentel’s picture

FileSize
404.17 KB

Wrong patch

swentel’s picture

FileSize
409.73 KB

More test fixing - moved some properties from 'data' to top level of entity.

swentel’s picture

FileSize
410.08 KB

Should fix the upgrade tests

swentel’s picture

FileSize
410.47 KB

Should fix the imagedisplay tests - debugging on the others now

swentel’s picture

FileSize
410.43 KB

Fixes more translation tests and a logic fix in field_update_field()

swentel’s picture

FileSize
688 bytes
410.58 KB

Fixes options module tests - you have to see the interdiff for this one ....

swentel’s picture

Status: Needs review » Needs work

God damn it, lost hours on those new notices - see http://drupal.org/node/1862758#comment-7035586

swentel’s picture

Status: Needs work » Needs review
FileSize
1.61 KB
410.87 KB

Down to two failures

swentel’s picture

Big chance this is going to be green - although I removed a variable that I introduced relatively early, not sure how that's going to react.

swentel’s picture

And the patch

swentel’s picture

FileSize
411.33 KB

Test 2 for green

swentel’s picture

FileSize
411.47 KB

Should still be fine - changed $field->uuid to $field->id() which returns $this->uuid - still the most unique way for a field as we can delete them.

swentel’s picture

FileSize
411.47 KB

Bah

swentel’s picture

FileSize
411.48 KB

Let's try this

swentel’s picture

FileSize
521.74 KB

Oh well - anyway lets see how tests behave with field instance config objects

swentel’s picture

FileSize
564.72 KB

Installation should work now

swentel’s picture

FileSize
566.42 KB

remove key exceptions

swentel’s picture

FileSize
566.65 KB

Simple fix in text_field_load() - frees up lots of tests - last of one today

swentel’s picture

FileSize
565.42 KB

Fix tons of notices - and with that tests

swentel’s picture

FileSize
566.26 KB

More fixes

swentel’s picture

More fixes

swentel’s picture

FileSize
572.65 KB

Should fix the upgrade path

swentel’s picture

Should fix a ton more tests

swentel’s picture

FileSize
577.97 KB

Getting there

swentel’s picture

FileSize
578.87 KB

Let's see

yched’s picture

Not loosing my hopes to get back to this early next week :-/

Is there really no way to keep the old $field['some_property'] syntax working ?
The BlocksNG patch was 400K and drove @xjm half crazy. And we're nearing 50% bigger here.

swentel’s picture

Yeah, I'm just trying to figure out how much this change has impact on the code base - and in a way there could be some major refactoring in more parts, that hasn't been even touched yet.

I'll implement arrayAccess on both entities in another branch to make the patch significantly smaller.

swentel’s picture

FileSize
579.73 KB

Relying on the schema version isn't safe during upgrade.

swentel’s picture

FileSize
579.37 KB

More fixes

swentel’s picture

FileSize
580.84 KB

Should be green ....

swentel’s picture

Here's a diff on only field, field_ui, field_sql and file + a few new files (entities, storage controllers and test module) - it's still relatively big, but maybe a bit more sane to review.

swentel’s picture

Datetime field has landed - not sure if this also affects the custom_block module, but we'll see. Once it's back green, I'll make a smaller diff than #68.

swentel’s picture

FileSize
611.42 KB

Oh damn unit test conversions

swentel’s picture

FileSize
284.45 KB

Using ArrayAccess because this is getting out of hand :)
Let's see what the bot thinks, because I might have missed some obvious reverts

swentel’s picture

FileSize
262.03 KB

Missed some obvious onces, kill previous testrun, patch got smaller

swentel’s picture

FileSize
246.2 KB

A couple of other misses, going to let it run for real now ..

swentel’s picture

FileSize
207.13 KB

Re-renamed widget_settings to widget - what the hell was I thinking

swentel’s picture

FileSize
207.13 KB

And now for real real

swentel’s picture

FileSize
206.36 KB

I'm a moron

andypost’s picture

Status: Needs review » Needs work

Currently most of broken tests are caused by mixing argument

+++ b/core/modules/field_sql_storage/field_sql_storage.moduleundefined
@@ -45,7 +45,7 @@ function field_sql_storage_field_storage_info() {
 function _field_sql_storage_tablename($field) {
   if ($field['deleted']) {
-    return "field_deleted_data_{$field['id']}";
+    return "field_deleted_data_" . $field->generate_table_id();

This is a primary trouble. You converting array to object that does not have the method!!!

+++ b/core/modules/field/field.attach.incundefined
@@ -1170,7 +1170,7 @@ function field_attach_insert(EntityInterface $entity) {
-    $field_id = $field['id'];
+    $field_id = $field['uuid'];

@@ -1211,7 +1211,7 @@ function field_attach_update(EntityInterface $entity) {
-    $field_id = $field['id'];
+    $field_id = $field['uuid'];

@@ -1254,7 +1254,7 @@ function field_attach_delete(EntityInterface $entity) {
-    $field_id = $field['id'];
+    $field_id = $field['uuid'];

@@ -1287,7 +1287,7 @@ function field_attach_delete_revision(EntityInterface $entity) {
-    $field_id = $field['id'];
+    $field_id = $field['uuid'];

+++ b/core/modules/field/field.crud.incundefined
@@ -399,7 +396,7 @@ function field_read_fields($params = array(), $include_additional = array()) {
-      $field_name = $field['id'];
+      $field_name = $field['uuid'];

+++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/FieldEntity.phpundefined
@@ -0,0 +1,219 @@
+ * @Plugin(
+ *   id = "field_entity",
...
+   * The field ID (config name).
+   *
+   * @var string
+   */
+  public $id;
...
+  public $uuid;
...
+  public $field_name;

Any reason using uuid as ID? Also it's confusing because field_entity have id that supposed by DX to be used as $field->id()
I think this would lead to many errors in contrib

+++ b/core/modules/field/field.installundefined
@@ -382,15 +245,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,
+  );

should be different order:

  $dependencies['field'][8001] = array(
    'field_sql_storage' => 8000,
  );

but makes no sense ... there's no relation

+++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/FieldEntity.phpundefined
@@ -0,0 +1,219 @@
+class FieldEntity extends ConfigEntityBase implements \ArrayAccess {
+  /**

+++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/FieldInstance.phpundefined
@@ -0,0 +1,196 @@
+class FieldInstance extends ConfigEntityBase implements \ArrayAccess {
+  /**

needs a blank line before php-doc

swentel’s picture

This is a primary trouble. You converting array to object that does not have the method!!!

Actually it does have the method, so this should work fine.

As to uuid vs id, yeah it should be using $field->id() to begin with. I think I'm just finding out here whether id (which would be the field_name) is safe enough - needs more testing. However, in field land, the uuid is the safest and the unique thing as it's possible to have a deleted field with the same name, but maybe it's safe to use the field name as id, I think yched can confirm this or not.

andypost’s picture

@swentel I seen more then one array to object conversions in path. Also tried locally to fix'em but suppose there's more.

+++ b/core/modules/field_sql_storage/field_sql_storage.installundefined
@@ -12,15 +12,16 @@ function field_sql_storage_schema() {
+    $fields = entity_load_multiple('field_entity');
...
-        $schema += _field_sql_storage_schema($field);
+        $schema += _field_sql_storage_schema((object) $field);

Already object!

+++ b/core/modules/field_sql_storage/field_sql_storage.installundefined
@@ -113,8 +114,8 @@ function field_sql_storage_update_8000(&$sandbox) {
-    $data_table = _field_sql_storage_tablename($field);
-    $revision_table = _field_sql_storage_revision_tablename($field);
+    $data_table = _field_sql_storage_tablename((object) $field);
+    $revision_table = _field_sql_storage_revision_tablename((object) $field);

+++ b/core/modules/field_sql_storage/field_sql_storage.moduleundefined
@@ -45,7 +45,7 @@ function field_sql_storage_field_storage_info() {
 function _field_sql_storage_tablename($field) {
...
+    return "field_deleted_data_" . $field->generate_table_id();

$field is array. So this wont work

+++ b/core/modules/field/field.installundefined
@@ -484,6 +346,122 @@ function field_update_8002() {
+  $fields = db_query("SELECT * FROM {field_config}")->fetchAll(PDO::FETCH_ASSOC);
+  foreach ($fields as $field) {
+    $field['data'] = unserialize($field['data']);
...
+    $schema = (array) module_invoke($field['module'], 'field_schema', (object) $field);

This object does not have the method... so upgrade path broken

swentel’s picture

Yeah, there's a reason I said on IRC to wait before posting feedback as I knew there were problems ;) They work fine now locally, finishing the last ones, will post new tomorrow.

swentel’s picture

Status: Needs work » Needs review
FileSize
154.99 KB

I'm hitting a weird endless loop again somewhere, even though I can install fine ... uploading because patch is even smaller now.

swentel’s picture

FileSize
155.36 KB

Green on upgrade path - well at least one :)

Status: Needs review » Needs work

The last submitted patch, 1906218-BC-layer.patch, failed testing.

yched’s picture

Status: Needs work » Needs review

re: field_name / id / uuid :
Right, obviously there's one too much here.
So yeah, as @swentel said, field_name is unique amongst non-deleted fields, which is what most regular runtime operations need to deal with. The $field['id'] (a serial numeric id in D7) is the unique way to identify a field when considering deleted fields as well.

All config entities have a uuid in D8 anyway - and uuids are better than serial ids since they are deployable (i.e they can reliably be used for : "when importing a field CMI file, is it an update of an existing field, or a 'delete existing + create new' ?").
So we should ditch the serial id altogether and go with field_name & uuid.

I'm not sure what $field->id() should be, though - field_name ? uuid ?
I'd guess field_name (the id of an image style is its machine name, right ?), but I'm not fully up to speed on what ConfigEntities / CMI impose (or just settle as "reasonable DX expectancies") on that regard.

That would mean
$field['id'] --> $field->uuid,
$field['field_name'] --> $field->id()
Which, I agree, looks like fun mind twisting for people coming from D7, but is meaningful / not surprising when considered in the context of the other D8 ConfigEntities - which IMO is what we should be designing for.

yched’s picture

Also, this scenario would mean "when looking a deleted + non-deleted fields, there can be several $field with the same id()", which is not great DX wise.
This strengthens my feeling that we should ultimately get rid of API functions that do "get me all fields, deleted + non-deleted".
Regular API functions return regular (non deleted) fields. If you need to do stuff on non deleted fields as well, you call a separate helper function. Those are different sets of data.

Doesn't need to be done in the initial "Field API / CMI" patch, of course, just saying.

alexpott’s picture

+1 for get rid of API functions that do "get me all fields, deleted + non-deleted"

Those are different sets of data.

I agree 100%

swentel’s picture

FileSize
154.63 KB

Almost there - off until the weekend

swentel’s picture

FileSize
156.62 KB

This should be green.

- edit - yeah had some time on the train ;)

yched’s picture

FileSize
160.56 KB

Started delving into this - I'm still at the periphery, slowly moving to the heart of it.

Merged latest 8.x, and pushed a couple fixes along the way:
1c96a19 Fix missing id -> uuid replacement
68c0b51 Update docs in field.api.php : IDs ->UUIDs where relevant
44d888c Upgrade path: typo in var name (only for deleted fields)
93815d0 Upgrade path: Fix seemingly broken logic around UUID generation and deleted fields / instances

yched’s picture

FileSize
159.71 KB

Just checking whether this change is still needed :

--- a/core/lib/Drupal/Core/Entity/EntityManager.php
@@ -191,7 +191,7 @@ public function processDefinition(&$definition, $plugin_id) {
-    if (isset($definition['base_table'])) {
+    if (isset($definition['base_table']) && drupal_get_schema($definition['base_table']) !== FALSE) {

Status: Needs review » Needs work

The last submitted patch, field_CMI_BC-1906218-100.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
160.09 KB

My bad. Reverted "1c96a19 Fix missing id -> uuid replacement" above.

yched’s picture

FileSize
159.25 KB

And re-checking the EntityManager::processDefinition part.

yched’s picture

OK then, pushed
e3099ce revert the fix for EntityManager::processDefinition(), not needed anymore

yched’s picture

Just checking whether the precautions in field_sql_storage_schema() are still needed.

Status: Needs review » Needs work

The last submitted patch, field_CMI_BC-1906218-105.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
1.09 KB
159.19 KB

Doh - not if (db_table_exists('field_config')) {, dumbass :-/
Interdiff with green #103

Status: Needs review » Needs work

The last submitted patch, field_CMI_BC-1906218-107.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
999 bytes
158.87 KB

OK. What about with just the if (!defined('MAINTENANCE_MODE')) { check ?

Status: Needs review » Needs work

The last submitted patch, field_CMI_BC-1906218-109.patch, failed testing.

swentel’s picture

Status: Needs work » Needs review
FileSize
1.16 KB
159.35 KB

Using EFQ for field_read_fields() - the interesting thing about field_read_fields() though is that it's called very often now due to the menu_link conversion, see #1931900: EFQ calls field_info_field() too liberally

Not committed yet - will wait until this is green - and if we really want it ?

Status: Needs review » Needs work

The last submitted patch, field_CMI_BC-1906218-111.patch, failed testing.

swentel’s picture

Status: Needs work » Needs review
FileSize
1.32 KB
159.51 KB

This is probably better.

Status: Needs review » Needs work

The last submitted patch, field_CMI_BC-1906218-113.patch, failed testing.

swentel’s picture

Status: Needs work » Needs review
FileSize
2.03 KB

Ok here's the interdiff that won't generate exceptions - but bulk delete tests fail because of the way the tests are setup. So either we add the get_me_deleted_fields_function() (and refactor a lot more) or keep that for a follow up.

swentel’s picture

Also added a comment over at #1740378: Implement renames in the import cycle (Link to comment) regarding renames and deletions.

yched’s picture

(merged latest 8.x in the field-configentity-BC branch)

yched’s picture

FileSize
175.36 KB

Now that they're green, merged the changes from my field-configentity-BC-yched branch (and deleted it)

Most notable changes:
- 41225f2 Remove schema info and duplicate storage_* entries from CMI files
Derived data (schema...) is accessible through methods on $field entities
- 711d8b7 Method for $field['storage']['details'] + static caches for "derived data" methods
- 3c312fd Do not export $field['deleted']
- fd482c9 Do not export $instance['deleted']
- ae730a5 Perfrmance: use raw config instead of instance entities for getFieldMap()
- 3b7501a Performance: only load needed fields and instances in getBundleInstances()
- 7a7547e Use $entity->delete() instead of entity_delete_multiple() in field_[CRUD]_*()
- 8a61b83 Hide test fails caused by purge on comment fields - EFQ doesn't know how to select comments by bundle, see #1845372-40: Deleting a field from a non-node entity bundle results in an Entity Field Query Exception

yched’s picture

FileSize
187.56 KB

Work in progress : Start moving field_[CUD]_(field|instance)() to save() / delete() methods.
The goal is to keep the old functions as dumb BC wrappers to minimize the patch size, but have the real Entity API working on $field and $instance objects.

Done :
- field_create_field(), field_update_field(), field_delete_field()
- field_delete_instance()
TODO :
- field_create_instance(), field_update_instance()

Status: Needs review » Needs work

The last submitted patch, field_CMI_BC-1906218-119.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
14.46 KB
188.68 KB

Should fix most test fails.

Remaining fails in LocaleUninstallFrenchTest are triggered by field_sync_field_status() doing $field->save(), which now does a lot more stuff than just updating the config record.
The function should probably act by direct manipulation of the config() data, just like it writes directly to {field_config} rather than field_update_field() in D7/HEAD.

But I won't have the time to tackle this today :-)

Status: Needs review » Needs work

The last submitted patch, field_CMI_BC-1906218-121.patch, failed testing.

swentel’s picture

The TextPlainUnitTest and SQLStorageTest pass on my machine - we might want to change the description of the test though.

yched’s picture

Status: Needs work » Needs review
FileSize
4.88 KB
188.22 KB

OK, this should fix the remaining fails (hopefully I didn't introduce new ones when rerolling)

yched’s picture

OK, pushed the corresponding changes :

5cf2032 field_delete_*() --> Entity API delete() methods
0d5b111 field_(create|update)_field() --> Entity API save() method
d682baa test fixes for FieldEntity::save()
0eb344f move container reads to Drupal::service()

Now, on to field_CU_instance() :-)

[edit: fixed commit links above, wrong commit ids somehow]

yched’s picture

Still very unpolished, but giving the bot something to chew on for tonight :-)

Status: Needs review » Needs work

The last submitted patch, field_CMI_BC-1906218-126.patch, failed testing.

swentel’s picture

Quickly talked something through with alexpott on IRC re: what to return in import* methods.

<alexpott> fire away
 <swentel> it's about this line: if (!empty($handled_by_module)) { }
 <swentel> what exactly should we return in our import{$op} hooks in our storage controller
 <alexpott> that line is a bit smelly eh :)
 <swentel> true or false, or .. yched and I are confused :)
 <swentel> a comment on that line would have done wonders :)
 <alexpott> swentel: I got a patch to completely refactor all this stuff
 <alexpott> return TRUE
 <alexpott> one sec...
 <alexpott> yep
 <alexpott> see ConfigStorageController::importCreate
 <swentel> but returning nothing seems to work too
 <swentel> or our tests lie completely
 <swentel> however, manual tests work fine :)
 <alexpott> yes because all config entities are handled by their storage controller
 <alexpott> the whole check is obselete

So, we know what todo. However, as yched pointed out yesterday already, there's a big chance our import hooks can actually go away, but we at least know what todo now :)

yched’s picture

Status: Needs work » Needs review
FileSize
2.4 KB
196.1 KB

Should fix the fails ?

Status: Needs review » Needs work

The last submitted patch, field_CMI_BC-1906218-129.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
7.1 KB
202.97 KB

#1883152: Field level access for EntityNG introduced to references to the old field_config* tables.

Discussing with @swentel in IRC, the FileFieldWidgetTest fails was apparently the resurgence of an über nasty bug with drupalPost and file uploads, triggered by random field names and their respective orders.

Thus, renamed all test file fields to explicit names rather than random strings, that are a pain to begin with...

yched’s picture

Refactored a bit, double checking with the bot.

yched’s picture

Fun experiment : remove the field_name entry in CMI files (duplicates the id), filling it only for BC at runtime.

yched’s picture

Hah, newly added hal.module wants to install field_config_* tables in its tests. OK, no biggie.

We'll see how the "remove the field_name entry in CMI files" patch in #133 does, for now pushed the changes relative to #132 :

1e79298 field_(create|update)_instance() -> Entity API save() method
3013d92 use 'id' also for the Field 'label' key
fec17cd remove import*() overrides in controllers
ba83901 test fixes
640461a minor refactor in Field::save()
020b290 rename widget plugin property
f4301c2 rename FieldEntity to Field
6fd11ca remove recent upstream references to field_config_* tables

yched’s picture

And :
8e12014 adjust update function for latest changes in the CMI files

yched’s picture

With that - off to the main issue :-)

Status: Needs review » Needs work

The last submitted patch, field_CMI_BC-1906218-135.patch, failed testing.

yched’s picture

FileSize
15.27 KB
203.63 KB

Trying to cleanup / sanitize the use of field_name / field_id in the CMI files :

- field.field.*.yml:
remove 'field_name' (duplicates the 'id') - In the future, we'll be talking about the $field->id, like for any other ConfigEntity, not "the field name".
(that part was already done in patch #133 above)

- field.instance.*.yml:
rename 'field_id' to 'field_uuid' (it's a uuid)
remove 'field_name' (not strictly needed, we have the field_uuid, it's better; for humans, it's present in the file name and 'id' key)

- Adds BC handling of what the legacy "array" properties expect:
$field['field_name'] (is $field->id)
$field['id'] (is $field->uuid)
$instance['field_id'] (is $instance->field_uuid)

$instance->field_name is still there for now (at runtime, not in the CMI file). It should be renamed field_id for consistency, but that will probably have to wait after the old $instance['field_id'] have been cleaned from all the code base...

See what the bot has to say.

Interdiff is with the current field-configentity-BC branch.

The last submitted patch, field_CMI_BC-1906218-138.patch, failed testing.

podarok’s picture

Status: Needs work » Needs review

#138: field_CMI_BC-1906218-138.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, field_CMI_BC-1906218-138.patch, failed testing.

swentel’s picture

Status: Needs work » Needs review
FileSize
4.62 KB
202.76 KB

Store raw config in field.field.deleted and field.instance.deleted.

yched’s picture

FileSize
203.68 KB

[edit: heh, crosspost - that one is a continuation of #138]
Still seems to have fails in upgrade tests, let's see about the rest.

yched’s picture

Stupid me. Still forgot to update field_update_8002() for the new keys in the CMI files, no wonder upgrade tests fail.
They work locally now.

OK, I'll let the bot finish its run see if there are other fails.

yched’s picture

Regarding #142 :

+++ b/core/modules/field/field.moduleundefined
@@ -588,7 +588,9 @@ function field_sync_field_status() {
-      $deleted_fields[$id] = $field;
+      $raw_config = config('field.field.' . $field['id'])->get();
+      $raw_config['deleted'] = TRUE;
+      $deleted_fields[$id] = $raw_config;

Not sure about that part: we're not *deleting* new fields here, we're just updating the data.
If the field is deleted, it's because it was already taken from state() and already has 'deleted' => TRUE.
So we shouldn't be trying to read it back from CMI (will probably fail, it's not in there anymore).

swentel’s picture

Hmm right, good catch, so I just take the config compare with what is changed and then save - or would casting to an array work fine enough too ?

Status: Needs review » Needs work

The last submitted patch, field_CMI_BC-1906218-142.patch, failed testing.

swentel’s picture

Status: Needs work » Needs review
FileSize
921 bytes
202.93 KB

Ok, this is probably better. (for the field.field/instance.deleted raw config storage)

swentel’s picture

FileSize
999 bytes
203.59 KB

And this should fix the cleanup in the config entities from #143

Status: Needs review » Needs work

The last submitted patch, field_CMI_BC-1906218-149.patch, failed testing.

yched’s picture

re #148:
Ah, right - $field is a Field object, so $field->getExportProperties() + explicit reset of 'deleted' to TRUE.
Works, but is not too great.

I think it would make much more sense for field_sync_field_status() to work on raw data altogether (as it does in HEAD).
I.e. at the beginning of the function, get $fields not from field_read_fields() but from config_get_storage_names_with_prefix() + state()->get('field.field.deleted'). My bad for using field_read_fields() here.

[edit: this means the code in the function will need to be updated for the new raw CMI properties ('field_name' -> 'id', etc).
@swentel : I'll try to tackle that monday night if you don't beat me to it]

yched’s picture

Status: Needs work » Needs review

On the "other" patch :-) - thanks for #149 !
Fixed the last missing change in the update func ($config['field_id'] --> $config['field_uuid'] in the db_update('file_usage')), pushed to the BC branch, and rolled to a new patch in the main issue.

swentel’s picture

FileSize
1.9 KB
204.47 KB

Using only raw config - it feels a little bit more convoluted to me, but that's maybe only me.

swentel’s picture

FileSize
644 bytes
204.45 KB

Small change in loop

yched’s picture

Yes, reading collecting the definitions from config involves more LoC, but I still think it makes more sense for the function to operate at one single level - entities or raw config.

Minor nits:
$field_names --> $ids (or just inline the config_get_storage_names_with_prefix() call inside the foreach ?)
$name in foreach () --> $id
$id in foreach() (three of them) --> $uuid

Other than than, I think we're good.

yched’s picture

FileSize
11.64 KB
204.13 KB

Sorry, couldn't stop myself from doing a couple additional refactorings / cleanups...

yched’s picture

Checking why that comment_uninstall() change is needed.
(interdiff is with current field-configentity-BC branch)

Status: Needs review » Needs work

The last submitted patch, field_CMI_BC-1906218-157.patch, failed testing.

swentel’s picture

Status: Needs work » Needs review

#157: field_CMI_BC-1906218-157.patch queued for re-testing.

yched’s picture

My laptop has troubles running upgrade tests :-/

yched’s picture

FileSize
214.67 KB

And so does the testbot...
Let's try this one.

Status: Needs review » Needs work

The last submitted patch, field_CMI_BC-1906218-161.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
214.71 KB

Can't remember whether that if (!defined('MAINTENANCE_MODE')) { check in field_sql_storage_schema() is really needed now...

Status: Needs review » Needs work

The last submitted patch, field_CMI_BC-1906218-163.patch, failed testing.

Anonymous’s picture

Issue summary: View changes

Updated issue summary.

yched’s picture

Status: Needs work » Needs review
FileSize
215.24 KB

Check after reroll.

jibran’s picture

Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary.