Rename Field::fieldInfo() to FieldService::fieldInfo() for better DX [#2089807]

Updated: Comment #0

Problem/Motivation

There's too much Field classes in core now so it's hard to use them:
- use statement could have collisions (primary for \Drupal\Core\Entity\Field\Field
- DX confusion, only a wrapper for single function
- used only for purpose Field::fieldInfo()

Some name conflicts would be resolved in #2051923: Rename \Drupal\Core\Entity\Field\Field, related subclasses and interfaces to *FieldItemList

Proposed resolution

Replace wrapped function class with \Drupal::service('field.info')->fieldInfo()
Use proper injection of 'field.info' service for controllers and services
Use $this->container->get('field.info') for tests

$ git grep '::fieldInfo'
core/lib/Drupal/Core/Entity/Query/Sql/Tables.php:61:    $field_info = FieldInfo::fieldInfo();
core/modules/content_translation/lib/Drupal/content_translation/Form/TranslatableForm.php:84:    $this->fieldInfo = FieldInfo::fieldInfo()->getField($entity_type, $field
core/modules/editor/editor.module:544:    $settings = Field::fieldInfo()
core/modules/entity_reference/lib/Drupal/entity_reference/Tests/EntityReferenceFieldTest.php:92:    $this->field = Field::fieldInfo()->getField($this->entityType, $this-
core/modules/entity_reference/lib/Drupal/entity_reference/Tests/EntityReferenceFieldTest.php:93:    $instances = Field::fieldInfo()->getBundleInstances($this->entityType
core/modules/field/field.deprecated.inc:186: *   Field::fieldInfo()->getFieldMap().
core/modules/field/field.deprecated.inc:189:  return Field::fieldInfo()->getFieldMap();
core/modules/field/field.deprecated.inc:212: *   Field::fieldInfo()->getField($field_name).
core/modules/field/field.deprecated.inc:215:  return Field::fieldInfo()->getField($entity_type, $field_name);
core/modules/field/field.deprecated.inc:233: *   Field::fieldInfo()->getFieldById($field_id).
core/modules/field/field.deprecated.inc:236:  return Field::fieldInfo()->getFieldById($field_id);
core/modules/field/field.deprecated.inc:258: *   Field::fieldInfo()->getFields().
core/modules/field/field.deprecated.inc:261:  return Field::fieldInfo()->getFields();
core/modules/field/field.deprecated.inc:290: *   Field::fieldInfo()->getInstances(),
core/modules/field/field.deprecated.inc:291: *   Field::fieldInfo()->getInstances($entity_type) or
core/modules/field/field.deprecated.inc:292: *   Field::fieldInfo()->getBundleInstances($entity_type, $bundle_name).
core/modules/field/field.deprecated.inc:295:  $cache = Field::fieldInfo();
core/modules/field/field.deprecated.inc:327: *   Field::fieldInfo()->getInstance($entity_type, $bundle_name, $field_name).
core/modules/field/field.deprecated.inc:330:  return Field::fieldInfo()->getInstance($entity_type, $bundle_name, $field_name);
core/modules/field/field.info.inc:44:  Field::fieldInfo()->flush();
core/modules/field/field.info.inc:88:  return array_filter(Field::fieldInfo()->getFields(), function ($field) {
core/modules/field/field.info.inc:147:  $info = Field::fieldInfo()->getBundleExtraFields($entity_type, $bundle);
core/modules/field/lib/Drupal/field/Plugin/Type/FieldType/ConfigField.php:42:      $instances = FieldAPI::fieldInfo()->getBundleInstances($entity->entityType(), $entity-
core/modules/field/lib/Drupal/field/Tests/FieldImportCreateTest.php:64:    $instances = Field::fieldInfo()->getInstances('entity_test');
core/modules/forum/forum.module:159:    $field = Field::fieldInfo()->getField('node', 'taxonomy_forums');

Remaining tasks

approve api clean-up
file change notice
update existing change notices

User interface changes

API changes

removal of Drupal\field\Field not widely used

#2051923: Rename \Drupal\Core\Entity\Field\Field, related subclasses and interfaces to *FieldItemList
#2089853: TranslatableForm totally broken

#1875086: Improve DX of drupal_container()->get()
#1961548: [Policy, no patch] Decide how the module-specific services are made available as static methods (similar to \Drupal::$service())
#1950088: Move FieldInfo class to the DIC

Comment	File	Size	Author
#15	interdiff.txt	10.14 KB	andypost
#15	drupal8.field-system.2089807-15.patch	12.74 KB	andypost
#15
#12	drupal8.field-system.2089807-10.patch	12.49 KB	andypost
#12
#8	drupal8.field-system.2089807-8.patch	12.36 KB	andypost
#8
#4	interdiff.txt	10.65 KB	andypost
#4	drupal8.field-system.2089807-4.patch	12.49 KB	andypost
#4
#2	drupal8.field-system.2089807-2.patch	12.81 KB	andypost
#2

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

Berdir

German

Switzerland

CreditAttribution: Berdir commented 16 September 2013 at 08:20

Not sure but you should grep for Field::fieldInfo, a lot of the Field:: stuff is something else.

Comment #2

andypost

he/him

Russian

CreditAttribution: andypost commented 16 September 2013 at 08:53

Status:

Active

» Needs review

File	Size
drupal8.field-system.2089807-2.patch	12.81 KB

That's all usage

Comment #3

16 September 2013 at 08:56

Status:

Needs review

» Needs work

The last submitted patch, drupal8.field-system.2089807-2.patch, failed testing.

Comment #3.0

(not verified) CreditAttribution: commented 16 September 2013 at 08:56

Issue summary:

View changes

Updated issue summary.

Comment #4

andypost

he/him

Russian

CreditAttribution: andypost commented 16 September 2013 at 10:16

Status:

Needs work

» Needs review

File	Size
drupal8.field-system.2089807-4.patch	12.49 KB

interdiff.txt	10.65 KB

right patch

Comment #4.0

andypost

he/him

Russian

CreditAttribution: andypost commented 16 September 2013 at 10:16

Issue summary:

View changes

Updated issue summary.

Comment #5

andypost

he/him

Russian

CreditAttribution: andypost commented 16 September 2013 at 10:43

Issue tags:

+DX (Developer Experience), +d8dx

Discussed with alexpott in IRC

alexpott> andypost: honestly the change as it is depresses me. I agree that there is scope for confusion but replacing it with Drupal::service('field.info') feels wrong
andypost: it means we have no clue what is coming back so we lose all the nice typehinting in your editor of choice
andypost: I think we can do better
alexpott, what could be better?
alexpott, Drupal::fieldInfo()?
andypost: not sure at this point
andypost: DX is going to be a big topic at Prague
andypost: and for me this is an example of where we are getting much worse
alexpott, type hitting is less evil because 'use as' is really bad
andypost: like in D7 function discovery and autocompletion is easy for editors but service names and completely undiscoverable and at the moment a bit random too
andypost: I agree with the problem - just not the solution :)
andypost: and I don't have a solution at the moment
alexpott, anyway thanx for opinion, at least controllers should use injection so type hitting is not a probelem for dx in future
andypost: yep for controllers and services constructor injection takes the problem away
andypost: but we need to come up with good solutions for procedural code because it's not going away and I think it is important that we serve up a good DX for such code.
andypost: I think we need to balance everything out
andypost: OO for OO's sake does not win us anything apart from complexity where there shouldn't be

Comment #6

Berdir

German

Switzerland

CreditAttribution: Berdir commented 16 September 2013 at 10:58

The problem is that this *is* the pattern that was defined when the Drupal class was added. I already tried to point out back then that it has a high risk for duplicate class names or back then even clashes (as entity classes were in the main namespace, not /Entity).

Views for example has such a class too (Views), it's cheating a bit because the module name is plural and the entity type is singular, but it's close enough too.

So this isn't just about removing that class, this is just following a defined pattern. It's about changing that pattern.

Comment #7

yched CreditAttribution: yched commented 16 September 2013 at 12:37

@Berdir +1

I already tried to point out back then that it has a high risk for duplicate class names or back then even clashes (as entity classes were in the main namespace, not /Entity).

Views for example has such a class too (Views), it's cheating a bit because the module name is plural and the entity type is singular, but it's close enough too.

And I tried to make those exact same arguments too :-)

Comment #8

andypost

he/him

Russian

CreditAttribution: andypost commented 16 September 2013 at 17:36

File	Size
drupal8.field-system.2089807-8.patch	12.36 KB

So let's rename it to Fields at least, namespace protects the class from clashes for contrib

Comment #9

andypost

he/him

Russian

CreditAttribution: andypost commented 16 September 2013 at 17:37

Title:

Get rid of Field::fieldInfo() in favour of \Drupal::service('field.info')

» Rename Field::fieldInfo() to Fields::fieldInfo() for DX

Suppose better title

Comment #10

16 September 2013 at 17:39

Status:

Needs review

» Needs work

The last submitted patch, drupal8.field-system.2089807-8.patch, failed testing.

Comment #11

Berdir

German

Switzerland

CreditAttribution: Berdir commented 16 September 2013 at 17:48

No, that's not what I meant :)

field.module => Field.php
view*s*.module => Views.php

Fields.php is not following the defined pattern.

I'd rather see something like FieldService(s) and ViewsService(s), but I also argued for \Drupal\Core\Service(s) instead of \Drupal ;)

Comment #12

andypost

he/him

Russian

CreditAttribution: andypost commented 16 September 2013 at 17:54

Status:

Needs work

» Needs review

File	Size
drupal8.field-system.2089807-10.patch	12.49 KB

now file renamed too

Comment #13

andypost

he/him

Russian

CreditAttribution: andypost commented 16 September 2013 at 17:55

So maybe better rename it to FieldInfo()::fieldInfo()?

Comment #14

yched CreditAttribution: yched commented 16 September 2013 at 18:32

FieldInfo::fieldInfo() looks like a WTF and doesn't work when we find out we need to add more services in field.module.

I stick with @Berdir, FieldService(s) / ViewsService(s) +1 on principle, but yeah no way to be consistent with \Drupal :-(

Comment #15

andypost

he/him

Russian

CreditAttribution: andypost commented 17 September 2013 at 17:01

Title:

Rename Field::fieldInfo() to Fields::fieldInfo() for DX

» Rename Field::fieldInfo() to FieldService::fieldInfo() for better DX

File	Size
drupal8.field-system.2089807-15.patch	12.74 KB

interdiff.txt	10.14 KB

Re-rolled for FieldService also changed doc-block

Comment #15.0

andypost

he/him

Russian

CreditAttribution: andypost commented 17 September 2013 at 17:01

Issue summary:

View changes

Updated issue summary.

Comment #16

David_Rothstein CreditAttribution: David_Rothstein commented 19 September 2013 at 04:11

Another option might be FieldModule::fieldInfo()? I think it's more descriptive, and wouldn't be inconsistent with \Drupal since Drupal is not a module...

Comment #17

yched CreditAttribution: yched commented 19 September 2013 at 14:20

Status:

Needs review

» Postponed

#16: why not - works in the sense its less inconsistent with \Drupal. It remains quite obscure as to what the class is useful for, but I guess that's also true of \Drupal, so...

Also, we're not just discussing field.module here, this about a policy across core and contrib modules, so the discussion and proposals
should happen in #1961548: [Policy, no patch] Decide how the module-specific services are made available as static methods (similar to \Drupal::$service()) (which already contains arguments about why the current policy sucks). Copied the above over there, and postponing this issue on that other one.