Should \Drupal\Core\Config\Action\Plugin\ConfigAction\SimpleConfigUpdate::apply() error if it processes a config entity

I would say yes, worst case scenario we could put ActionMethod on ConfigEntityBase and encourage that instead

Note that this is going to be a BC-breaking change for early adopters of the recipe system. It's common to use simple_config_update to change config entities in ways for which config actions don't yet exist.

So it might be preferable to raise a deprecation here instead in the 10.x branches, and throw in the 11.x branch. But I leave that determination to @alexpott.

How would a recipe creator alter config entities then? I don't have much confidence that I understand the difference between simple config and config entities, but I believe a metatag default is a config entity.

To override the home page metatag default that the module installs, I have been using simple_config_update:

    # Set Metatag Home page:
    metatag.metatag_defaults.front:
      simple_config_update:
        tags.canonical_url: '[site:url]'
        tags.description: '[node:sa_description|node:sa_seo_description]'
        tags.image_src: '[node:sa_seo_image:entity:field_media_image:sa_social_media_facebook|node:sa_featured_image:entity:field_media_image:sa_social_media_facebook]'
        tags.og_description: '[node:sa_description|node:sa_seo_description]'
        tags.og_image: '[node:sa_seo_image:entity:field_media_image:sa_social_media_facebook|node:sa_featured_image:entity:field_media_image:sa_social_media_facebook]'
        tags.og_image_alt: '[node:sa_seo_image:entity:field_media_image:alt|node:sa_featured_image:entity:field_media_image:alt]'
        tags.og_image_height: '[node:sa_seo_image:entity:field_media_image:sa_social_media_facebook:height|node:sa_featured_image:entity:field_media_image:sa_social_media_facebook:height]'
        tags.og_image_type: '[node:sa_seo_image:entity:field_media_image:sa_social_media_facebook:mimetype|node:sa_featured_image:entity:field_media_image:sa_social_media_facebook:mimetype]'
        tags.og_image_width: '[node:sa_seo_image:entity:field_media_image:sa_social_media_facebook:width|node:sa_featured_image:entity:field_media_image:sa_social_media_facebook:width]'
        tags.og_locale: en_US
        tags.og_site_name: '[site:name]'
        tags.og_title: '[node:sa_seo_page_title|node:title]'
        tags.og_type: website
        tags.og_updated_time: '[node:changed:custom:c]'
        tags.og_url: '[current-page:url:absolute]'
        tags.referrer: unsafe-url
        tags.rights: 'Copyright ©[date:html_year] All rights reserved.'
        tags.shortlink: '[site:url]'
        tags.title: '[node:sa_seo_page_title|node:title] | [site:name]'
        tags.twitter_cards_description: '[node:sa_description|node:sa_seo_description]'
        tags.twitter_cards_image: '[node:sa_seo_image:entity:field_media_image:sa_social_media_x:url|node:sa_featured_image:entity:field_media_image:sa_social_media_x:url]'
        tags.twitter_cards_image_alt: '[node:sa_seo_image:entity:field_media_image:alt|node:sa_featured_image:entity:field_media_image:alt]'
        tags.twitter_cards_title: '[node:sa_seo_page_title|node:title]'
        tags.twitter_cards_type: summary_large_image

Would this be achievable in another way after this proposed change? Would we need to add config_entity_update?

How would a recipe creator alter config entities then?

A module -- either Metatag itself, or a custom module -- would need to provide the appropriate config action(s).

The problem with simple_config_update is that it's a crutch. It also bypasses things like entity save hooks and entity-level validation, which may well be providing critical under-the-hood functionality and sanity checking. So doing a simple_config_update on a config entity right now may appear to work, but have secondary side effects that aren't discovered until later, at which point they'll be nearly impossible to debug.

Is it simply the number of dots in the config file? One dot for simple config, two for config entities?

So in my example, with metatag.metatag_defaults.front which is provided by the metatag module, I will be unable to alter it using recipes after this change? I would need a custom module and an update hook?

Ah, sorry, you did answer my second question:

A module -- either Metatag itself, or a custom module -- would need to provide the appropriate config action(s).

So there would need to be a new config action for altering the different types of config entities.

I'm not totally against this being used for config entities. It's like a release valve - just to get stuff done. I wonder if there is a way we can detect if it is a config entity and then do what the config importer does and map from the config record so we can work with a real entity and save via the entity API rather than the config API. See \Drupal\Core\Config\ConfigImporter::importInvokeOwner() for ideas.

We can definitely detect if it's a config entity (I've been using \Drupal\Core\Config\ConfigManagerInterface::getEntityTypeIdByName() for that). If there's a way to map to an entity from the config record, and then bring in the entity API, I'd rather use that -- best of all worlds.

All config entities have a set method, can we expose that as an action?

Exploring #13 seems like a great idea.

I also really like the idea of #13. set() has its own problems, but it makes perfect sense and will be useful in many, many situations.

+1, saving config entities through config API will bypass API's and things might not work as expected. For example, I tried to add workflow configuration but caches weren't invalidated because content_moderation_workflow_update() didn't run (I did then find the specific config action to do what I wanted).

One thing keep in mind is that set() does not offer the same functionality, specifically it can only replace a whole top-level property, while SimpleConfigUpdate supports the usual dot based syntax to set child keys directly. Maybe set() should be expanded to have similar capabilities.

Made some small nits.

One thing keep in mind is that set() does not offer the same functionality, specifically it can only replace a whole top-level property, while SimpleConfigUpdate supports the usual dot based syntax to set child keys directly. Maybe set() should be expanded to have similar capabilities.

As simpleConfigUpdate is in use by most recipe authors on config entities, I feel like we need a suitable replacement that as @berdir points out in comment #16

Updating the title of this issue with the current direction.

I don't think we can just flat-out throw an exception here. I think we need to deprecate the use of simpleConfigUpdate on config entities.

I'm not entirely sold on the idea of extending set and setMultiple to support nested property paths, because that would be a config action-only special-casing of those methods.

But I'm open to adding a totally new setNested action which does support setting a nested property path (it's okay if it calls $entity->set() internally), and having that be the alternative to simpleConfigUpdate.

Moving this to core.

phenaproxima → changed the visibility of the branch 3439713-should-drupalcoreconfigactionpluginconfigactionsimpleconfigupdateapply-error to hidden.

Took a stab at implementing #19. Still needs test coverage, a change record, and updates to the recipe system documentation.

Discussed at DrupalCon to use the name setEntityProperties and use the simpleConfigUpdate style. set and setMultiple will remain, but the new action will be able to handle all the syntax.

config:
  action:
    node.type.article:
      setEntityProperties:
      # Can work on singular
      foo: bar
      # multiple
      cat: dog
      mouse: bird
      # and nested properties.
      nested.thing: yes

OK, that's a lot more elegant.

So effectively, the new setEntityProperties method will have exactly the same syntax as simpleConfigUpdate. Super clear, super simple conversion path.

Alright, I wrote some tests and updated the issue summary.

Looks good. Marking as RTBC. Will add a change record.

Change record added.

I think we should prevent the UUID key and ID key being changed. It'd be a really really odd thing for this to be able to do. We prevent config install and sync from changing UUIDs keys - see \Drupal\Core\Config\Entity\ConfigEntityStorage::updateFromStorageRecord()

Thanks @sonfd!

Back to RTBC

I'm actually not quite sure this is correct, because:

1. I could be wrong about this, but I don't think those additional expectException() calls are being reached in the test. Usually the test method will exit after the first exception. So this actually needs a new test method.
2. We shouldn't just handle IDs and UUIDs, we should be preventing setProperties() from changing any of the entity's keys.
3. Not all entity types use 'id' and 'uuid' as their ID and UUID keys, respectively. We need to get that from the entity type metadata.

I'll deal with this tomorrow, since it shouldn't be too hard to adjust what we have. Leaving assigned to myself for that reason.

You're right about expectException(). I got output like 1 Test, 5 assertions so I thought they were working, but they were not.

OK, I refactored the test a bit so that the exceptions are actually caught.

I originally had it so that all entity keys are not allowed by the action, but that's going to be too restrictive, it turns out, since the label can also be an entity key, and there's no reason we should forbid something like that from changing. So, for now, only the ID and UUID keys are treated as immutable.

Looks good to me. (And sorry for earlier mistake.)

Updated the change record to add that you can’t change those two keys.

Committed 2778882 and pushed to 11.x. Thanks!