Problem/Motivation

When a media type is created in the admin UI, a field is automatically created to store the media. The field name is generated automatically:

    $base_id = 'field_media_' . str_replace(static::DERIVATIVE_SEPARATOR, '_', $this->getPluginId());

Here, the field_ prefix in the field name is hardcoded. However, there is a config setting for this: see https://www.drupal.org/node/2060489.

The config value should be respected here.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#4 3218103-4-FAIL.patch1.35 KBphenaproxima

Issue fork drupal-3218103

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

joachim created an issue. See original summary.

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

phenaproxima opened merge request !1056

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Active » Needs review
StatusFileSize
new 3218103-4-FAIL.patch1.35 KB

Fail patch to prove that the fix works as intended. The full fix is in the merge request.

Status: Needs review » Needs work

The last submitted patch, 4: 3218103-4-FAIL.patch, failed testing. View results

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Needs work » Needs review

Fail patch failed; merge request passed. (in Emperor Palpatine voice) Good...good.

joachim’s picture

joachim commented
Status: Needs review » Needs work

Looks good, just one comment about the use of a default value with the config.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented

With the D8 config system, the default value is in config/install, and imported on installation, isn't it? So the only way that the config value can be empty is if the user specifically set it to an empty string, so we should use that in that case.

As @longwave points out, it could also be empty if Field UI isn't installed. So I think we want to handle three cases:

  • Field UI is installed, and there is some value in there: use it as the prefix.
  • Field UI is installed, but there's no prefix: don't have a prefix.
  • Field UI isn't installed, so that config value is null (as opposed to an empty string): use the default field_ prefix.

Maybe what's needed here is to change ?: to ?? so that we detect, and add test coverage for an empty prefix. Oh, and add a comment.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Needs work » Needs review
joachim’s picture

joachim commented

> Field UI isn't installed, so that config value is null (as opposed to an empty string): use the default field_ prefix.

I think that's the right thing to do in terms of the effect... but it violates DRY, and having to differentiate between '' and NULL looks like a code smell to me.

That makes me think that the config value should be moved into the field system, rather than being in Field UI. But one for a separate issue, too much scope creep for here.

joachim’s picture

joachim commented

> That makes me think that the config value should be moved into the field system, rather than being in Field UI. But one for a separate issue, too much scope creep for here.

Although the other question that could be asked here, is: why does a field that is created programatically use the prefix that's intended for user-created fields? More scope creep though!

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
Issue tags: +Needs Review Queue Initiative

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.

Can the MR please be updated for 10.1

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

rpayanm opened merge request !3392

rpayanm’s picture

rpayanm
he/him
Primary language Spanish
commented
Status: Needs work » Needs review

I updated the MR for 10.1

smustgrave’s picture

smustgrave commented

+1 from me but will let another also take a look.

smustgrave’s picture

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

So it doesn't sit here and it got 1+ for RTBC will move it on.

  • catch committed c10a6abe on 10.1.x 
    Issue #3218103 by phenaproxima, rpayanm, joachim: media module creates...
catch’s picture

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

Committed c10a6ab and pushed to 10.1.x. Thanks!

Status: Fixed » Closed (fixed)

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