Problem/Motivation

In the Drupal 8 cycle we removed most info hooks and replaced them with plugins.

In #3356894: Make field selection less overwhelming by introducing groups we added a new one hook_field_type_category_info

That issue represents a huge win for Drupal so we didn't want to slow progress debating whether it should be a hook or plugins.

This issue is to explore that further.

Approach Pros Cons
Info hook
  • lower overhead
  • performance?
  • less discoverable
  • less consistent with other APIs / established standards in D9
  • Need a separate alter hook (which is missing atm)
  • Magic keys for definer and consumer
Plugins (yml)
  • established pattern
  • don't need to know PHP to work with
  • alter hook is simple to add
  • YML is backed by a class, so an API for the consumer
  • Needing to define a plugin manager instead of just firing a hook
  • Magic keys for definer

Steps to reproduce

Proposed resolution

See what the code looks like using YML discovery with an alter hook.
Add a default plugin implementation and change some of the logic in FieldStorageAddForm to it (e.g ->getWeight, ->getDescription, ->isDefault)
Consider bundling in #3372092: Allow field_type_categories.yml entries to define asset libraries too

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3372097

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

larowlan created an issue. See original summary.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Related issues: +#3356894: Make field selection less overwhelming by introducing groups
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented

Converting to plugins would make implementing DX improvements like #3372092: Allow field_type_categories.yml entries to define asset libraries easier too.

srishtiiee made their first commit to this issue’s fork.

srishtiiee opened merge request !4318

srishtiiee’s picture

srishtiiee commented
Status: Active » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Some unresolved threads.

srishtiiee’s picture

srishtiiee commented
Status: Needs work » Needs review
tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented
Status: Needs review » Needs work
Issue tags: +Needs tests

Looking good. Glad we are doing this!

we also need a test for the new plugin manager

srishtiiee’s picture

srishtiiee commented
Status: Needs work » Needs review
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Needs review » Needs work

Left a review, also updated issue credits

srishtiiee’s picture

srishtiiee commented
Status: Needs work » Needs review
Issue tags: -Needs tests
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs followup, +Needs change record updates

This looks good to go.

Adding tag for the followup to change $val to something else per review from @tedbow

Adding 'needs change record updates' to repurpose the one about the info hook to be about the new plugin-type

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Issue tags: +Field UX
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Reviewed & tested by the community » Needs work

Some BC considerations and a bikeshed comment about the naming of this whole thing.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Issue tags: +ddd2023
srishtiiee’s picture

srishtiiee commented
Status: Needs work » Needs review
Issue tags: -Needs change record updates
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Status: Needs review » Reviewed & tested by the community

Confirmed that the feedback has been addressed 👍 I also postponed #3371301: Retrieve field type category information in \Drupal\Core\Field\FieldTypePluginManager::getGroupedDefinitions on this.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Issue tags: -Needs followup

Filed the follow-up issue: #3375816: Rename $val in \Drupal\field_ui\Form\FieldStorageAddForm::buildForm to something more descriptive.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Reviewed & tested by the community » Needs work

One more naming nitpick: plugin managers are singular, e.g. plugin.manager.block or plugin.manager.action and also field plugin managers have a common namespace:

  • plugin.manager.field.field_type
  • plugin.manager.field.widget
  • plugin.manager.field.formatter

Therefore I think the service name for this should be plugin.manager.field.field_type_category

We also should add an interface for this:

interface FieldTypeCategoryManagerInterface extends PluginManagerInterface {
}

We should typehint this interface in the FieldTypePluginManager constructor and docblock (instead of the generic PluginManagerInterface), and then also should add an interface alias so this service can be autowired:

Drupal\Core\Field\FieldTypeCategoryManagerInterface: '@plugin.manager.field.field_type_category'

Also it would be good if the new service was immediately after plugin.manager.field.field_type in core.services.yml as we try to group similar services together.

srishtiiee’s picture

srishtiiee commented
Status: Needs work » Needs review
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Status: Needs review » Reviewed & tested by the community

Confirmed that #20 has been addressed! Thanks for the reviews @longwave and thank you for addressing all the feedback @srishtiiee! 😊

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed f1a52222a7 to 11.x. Thanks!

Will publish the change records.

  • longwave committed f1a52222 on 11.x 
    Issue #3372097 by srishtiiee, lauriii, larowlan, longwave, tedbow:...

Status: Fixed » Closed (fixed)

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