Problem/Motivation

At the moment, \Drupal\field_ui\Form\FieldStorageAddForm::buildForm retrieves the field category information from hook_field_type_category_info. It might make sense to retrieve this information in \Drupal\Core\Field\FieldTypePluginManager::getGroupedDefinitions.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#3 3371301-3.patch8.48 KBlauriii

Issue fork drupal-3371301

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

lauriii created an issue. See original summary.

amateescu’s picture

amateescu commented
Component: file system » field system
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Status: Active » Needs review
StatusFileSize
new 3371301-3.patch8.48 KB

I realized it would be pretty nice if we could retain the existing behavior of this method because there are existing calls in contrib. Here's a patch that reverts back to the old behavior, which I think probably would be the most sensible thing to do for now.

Maybe we'd like to introduce something on top of this so that \Drupal\field_ui\Form\FieldStorageAddForm::buildForm didn't have to trigger hook_field_type_category_info directly.

smustgrave’s picture

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

This something that could use a change record for the new parameter?

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Status: Needs work » Needs review
Issue tags: -Needs change record

Wrote a draft CR 👍

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative

Thanks! CR looks good.

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 
+++ b/core/lib/Drupal/Core/Field/FieldTypePluginManager.php
@@ -158,20 +158,50 @@ public function getFieldSettingsSummary(FieldDefinitionInterface $field_definiti
+   * @param string $category_label_key
+   *   (optional) The array key to use as the label of the category.
...
+      $category_info[$group]['key'] = $group;

I think this comment should mention the special key value that is also available.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Title: Retrieve field type category information in \Drupal\Core\Field\FieldTypePluginManager::getGroupedDefinitions » [PP-1] Retrieve field type category information in \Drupal\Core\Field\FieldTypePluginManager::getGroupedDefinitions
Status: Needs work » Postponed

We don't have to do #8 if we get #3372097: Consider replacing hook_field_type_category_info with YML based plugin discovery in first. Postponing this until that has been committed.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Title: [PP-1] Retrieve field type category information in \Drupal\Core\Field\FieldTypePluginManager::getGroupedDefinitions » Retrieve field type category information in \Drupal\Core\Field\FieldTypePluginManager::getGroupedDefinitions
Status: Postponed » Needs work

#3372097: Consider replacing hook_field_type_category_info with YML based plugin discovery landed, this needs merging with the changes there.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Issue tags: +ddd2023

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

srishtiiee opened merge request !4459

srishtiiee’s picture

srishtiiee commented
Status: Needs work » Needs review

Rerolled patch #3 in MR !4459

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Can file.module line 1301 also use the constant?

srishtiiee’s picture

srishtiiee commented
Status: Needs work » Needs review

RE:#15 I don't think we can replace it with the FALLBACK_CATEGORY constant as it is unrelated to the field type category system.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

My question in #15 was answered.

longwave’s picture

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

Added a comment about the assertion testing.

longwave’s picture

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

On second thoughts let's just fix that in the linked issue, it doesn't need to hold this up.

longwave’s picture

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

Committed e8568a2 and pushed to 11.x. Thanks!

  • longwave committed e8568a29 on 11.x 
    Issue #3371301 by lauriii, srishtiiee, longwave, smustgrave: Retrieve...

lauriii closed merge request !4459

Status: Fixed » Closed (fixed)

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

quietone’s picture

quietone commented

Published change record.