Problem/Motivation

This is spun out of #3418179: [meta] Make config actions more dynamic. One of the use cases we identified therein is instantiating the same field across every bundle of an entity type. One concrete use I can imagine for this is installing Metatag on a site, and wanting to create a metatags field on every content type.

For example:

field.storage.node.meta_tags:
  addToAllBundles:
    label: Meta tags
    description: 'Adjust your meta tags.'

Proposed resolution

Create a new config action, for field storages only, that allows them to be instantiated on all bundles of their target entity type. Each instance should have the same label and description, which can be specified in the action (as you see above).

The action should assume that the field storage already exists. If it doesn't, then the fields won't be createable - they'll just throw an exception. That's okay.

If one of the fields already exists, by default the action should simply skip over it and do nothing. Respect what's already there. But let's support a fail_if_exists: true flag, just in case -- assuming it's easy enough to do. For example:

field.storage.node.meta_tags:
  addToAllBundles:
    label: Meta tags
    description: 'Adjust your meta tags.'
    fail_if_exists: true

In this case, the action should throw an exception if the field with that name already exists on that bundle. (Truth is, I'm not sure exactly why a recipe author might want to use this behavior, but it seems fairly trivial to add, so I think we can implement it now regardless.)

Remaining tasks

Implement this action and add test coverage.

User interface changes

None.

API changes

A new config action!

Data model changes

TBD, likely none.

Issue fork distributions_recipes-3420521

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

phenaproxima created an issue. See original summary.

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

narendraR opened merge request !74

narendrar’s picture

narendrar
He/Him
Primary language English
Location Jaipur, India
commented
Status: Active » Needs work

I am not sure if configfield.storage.node.meta_tagswould already exists or need to be created when config action is applied. I think it will be present.

From the config file I need to get entity type which will be used to get the related bundles. From bundles lets say article and page, I need to create field.field.node.article.meta_tags and field.field.node.page.meta_tags. Is this assumption correct? What if one of the field already exists?

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue tags: +Needs issue summary update

I am not sure if configfield.storage.node.meta_tagswould already exists or need to be created when config action is applied. I think it will be present.

Based on reading the issue summary, I would also assume that field.storage.<entity type>.<something> already exists prior to applying the config action.

But it should be possible for the same recipe to declare that to be installed.

From the config file I need to get entity type which will be used to get the related bundles. From bundles lets say article and page, I need to create field.field.node.article.meta_tags and field.field.node.page.meta_tags. Is this assumption correct?

Based on the IS: yes. 👍

What if one of the field already exists?

Great question! Again based on the IS (and the parent issue), I'd think that is fine. The purpose is to have this field instantiated on all bundles of a fieldable entity type. If some already exist, that's fine.

But that does suggest that createFieldForEveryBundleIfNotExists would be a better name. Or … perhaps it's better still to accept an additional parameter? So that'd become:

field.storage.node.meta_tags:
  instantiateOnAllBundles:
    label: Meta tags
    description: 'Adjust your meta tags.'
    failIfExists: false

We'll need a detailed rationale for either approach, but either way this can continue for now — let's allow an extra parameter for this for now and then we can still remove it if that is what we decide.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Issue summary: View changes
Issue tags: -Needs issue summary update

Responding to #5:

Based on reading the issue summary, I would also assume that field.storage.. already exists prior to applying the config action.

Yes. The config action should assume the storage exists already.

What if one of the field already exists?

Then the action shouldn't do anything, I think. But I agree with Wim's thoughts - we could parameterize that so that it fails if the field exists.

Updating the issue summary with these clarifications.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented

This looks great to me. It does exactly what I imagined!

I think the code should be streamlined and simplified in some places but I don't have any serious concerns. The one thing is that we still need test coverage for fail_if_exists: true (we need to test what happens when the flag is omitted -- existing fields are silently ignored -- and what happens when the flag is set to true).

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Issue tags: +Needs tests
narendrar’s picture

narendrar
He/Him
Primary language English
Location Jaipur, India
commented
Status: Needs work » Needs review
Issue tags: -Needs tests

Changes done as suggested. Ready for another review.

phenaproxima’s picture

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

This is looking much better, and clearer! I have some thoughts, mostly minor things, and we still need to cover a particular test case. Other than that I think I'm rapidly running out of complaints.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue tags: +DX (Developer Experience)

Nit: It's kinda odd to see fail_if_exists if all config actions seem to be camelCased? Is there another example of that naming scheme elsewhere in Recipes or Config Actions?

narendrar’s picture

narendrar
He/Him
Primary language English
Location Jaipur, India
commented
Status: Needs work » Needs review

Re #11, entity_create:ensure_exists, simple_config_update is what I referred. Should I change this?

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Issue summary: View changes

#12: I think changing them here is out of scope. The plugins are not really named with clear casing; they generally have snake_case IDs (as do most plugins in core); they only seem to be camelCased if they are delegating directly to methods of config entities, which are all named in camelCase.

So, I think instantiate_on_all_bundles is probably the correct name for the plugin, since it is not delegating to a particular method of a config entity.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Needs work

#12 + #13: fair! 👍 Glad to see that there's a pattern at least — didn't realize that at the time of writing!

This is very close!

The logic looks ready 👍

The test coverage: almost — there is one incorrect argument name and there are a few changes that'd make this test coverage lot clearer 🙏

phenaproxima’s picture

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

Pushing back on one point, because it is a pain in the butt.

But otherwise, this feedback has been addressed, I think.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Needs work

Proposed a different testing strategy that seems more viable 😄

phenaproxima’s picture

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

narendrar
He/Him
Primary language English
Location Jaipur, India
commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs followup

A follow-up documentation issue is needed to include this change. Rest looks good to me.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Issue tags: -Needs followup
Related issues: +#3427877: Document the ability to instantiate a field on all bundles of an entity type

Follow-up opened.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs review

I'm not sure about the word instantiate everywhere. It feels odd and overly complex. Why not add_field_to_all_bundles? And change the description etc to match?

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented

No objection! I think that's a clearer name.

I changed it to camelCasing as well (addToAllBundles), to match the entity method actions. I think we are making trouble for ourselves by having some actions be snake_case and others be camelCased - the inconsistency will lead to poor DX.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Issue summary: View changes
phenaproxima’s picture

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

Since this was just a pure rename, I'm restoring RTBC.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Version: 11.x-dev » 10.3.x-dev
Status: Reviewed & tested by the community » Fixed

Committed and pushed 824456063fb to 11.x and f5217923f8d to 10.3.x. Thanks!

  • alexpott committed f5217923 on 10.3.x 
    Issue #3420521 by narendraR, phenaproxima, Wim Leers, alexpott: Create a...

  • alexpott committed 82445606 on 11.x 
    Issue #3420521 by narendraR, phenaproxima, Wim Leers, alexpott: Create a...

Status: Fixed » Closed (fixed)

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