Follow up from #1735118: Convert Field API to CMI

Convert array syntax to object syntax in existing code, add type hints for $field & $instance arguments across core functions, update docs.

Commit message:

Issue #1953408 by swentel, aspilicious, andypost, yched: Remove ArrayAccess BC layer from field config entities.

Conversion happens in sandbox
Log of commits
Helper issue is over at #2013679: Helper issue for #1953408 (Remove ArrayAccess BC layer)

API changes

Remove ArrayAccess from Fields and Instance object and replace all array access to object property access.

Direct mapping for most properties:
$instance['label'] -> $instance->label
...

Special cases:
- $field['id'] -> $field->uuid (means renaming a couple $field_id variables for clarity...)
- $field['field_name'] -> $field->name (after #1497374: Switch from Field-based storage to Entity-based storage, ->id() right now)
- $field['columns'] -> $field->getFieldPropertyNames()
- $field['foreign keys'] -> $schema = $field->getSchema(); $schema['foreign keys'];
- $field['bundles'] -> $field->getBundles()
- $instance['field_id'] -> $instance->field_uuid (+ $field_id renames)
- $instance['field_name'] -> $instance->getFieldName()

Note:
This is technically an API break, but the support for the old "array access" syntax has been explicitly mentioned as "temporary, will be removed" in the change notice for "Convert Field API to CMI".
Also, the array access layer has a performance impact (hard to evaluate precisely without writing the actual patch...)

Files: 
CommentFileSizeAuthor
#63 interdiff.txt914 bytesswentel
#63 1953408-63.patch245.98 KBswentel
PASSED: [[SimpleTest]]: [MySQL] 58,695 pass(es).
[ View ]
#60 1953408-60.patch245.09 KBswentel
FAILED: [[SimpleTest]]: [MySQL] 54,303 pass(es), 227 fail(s), and 250 exception(s).
[ View ]
#60 interdiff.txt2.45 KBswentel
#53 1953408-53.patch249.92 KBaspilicious
PASSED: [[SimpleTest]]: [MySQL] 58,925 pass(es).
[ View ]
#51 1953408-51.patch250.06 KBaspilicious
FAILED: [[SimpleTest]]: [MySQL] 58,991 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#49 1953408-49.patch250.06 KBaspilicious
FAILED: [[SimpleTest]]: [MySQL] 59,280 pass(es), 2 fail(s), and 1,744 exception(s).
[ View ]
#47 1953408-47.patch250.05 KBaspilicious
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/rest/lib/Drupal/rest/LinkManager/RelationLinkManager.php.
[ View ]
#45 1953408-45.patch245.08 KBaspilicious
FAILED: [[SimpleTest]]: [MySQL] 59,084 pass(es), 8 fail(s), and 12 exception(s).
[ View ]
#44 1953408-44.patch242.24 KBaspilicious
FAILED: [[SimpleTest]]: [MySQL] 59,140 pass(es), 1 fail(s), and 4 exception(s).
[ View ]
#41 1953408-41.patch235.58 KBaspilicious
FAILED: [[SimpleTest]]: [MySQL] 58,938 pass(es), 21 fail(s), and 11 exception(s).
[ View ]
#38 1953408-38.patch235.78 KBswentel
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1953408-38.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#37 drupal8.field-system.1953408-37.patch237.96 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 58,830 pass(es).
[ View ]
#34 interdiff.txt1.7 KBandypost
#34 drupal8.field-system.1953408-34.patch237.96 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 58,811 pass(es), 6 fail(s), and 2 exception(s).
[ View ]
#32 interdiff.txt2.03 KBandypost
#32 drupal8.field-system.1953408-32.patch238.04 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 59,180 pass(es).
[ View ]
#30 drupal8.field-system.1953408-30.patch237.52 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 59,081 pass(es).
[ View ]
#27 1953408-27.patch243.21 KBaspilicious
FAILED: [[SimpleTest]]: [MySQL] 59,250 pass(es), 0 fail(s), and 36 exception(s).
[ View ]
#25 1953408-24.patch243.1 KBaspilicious
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1953408-24_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#24 1953408-24.patch243.1 KBaspilicious
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#23 1953408-23.patch244.87 KBaspilicious
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/node/lib/Drupal/node/Tests/NodeTokenReplaceTest.php.
[ View ]
#20 1953408-20.patch251.51 KBswentel
PASSED: [[SimpleTest]]: [MySQL] 59,071 pass(es).
[ View ]

Comments

Issue tags:+Field API

tagging

I think we should tackle #1967106: FieldInstance::getField() method first. Then, the patch in this issue will naturally go over all the places where that new $instance::getField() method makes more fluent code.

Easier than the other way around (move all code to the new object syntax, then go over it all again to find the places where $instance::getField() makes sense)

Thus, keeping this one postponed on #1967106: FieldInstance::getField() method

This would be a second pass after #1953410: [Meta] Remove field_create_*(), field_update_*() and field_delete_*() in favor of just using the ConfigEntity API to fix a lot of tests so let's decide on strategy to review it.

@andypost: I don't understand what you mean.
What kind of strategy do you feel is missing here, for what kind of concerns ?

I mean that tests will need a lot of work and get a lot of collisions with #1822000: Remove Drupal\field_test\Plugin\Entity\Type\TestEntity in favor of EntityTest
So better join forces and initially refactor tests in separate issues on per-file or per-functionality base

Right, waiting on #1822000: Remove Drupal\field_test\Plugin\Entity\Type\TestEntity in favor of EntityTest to be sorted out before we mess with the same files makes sense to me.
This issue is already postponed anyway, but adding that to the summary.

Title:Remove ArrayAccess BC layer from field config entities[META] Remove ArrayAccess BC layer from field config entities
Priority:Normal» Critical

Let's go with meta. :) Also, this is release-blocking I think?

We agreed doing this in small chunks.

Status:Postponed» Active

As this is a meta issue now, seems like a lot can already be done outside the files touched in that issue and especially once #2004224: Convert tests using TestEntity to EntityTest (except Field API tests) is in, no?

Unpostponing...

Created a branch in the sandbox 'field-remove-BC' - will start on it slowly and we'll create additional issues like we've done in #1953410: [Meta] Remove field_create_*(), field_update_*() and field_delete_*() in favor of just using the ConfigEntity API

Issue summary:View changes

Link to issues this depends on

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Updated issue summary.

@yched in case you read this.

There's a lot of places where we have instance['field_name']. Now we could do $instance->getField()->id() then of course, however, wouldn't it be interesting to have a $instance->getFieldName() as well. Looks a bit more sane to me.

Yeah, $instance->getField()->id() is the current way, and I agree it's not too great.
+1 on a dedicated method. $instance->getFieldId(), then ?

Issue summary:View changes

Updated issue summary.

re #12 / 13: #1950632: Create a FieldDefinitionInterface and use it for formatters and widgets has added FieldInstance::getFieldName() since then, so this part is solved :-)

Added "API changes" considerations in the OP.

Issue summary:View changes

API changes

@swentel : is this what the field-remove-BC sandbox is about ? Do you have a "status estimation" on what's in there ?

Tagging to approve the API changes necessary to remove the BC layer. The new API has already landed. We need to convert everything to it and remove the BC layer. The removal was announced pre code freeze.

Status update: aspilicious, andypost, pcambra and me have been working on this one periodically, but it's been a while since we merged HEAD on the field-remove-BC branch in the sandbox. I'll kickstart this again next week during drupalaton.

Issue summary:View changes

duplicate header

@swentel: thanks !

- I added a note in the OP about renaming $field_id variables when they now receive a UUID
- is this also the issue where we properly typehint FieldInterface / FieldInstanceInterface in function params ?

@yched yeah I'm typehinting them, see helper issue already.

Title:[META] Remove ArrayAccess BC layer from field config entitiesRemove ArrayAccess BC layer from field config entities
Status:Active» Needs review
StatusFileSize
new251.51 KB
PASSED: [[SimpleTest]]: [MySQL] 59,071 pass(es).
[ View ]

First patch to get reviews on. Given the size, I think it's doable to get this in in one move. I've added the suggested commit message to the IS as well (please update in case I missed someone).

Issue summary:View changes

rename of $field_id variables

I think we should update summary with 2 columns: field and instance
Also usage of settings needs to be cleared according #2074217: Reconsider support for field-level & instance-level settings with the same name ? because some places use getFieldSetting*() but others needs direct access to instance (image field for example)

@andypost: let's not mix issues please. The special case of image field that has to do tricky dance to access its 'default_image' in either the $field or the $instance is discussed at #2074217: Reconsider support for field-level & instance-level settings with the same name ?. It's an edge case.

The motto is:
- code that works on a generic FieldDefinitionInterface (widgets, formatters, field type classes) needs to access settings through ->getSettings(), which returns the "merged" settings.
- code that specifically handles configurable $fields & $instances (housekeeping code in field.module, field_ui...) needs to work with $field->settings and $instance->settings.

StatusFileSize
new244.87 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/node/lib/Drupal/node/Tests/NodeTokenReplaceTest.php.
[ View ]

Keeping up with core. If this one fails I'm going back to the helper issue :)

StatusFileSize
new243.1 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Wrong patch

StatusFileSize
new243.1 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1953408-24_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

reupload...

Status:Needs review» Needs work

The last submitted patch, 1953408-24.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new243.21 KB
FAILED: [[SimpleTest]]: [MySQL] 59,250 pass(es), 0 fail(s), and 36 exception(s).
[ View ]

reroll hell

Status:Needs review» Needs work

The last submitted patch, 1953408-27.patch, failed testing.

StatusFileSize
new237.52 KB
PASSED: [[SimpleTest]]: [MySQL] 59,081 pass(es).
[ View ]

another round of merge

Status:Needs work» Needs review

StatusFileSize
new238.04 KB
PASSED: [[SimpleTest]]: [MySQL] 59,180 pass(es).
[ View ]
new2.03 KB

Cleaned up ImageFormatterBase according #2075095: Widget / Formatter methods signatures needlessly complex
Suppose this patch is ready

Title:Remove ArrayAccess BC layer from field config entitiesRemove ArrayAcceshttps://drupal.org/files/drupal8.field-system.1953408-32.patchs BC layer from field config entities
  1. +++ b/core/lib/Drupal/Core/Entity/DatabaseStorageController.php
    @@ -871,7 +871,7 @@ public function onBundleRename($bundle, $bundle_new) {
    -      if ($field['storage']['type'] == 'field_sql_storage') {
    +      if ($field->storage['type'] == 'field_sql_storage') {

    $field->storage?

    Isn't that gone now? Are we missing test coverage here? The condition can just be removed I assume?

  2. +++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Tests/CustomBlockTypeTest.php
    @@ -79,7 +79,7 @@ public function testCustomBlockTypeEditing() {
    -    $this->assertEqual($instance['label'], 'Block body', 'Body field was found.');
    +    $this->assertEqual($instance->getFieldLabel(), 'Block body', 'Body field was found.');

    A bit strange that we added getFieldLabel() as an alias for label(). I can understand that we added getTitle() on NodeInterface but both methods use label(). Not relevant..

  3. +++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentLanguageTest.php
    @@ -71,8 +71,9 @@ function setUp() {
    -    $field['translatable'] = TRUE;
    +    $field->translatable = TRUE;
         $field->save();
    +

    Interesting, looks like this triggers a bug in git diff --color-words. There, $field->save() is completely green, looks like that was added.

  4. +++ b/core/modules/options/lib/Drupal/options/Tests/OptionsFieldUITest.php
    @@ -232,9 +232,11 @@ function testOptionsAllowedValuesBoolean() {
    +    $this->assertFalse(isset($off_value), 'The off value is not saved into settings');

    This puts the getFieldSetting() on a separate line because isset($field->getFieldSetting()) doesn't work, but isset($variable) on a variable that was set with $variable = .. on the line above is unecessary.

    If you want to check for NULL, why not use assertNull($field->getFieldSetting()) ? That does assert(!isset() internally, which means it's the same as the current code.

Title:Remove ArrayAcceshttps://drupal.org/files/drupal8.field-system.1953408-32.patchs BC layer from field config entitiesRemove ArrayAccess BC layer from field config entities
StatusFileSize
new237.96 KB
FAILED: [[SimpleTest]]: [MySQL] 58,811 pass(es), 6 fail(s), and 2 exception(s).
[ View ]
new1.7 KB

addressed 3 and 4

Status:Needs review» Needs work

The last submitted patch, drupal8.field-system.1953408-34.patch, failed testing.

@Berdir #33.1 : this is fixed in #2086095: Remove remaining references to field_sql_storage, with tests

Status:Needs work» Needs review
StatusFileSize
new237.96 KB
PASSED: [[SimpleTest]]: [MySQL] 58,830 pass(es).
[ View ]

current sandbox

StatusFileSize
new235.78 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1953408-38.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

rerolled

Status:Needs review» Needs work
Issue tags:+Needs issue summary update, +API change, +Field API, +Approved API change

The last submitted patch, 1953408-38.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new235.58 KB
FAILED: [[SimpleTest]]: [MySQL] 58,938 pass(es), 21 fail(s), and 11 exception(s).
[ View ]

Reroll...

Status:Needs review» Needs work

The last submitted patch, 1953408-41.patch, failed testing.

Pushed a reroll to the sandbox

Status:Needs work» Needs review
StatusFileSize
new242.24 KB
FAILED: [[SimpleTest]]: [MySQL] 59,140 pass(es), 1 fail(s), and 4 exception(s).
[ View ]

Lets see what happens now...

StatusFileSize
new245.08 KB
FAILED: [[SimpleTest]]: [MySQL] 59,084 pass(es), 8 fail(s), and 12 exception(s).
[ View ]

Another update

Status:Needs review» Needs work

The last submitted patch, 1953408-45.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new250.05 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/rest/lib/Drupal/rest/LinkManager/RelationLinkManager.php.
[ View ]

Fixed some issues

Status:Needs review» Needs work

The last submitted patch, 1953408-47.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new250.06 KB
FAILED: [[SimpleTest]]: [MySQL] 59,280 pass(es), 2 fail(s), and 1,744 exception(s).
[ View ]

Mèèh

Status:Needs review» Needs work

The last submitted patch, 1953408-49.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new250.06 KB
FAILED: [[SimpleTest]]: [MySQL] 58,991 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

Should kill the exceptions

Status:Needs review» Needs work

The last submitted patch, 1953408-51.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new249.92 KB
PASSED: [[SimpleTest]]: [MySQL] 58,925 pass(es).
[ View ]

Lets try this one

Status:Needs review» Reviewed & tested by the community
Issue tags:+Avoid commit conflicts

Reviewed this in Prague, seen a couple adjustments with @swentel and @aspilicious, this is ready for me.
Thanks a lot @swentel, @andypost, @aspilicious for keeping this patch rolling. Let's get this in !

Er, sorry :-(

  1. +++ b/core/modules/field/lib/Drupal/field/Entity/Field.php
    @@ -454,7 +454,7 @@ public function delete() {
    -  public function getSchema() {
    +  public function getSchema($key = '') {

    Not sure I'm too keen on that :/
    At the very least, the default value should be NULL, not ''.
    Then, the @return phpdoc section for FieldInterface::getSchema() is now incomplete, since the returned array might be a full or a partial schema depending on the $key param.

    Functions whose return structure varies widely depending on a param are not a great pattern IMO. We have some in core, the goal should be to remove them, not to add new ones.

    In the end, this is just a helper to workaround the minor annoyance of PHP 5.3 not allowing $object->mehtod()['key'] - which is fixed in 5.4.
    I'd rather not add confusions to the APIs for this.

  2. +++ b/core/modules/file/file.module
    @@ -1909,7 +1909,7 @@ function file_get_file_references(File $file, $field = NULL, $age = EntityStorag
    -        if (($field_type && $current_field['type'] != $field_type) || ($field && $field['id'] != $current_field['id'])) {
    +        if (($field_type && $current_field->getFieldType() != $field_type) || ($field && $field->getFieldName() != $field_name)) {

    In HEAD, $field['id'] is actually the UUID, so the end of that line should be
    || ($field && $field->uuid() != $current_field->uuid())) {

Status:Reviewed & tested by the community» Needs work

@yched how about adding getSchemaKey() as an extra method then ?

Just quickly searching for getSchema in #53, I don't see where it's called with a $key parameter. What am I missing?

Hmm good question, will look at it.

Rerolling at the moment too, there are some conflicts, should get it in half an hour or so.

Status:Needs work» Needs review
StatusFileSize
new2.45 KB
new245.09 KB
FAILED: [[SimpleTest]]: [MySQL] 54,303 pass(es), 227 fail(s), and 250 exception(s).
[ View ]

So yeah, we don't need it anymore, the entity base storage patch made it obsolete.

Addressed .2 as well - I think we're good to go again ?

Status:Needs review» Reviewed & tested by the community

Thanks! Works for me if green.

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 1953408-60.patch, failed testing.

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new245.98 KB
PASSED: [[SimpleTest]]: [MySQL] 58,695 pass(es).
[ View ]
new914 bytes

Oh man

Issue summary:View changes

Updated issue summary.

Title:Remove ArrayAccess BC layer from field config entitiesChange notice: Remove ArrayAccess BC layer from field config entities
Priority:Critical» Major
Status:Reviewed & tested by the community» Active
Issue tags:-Needs issue summary update+Needs change record

Committed bcc5763 and pushed to 8.x. Thanks!

I had to rebase the patch because it no longer applied :)

git ac https://drupal.org/files/1953408-63.patch
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  245k  100  245k    0     0  83651      0  0:00:03  0:00:03 --:--:-- 89512
error: patch failed: core/modules/number/lib/Drupal/number/Tests/NumberFieldTest.php:101
error: core/modules/number/lib/Drupal/number/Tests/NumberFieldTest.php: patch does not apply

But git took care of the merge.

First, rewinding head to replay your work on top of it...
Applying: initial commit
Using index info to reconstruct a base tree...
M core/modules/comment/lib/Drupal/comment/Tests/CommentNonNodeTest.php
M core/modules/datetime/lib/Drupal/datetime/Tests/DatetimeFieldTest.php
M core/modules/field/lib/Drupal/field/Tests/FormTest.php
M core/modules/field/lib/Drupal/field/Tests/TranslationWebTest.php
M core/modules/forum/forum.module
M core/modules/link/lib/Drupal/link/Tests/LinkFieldTest.php
M core/modules/number/lib/Drupal/number/Tests/NumberFieldTest.php
M core/modules/options/lib/Drupal/options/Tests/OptionsWidgetsTest.php
M core/modules/system/tests/modules/entity_test/entity_test.module
M core/modules/taxonomy/lib/Drupal/taxonomy/Tests/TermFieldMultipleVocabularyTest.php
M core/modules/taxonomy/lib/Drupal/taxonomy/Tests/TermFieldTest.php
Falling back to patching base and 3-way merge...
Auto-merging core/modules/taxonomy/lib/Drupal/taxonomy/Tests/TermFieldTest.php
Auto-merging core/modules/taxonomy/lib/Drupal/taxonomy/Tests/TermFieldMultipleVocabularyTest.php
Auto-merging core/modules/system/tests/modules/entity_test/entity_test.module
Auto-merging core/modules/options/lib/Drupal/options/Tests/OptionsWidgetsTest.php
Auto-merging core/modules/number/lib/Drupal/number/Tests/NumberFieldTest.php
Auto-merging core/modules/link/lib/Drupal/link/Tests/LinkFieldTest.php
Auto-merging core/modules/forum/forum.module
Auto-merging core/modules/field/lib/Drupal/field/Tests/TranslationWebTest.php
Auto-merging core/modules/field/lib/Drupal/field/Tests/FormTest.php
Auto-merging core/modules/datetime/lib/Drupal/datetime/Tests/DatetimeFieldTest.php
Auto-merging core/modules/comment/lib/Drupal/comment/Tests/CommentNonNodeTest.php

Title:Change notice: Remove ArrayAccess BC layer from field config entitiesRemove ArrayAccess BC layer from field config entities
Status:Active» Fixed

Updated https://drupal.org/node/2012896 which mentioned the temporary array access layer. I think we're good here.

Removing tags

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

Issue summary:View changes

Updated issue summary.