Problem/Motivation

#3356894: Make field selection less overwhelming by introducing groups Added hook_field_type_category_info to allow modules to define metadata about field type categories.
In doing so, we are putting the onus on modules implementing this hook to also implement a preprocess hook if they need libraries attached to theme an icon

Steps to reproduce

Eg. datetime_range_preprocess_form_element__new_storage_type

Proposed resolution

Allow the info definition to nominate libraries (like we do for layout plugins).
Automatically attach the libraries to the form on their behalf so they don't need to implement two hooks
Remove any such hooks (e.g. datetime range, comment, file, link, media, options, telephone, text)

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3372092

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:

Comments

larowlan created an issue. See original summary.

larowlan’s picture

larowlan’s picture

Issue summary: View changes
lauriii’s picture

Title: Allow hook_field_type_category_info entries to define libraries » [PP-1] Allow hook_field_type_category_info entries to define libraries
Status: Active » Postponed
lauriii’s picture

Title: [PP-1] Allow hook_field_type_category_info entries to define libraries » Allow hook_field_type_category_info entries to define libraries
Status: Postponed » Active
catch’s picture

Title: Allow hook_field_type_category_info entries to define libraries » Allow field_type_categories.yml entries to define libraries

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

tim.plunkett’s picture

Issue tags: +Field UX

lauriii’s picture

Status: Active » Needs review
larowlan’s picture

Status: Needs review » Needs work

Left a review

lauriii’s picture

Status: Needs work » Needs review

Back to NR

larowlan’s picture

Status: Needs review » Reviewed & tested by the community

Wasn't as big a cleanup as I hoped, but still seems cleaner. Thanks!

srishtiiee’s picture

Status: Reviewed & tested by the community » Needs review
tim.plunkett’s picture

Status: Needs review » Needs work
$ grep -nrA3 hook_preprocess_form_element__new_storage_type * | grep icon

core/modules/datetime_range/datetime_range.module-35-  $variables['#attached']['library'][] = 'datetime_range/drupal.datetime_range-icon';
core/modules/comment/comment.module-791-  $variables['#attached']['library'][] = 'comment/drupal.comment-icon';
core/modules/telephone/telephone.module-42-  $variables['#attached']['library'][] = 'telephone/drupal.telephone-icon';
core/modules/link/link.module-73-  $variables['#attached']['library'][] = 'link/drupal.link-icon';
core/modules/media/media.module-540-  $variables['#attached']['library'][] = 'media/drupal.media-icon';

There are still 5 modules using the preprocess hook to add in icon libraries.

tim.plunkett’s picture

@srishtiiee points out that these 5 are in the General category. My suggestion was to switch from a preprocess hook to hook_library_info_alter implementations, with each adding their own icons to a General category library.

lauriii’s picture

I remember I tried to implement this first using hook_library_info_alter but that might be tricky unless we get rid of the library definition and try to inject the assets directly to the library (which seems more hacky than the current solution). The problem is that the library we're adding depends on the existing library, and we want to load the existing assets before the new assets. If we wanted vice versa, it would be simple because we could just make the existing library depend on the new library.

srishtiiee’s picture

Status: Needs work » Needs review
smustgrave’s picture

Status: Needs review » Needs work
Issue tags: +Needs change record

Think with a change like this a CR will be needed.

kim.pepper’s picture

I think we are ok because this updates a feature that has only been added in 11.x and never been released?

smustgrave’s picture

But what if someone used this new feature as a patch? Not sure if that counts though.

tim.plunkett’s picture

Issue tags: -Needs change record

This is an API addition to an unreleased feature, I agree it doesn't need a CR

tim.plunkett’s picture

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

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

Status: Needs work » Needs review
kunal.sachdev’s picture

Status: Needs review » Needs work

Looking great 👍. Needs work for two small nits.

srishtiiee’s picture

Status: Needs work » Needs review
kunal.sachdev’s picture

Status: Needs review » Reviewed & tested by the community
lauriii’s picture

Status: Reviewed & tested by the community » Needs work

Posted one comment on the MR

lauriii’s picture

srishtiiee’s picture

Status: Needs work » Needs review
wim leers’s picture

Title: Allow field_type_categories.yml entries to define libraries » Allow field_type_categories.yml entries to define asset libraries
Status: Needs review » Needs work
Issue tags: -Needs reroll

Reviewed :)

srishtiiee’s picture

Status: Needs work » Needs review

Addressed the review feedback.

wim leers’s picture

Status: Needs review » Needs work

🏓 Almost ready!

srishtiiee’s picture

Status: Needs work » Needs review
kunal.sachdev’s picture

Status: Needs review » Reviewed & tested by the community

Confirmed that there is no need for modules to implement preprocess hook for attaching asset libraries and all such hooks are removed.

lauriii’s picture

Status: Reviewed & tested by the community » Fixed

Committed d418da3 and pushed to 11.x. Thanks!

  • lauriii committed d418da39 on 11.x
    Issue #3372092 by srishtiiee, lauriii, larowlan, tim.plunkett, kunal....
wim leers’s picture

Looks great! 👍

Status: Fixed » Closed (fixed)

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