Remove the createCopy action from EntityDisplayBase, and make cloneAs compatible with wildcards

This is a flaw(?) in the actual method itself, not the recipe system, so a little housekeeping here...

One possible fix here is to add an optional boolean flag to createCopy() that loads an existing one if possible:

createCopy($mode, bool $use_existing = FALSE)

Not sure what the default behavior should be, though. Probably to keep the current behavior but allow it to be overridden.

Two things:

1. Can you post to the MR instead? Patches are hard to review.
2. We cannot update the interface as that would constitute a backwards compatibility break. That's why $use_existing has a default value -- because as long as it has a default value, it won't break the interface.

Looks sensible to me, but needs test coverage :) Once that's done I'd be fine RTBCing this.

Also, we need to target 11.x here - this changes public API to an extent and therefore I don't know if committers will be comfortable backporting it to 10.4.x. That will be a decision for them to make.

Just a couple of suggested clarifications that might benefit the test, but overall I don't have anything to complain about here.

Just a couple of suggested clarifications that might benefit the test, but overall I don't have anything to complain about here.

I did a hit-and-run review comment because I was curious after seeing the review request in Drupal Slack.

I don't know the full issue history, but could you have a look? (Not changing status myself.)

Appears to have test failures.

I followed the steps to reproduce the issue as outlined below:

1Installed Drupal with the standard profile.
2.Switched to the 3483353-entity-copy-use-existing branch from 11.x.
3.Created a recipe with the createCopy configuration action.
4.Ran the recipe twice and verified the result on the site.

Test result: After running the recipe twice, I am still encountering the same error as mentioned (please refer to the attached video for reference).

Please let me know if there are any steps I might be missing in order to properly reproduce this issue? Thank you.

Hi @a.dmitriiev, thank you for correcting me during the testing.

I went through the verification process again, following the steps outlined in comment #28. (You can check the attached video for reference.)
Test Results:
1.No errors occurred when I ran the same recipe twice.
2.Everything is functioning correctly, and all tests have passed.
Since everything is working as expected, I'm changing the issue status to RTBC.

Drupal CMS's search functionality depends on this patch, and it is therefore a Drupal CMS stable release blocker.

I'm not sure about this. I think with the clone and createCopy config actions we've kind of built new create and createIfNotExists actions and this specific issue has made me ponder if we should have extend those actions to support getting default values from existing entities. That way we'd get both functionalities in a consistent way and we would have the problem we have here where every recipe is going to want to have $use_existing set to TRUE… but obvs we don’t want to change the behaviour of the existing method

That's a good point.

I have opened a new merge request that implements two new config actions:

core.entity_view_display.node.test.new_mode:
  # Wraps `create`, and will err if `core.entity_view_display.node.test.new_mode` already exists.
  createFrom: core.entity_view_display.node.test.existing_mode

image.style.new_style:
  # Wraps `createIfNotExists`, and will not err if `image.style.new_style` already exists.
  createIfNotExistsFrom: image.style.existing_style

The cloneAs and createCopy actions have not yet made it into a stable release, so let's just remove them now and replace them with these more generalized and versatile actions. It turns out this is quite a bit simpler to implement, and the existing test coverage pretty much handles it (you just gotta switch the order of the "arguments", as it were).

There is only one major flaw here that I can see, which is that this gets very awkward when we think about wildcards. If you're trying to create entity displays for every content type and they don't already exist, the action gets bypassed entirely since the config that is being "targeted" for creation doesn't even exist yet. So that might not work for the search recipe. This is tricky.

As I play with this more, I kind of wonder...do we even need to expose createCopy() as an action?

We already changed EntityDisplayBase::set() so that, if you update the ID, the associated properties are updated as well.

And we already have a fail_if_exists flag in cloneAs -- it's a wrapper around both create and createIfNotExists.

So maybe there is actually no bug here at all, and we just should rip out createCopy, because it's not necessary. You could clone entity displays politely like this:

core.entity_view_display.node.foo.teaser:
  cloneAs:
    id: node.foo.search_result
    fail_if_exists: false # This is the default, by the way

The problem here, again, is wildcards. It's hard to do cloning with wildcards. I think that is why we exposed createCopy() as an action in the first place. But that obviously has its own problems. So I am now +1 on removing createCopy().

But we still need to figure out how to support wildcards. One possibility is to change cloneAs so that it supports positional wildcard arguments, something like this:

core.entity_view_display.node.*.teaser:
  cloneAs:
    id: node.${1}.search_result
    fail_if_exists: false

In this example, the ${1} is replaced by the second period-separated component of the original entity's ID. This would be giving cloneAs a superpower, but it would be similar in syntax to inputs and would handle the problem of cloning when wildcards are there.

Adjusting credit.

Tagging for change record update on commit.

Committed and pushed 3de28c0f679 to 11.x and fef85c9a8d8 to 11.1.x and d7ddf065f91 to 10.5.x and 21d16eedcce to 10.4.x. Thanks!

Which CRs need updating?

Updated https://www.drupal.org/node/3481718.