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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

swentel’s picture

Issue tags: +Field API

tagging

yched’s picture

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)

yched’s picture

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

andypost’s picture

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.

yched’s picture

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

andypost’s picture

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

yched’s picture

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.

xjm’s picture

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?

swentel’s picture

We agreed doing this in small chunks.

Berdir’s picture

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

swentel’s picture

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

swentel’s picture

Issue summary: View changes

Link to issues this depends on

swentel’s picture

Issue summary: View changes

Updated issue summary.

swentel’s picture

Issue summary: View changes

Updated issue summary.

swentel’s picture

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

yched’s picture

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

yched’s picture

Issue summary: View changes

Updated issue summary.

yched’s picture

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.

yched’s picture

Issue summary: View changes

API changes

yched’s picture

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

alexpott’s picture

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.

swentel’s picture

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.

swentel’s picture

Issue summary: View changes

duplicate header

yched’s picture

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

swentel’s picture

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

swentel’s picture

Title: [META] Remove ArrayAccess BC layer from field config entities » Remove ArrayAccess BC layer from field config entities
Status: Active » Needs review
FileSize
251.51 KB

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

swentel’s picture

Issue summary: View changes

rename of $field_id variables

andypost’s picture

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)

yched’s picture

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

aspilicious’s picture

FileSize
244.87 KB

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

aspilicious’s picture

FileSize
243.1 KB

Wrong patch

aspilicious’s picture

FileSize
243.1 KB

reupload...

Status: Needs review » Needs work

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

aspilicious’s picture

Status: Needs work » Needs review
FileSize
243.21 KB

reroll hell

Status: Needs review » Needs work

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

andypost’s picture

another round of merge

andypost’s picture

Status: Needs work » Needs review
andypost’s picture

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

Berdir’s picture

Title: Remove ArrayAccess BC layer from field config entities » Remove 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.

andypost’s picture

Title: Remove ArrayAcceshttps://drupal.org/files/drupal8.field-system.1953408-32.patchs BC layer from field config entities » Remove ArrayAccess BC layer from field config entities
FileSize
237.96 KB
1.7 KB

addressed 3 and 4

Status: Needs review » Needs work

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

yched’s picture

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

andypost’s picture

Status: Needs work » Needs review
FileSize
237.96 KB

current sandbox

swentel’s picture

FileSize
235.78 KB

rerolled

Berdir’s picture

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.

aspilicious’s picture

Status: Needs work » Needs review
FileSize
235.58 KB

Reroll...

Status: Needs review » Needs work

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

yched’s picture

Pushed a reroll to the sandbox

aspilicious’s picture

Status: Needs work » Needs review
FileSize
242.24 KB

Lets see what happens now...

aspilicious’s picture

FileSize
245.08 KB

Another update

Status: Needs review » Needs work

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

aspilicious’s picture

Status: Needs work » Needs review
FileSize
250.05 KB

Fixed some issues

Status: Needs review » Needs work

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

aspilicious’s picture

Status: Needs work » Needs review
FileSize
250.06 KB

Mèèh

Status: Needs review » Needs work

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

aspilicious’s picture

Status: Needs work » Needs review
FileSize
250.06 KB

Should kill the exceptions

Status: Needs review » Needs work

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

aspilicious’s picture

Status: Needs work » Needs review
FileSize
249.92 KB

Lets try this one

yched’s picture

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 !

yched’s picture

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

yched’s picture

Status: Reviewed & tested by the community » Needs work
swentel’s picture

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

effulgentsia’s picture

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

swentel’s picture

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.

swentel’s picture

Status: Needs work » Needs review
FileSize
2.45 KB
245.09 KB

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 ?

yched’s picture

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.

swentel’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
245.98 KB
914 bytes

Oh man

swentel’s picture

Issue summary: View changes

Updated issue summary.

alexpott’s picture

Title: Remove ArrayAccess BC layer from field config entities » Change 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
swentel’s picture

Title: Change notice: Remove ArrayAccess BC layer from field config entities » Remove 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.

swentel’s picture

Removing tags

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary.

yched’s picture