Consider making the createCopy config action optionally fail if the entity display already exists

Uploading patch for composer

I have checked the change with Drupal CMS search track recipes and it works as expected.

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($new_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.

I think optional parameter bool $use_existing = FALSE would be a nice solution here, so that the method doesn't try to load the copy always, but only when use_existing is TRUE. Nice approach.

Updated patch

Forgot about the interface core/lib/Drupal/Core/Entity/Display/EntityDisplayInterface.php

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.

The change is in MR https://git.drupalcode.org/project/drupal/-/merge_requests/9944/diffs . I posted a patch just to use it in composer. Ok, I will remove the change from the interface, but the LayoutBuilder complains that its declaration is different, so this has to stay.

Uploading the new patch for using in composer. The change was pushed to MR.

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.

a.dmitriiev → closed merge request !9944

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 history, but could you have a look? (Not changing status myself.)

Appears to have test failures.

The tests were fixed.

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.

For action method `createCopy` the new optional parameter was added `use_existing`, that allows avoiding the error. The recipe should have this config action:

    core.entity_view_display.node.*.default:
      # Clone the "Default" view display by default.
      createCopy:
        mode: search_index
        use_existing: true

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.
  createFromIfNotExists: 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).

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.

I can confirm that new approach works for Search recipe from Drupal CMS

Checked the changes, they are working properly in the recipe for entities with complex ids.

alexpott → changed the visibility of the branch 3483353-entity-copy-use-existing to hidden.

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 [#3481718].

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