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

no

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

Files: 
CommentFileSizeAuthor
#15 interdiff.txt10.14 KBandypost
#15 drupal8.field-system.2089807-15.patch12.74 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 58,836 pass(es).
[ View ]
#12 drupal8.field-system.2089807-10.patch12.49 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 58,834 pass(es).
[ View ]
#8 drupal8.field-system.2089807-8.patch12.36 KBandypost
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#4 interdiff.txt10.65 KBandypost
#4 drupal8.field-system.2089807-4.patch12.49 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 59,203 pass(es).
[ View ]
#2 drupal8.field-system.2089807-2.patch12.81 KBandypost
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Comments

Berdir’s picture

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

andypost’s picture

Status:Active» Needs review
StatusFileSize
new12.81 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

That's all usage

Status:Needs review» Needs work

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

Anonymous’s picture

Issue summary:View changes

Updated issue summary.

andypost’s picture

Status:Needs work» Needs review
StatusFileSize
new12.49 KB
PASSED: [[SimpleTest]]: [MySQL] 59,203 pass(es).
[ View ]
new10.65 KB

right patch

andypost’s picture

Issue summary:View changes

Updated issue summary.

andypost’s picture

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

Berdir’s picture

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.

yched’s picture

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

andypost’s picture

StatusFileSize
new12.36 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

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

andypost’s picture

Title:Get rid of Field::fieldInfo() in favour of \Drupal::service('field.info')» Rename Field::fieldInfo() to Fields::fieldInfo() for DX

Suppose better title

Status:Needs review» Needs work

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

Berdir’s picture

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

andypost’s picture

Status:Needs work» Needs review
StatusFileSize
new12.49 KB
PASSED: [[SimpleTest]]: [MySQL] 58,834 pass(es).
[ View ]

now file renamed too

andypost’s picture

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

yched’s picture

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

andypost’s picture

Title:Rename Field::fieldInfo() to Fields::fieldInfo() for DX» Rename Field::fieldInfo() to FieldService::fieldInfo() for better DX
StatusFileSize
new12.74 KB
PASSED: [[SimpleTest]]: [MySQL] 58,836 pass(es).
[ View ]
new10.14 KB

Re-rolled for FieldService also changed doc-block

andypost’s picture

Issue summary:View changes

Updated issue summary.

David_Rothstein’s picture

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

yched’s picture

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.

yched’s picture

Issue summary:View changes

added history to related issues

andypost’s picture

Status:Postponed» Closed (duplicate)