Many unwanted entity_form_display entities created when updating to 2.0.0-rc8

See merge request.

Hi m4olivei,
Thank you for taking the time to explain your scenario and to provide such detailed info. The change you suggest looks fine, and if it solves your problem then that's perfect. I will try your steps to reproduce.

On #3320341-19: Disable fields in form displays by default → you said:

Specifically, this code does not consider that the $form_display object here may be new, and therefore, may be able to be skipped. However, the line $form_display->removeComponent($field)->save(); will remove a component it doesn't have (b/c it's new) and then save it, which actually creates it for the first time. I don't think this is intentional. I think any $form_display that is new should be skipped.

Can you explain a bit more in what scenario the form display object will have the isNew() attribute. i.e when does not stop becoming 'new'. Is it after the first save?

Do you think we need to add a test for this change? I am tempted to say not, as it would be a lot of work for a situation we can prove by manual testing. But would be interested to hear your thoughts.

I've sucessfully replicated the probelm, and with the suggested fix it is solved.

Can you explain a bit more in what scenario the form display object will have the isNew() attribute. i.e when does not stop becoming 'new'. Is it after the first save?

Sure thing! So, lets take a look at a bigger piece of the code in question here:

The code is looping through for all entity types, for all bundles, for all form displays for the entity type. The key is line 1370, which will load the form display object:

$form_display = $display_repository->getFormDisplay($entityTypeId, $bundle_id, $display_mode);

If you look at Drupal's source code for this method, it will attempt to load the configuration object from active config, and if it's not found (doesn't exist), it will
create a fresh config entity object. So either way you get this in memory form display object. In the update hook, when save() is
called, for form displays that already existed for the entity typ e/ bundle combo, the expected behavior occurs. For entity type / bundle combos that didn't previously exist in active config, when save() is called, its persisted to active config for the first time. That results in the unintended extra form modes after the update hook runs.

Does that make sense?

Yes that makes perfect sense, and helps a lot. Thanks for taking the time to explain it. I knew it would be worth asking, because as you found the problem, you would have investigated exactly what is going on.

I don't think this is worth the overhead to try to write test coverage, so I will merge as is.

Committed and fixed. Thank you.

Yay! Thanks for the quick turn around jonathan1055. Can I ask what your feel is for when your next release cut will be? Seems like a fair bit of thing have been building since the last RC

https://git.drupalcode.org/project/scheduler/-/compare/2.0.0-rc8...2.x

I'm going to finish 🐛 Don't run hook_modules_installed during config install Fixed then make a new release immediately.

You are right that there have been quite a few commits, but many are "behind the scenes" such as coding standards, tugboat, travis, gitlab-CI. There are not that many actual major code changes. So I'm undeciced whether to make another release candidate (rc9) or just release 2.0.

I think it would be better to have rc9 live for a few weeks, before the full 2.0. What's your view on that?

You are right that there have been quite a few commits, but many are "behind the scenes" such as coding standards, tugboat, travis, gitlab-CI. There are not that many actual major code changes. So I'm undeciced whether to make another release candidate (rc9) or just release 2.0.

I think it would be better to have rc9 live for a few weeks, before the full 2.0. What's your view on that?

Ah, right. I just read through the full diff. Looks light and safe to me from a static review perspective. RC8 has been in the wild since Nov 2022. My $0.02 CAD would be to just go for it, cut the 2.0.0. Release notes for the RC8 stated:

The final rc release before 2.0

Totally up to you, you're clearly a solid maintainer, I'd trust and respect your decision.

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

We are interested in seeing this fix released for Scheduler module. Is it possible to make a new release candidate? What do you need help with?

Hi matthand,
I have just committed the fix to the final 2.0 blocker 🐛 Don't run hook_modules_installed during config install Fixed and released Scheduler 2.0.0 →

I appreciate the offer of help, thank you. There are plenty of 2.x issues to work on → and I'm ok with making a 2.0.1 release soon if there are some major things that need urgent fixing.

I have just found that this change causes a problem for taxonomy terms when initialising a new site, eg in Tugboat build, and the outcome is that the Scheduler input fields remain hidden. For some reason, $form_display->isNew() is true for the vocabulary default display.

entityTypeId => taxonomy_term
bundle_id => tags
display_mode => default
form_display => stdClass Object
(
    [__CLASS__] => Drupal\Core\Entity\Entity\EntityFormDisplay
    [entityTypeId:protected] => entity_form_display
    [enforceIsNew:protected] => 1
    [typedData:protected] => 

At the same point in the site build, the node form, product and media forms all have isNew() false. I don't know why Taxonomy is different.

Opened 🐛 Scheduler fields for Taxonomy Terms remain hidden when site is built for Tugboat preview Active