Problem/Motivation

When updating from 8.x-1.3 to 2.0.0-rc8, after the update hook runs and we run config export, we noticed that there were many seemingly unnecessary and unwanted entity_form_display entities that were created.

In our particular case, we have an inordinate number of content types (236). We also have 14 entity_form_mode entities defined for nodes. At the end of the update hook, we end up with 236 * 14 = 3304 new entity_form_display config entities, eg. core.entity_form_display.node.<bundle>.<form_display>.yml, which before running the update we did not have.

We don't want all the new entity_form_display config entities. Are they necessary for the new version of scheduler?

Steps to reproduce

  • Install out of the box Drupal 9.5
  • Install scheduler at version 8.x-1.3
  • Add a entity form mode for nodes (/admin/structure/display-modes/form/add/node), but don't enable it / customize it on any particuler bundle
  • Export config
  • Update to scheduler 2.0.0-rc8, run the update hooks
  • Export config

Expected result

No new entity form displays.

Actual result

New entity_form_display config entites are created for each bundle, for the form mode created above, reflected in the config export.

Proposed resolution

Form modes that aren't already being used for a particular bundle, should be ignored by the scheduler update hook.

Remaining tasks

  • Verify the new entity_form_displays being created are unintentional
  • If unintentional and serve no practical purpose, fix the update hooks

User interface changes

None

API changes

None

Data model changes

Extra entity_view_display's are not created.

Issue fork scheduler-3359790

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

m4olivei created an issue. See original summary.

m4olivei’s picture

m4olivei
he/him
Primary language English
Location Grimsby, ON
commented
Assigned: Unassigned » m4olivei

m4olivei opened merge request !90

m4olivei’s picture

m4olivei
he/him
Primary language English
Location Grimsby, ON
commented
Assigned: m4olivei » Unassigned
Status: Active » Needs review

See merge request.

m4olivei’s picture

m4olivei
he/him
Primary language English
Location Grimsby, ON
commented
Version: 2.0.0-rc8 » 2.x-dev

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

jonathan1055’s picture

jonathan1055 commented

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.

jonathan1055’s picture

jonathan1055 commented

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

m4olivei’s picture

m4olivei
he/him
Primary language English
Location Grimsby, ON
commented

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, and sometimes (as in our case), $form_display->isNew() === TRUE. 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?

jonathan1055’s picture

jonathan1055 commented

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.

jonathan1055’s picture

jonathan1055 commented
Status: Needs review » Fixed

Committed and fixed. Thank you.

m4olivei’s picture

m4olivei
he/him
Primary language English
Location Grimsby, ON
commented

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

jonathan1055’s picture

jonathan1055 commented

I'm going to finish #3336108: Don't run hook_modules_installed during config install 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?

m4olivei’s picture

m4olivei
he/him
Primary language English
Location Grimsby, ON
commented

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.

Status: Fixed » Closed (fixed)

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

matthand’s picture

matthand
he/him
Primary language English
Location Maryland
commented

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?

jonathan1055’s picture

jonathan1055 commented

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

I appreciate the offer of help, thank you. Here are the 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.

jonathan1055’s picture

jonathan1055 commented

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__] =&gt; 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.

jonathan1055’s picture

jonathan1055 commented
Related issues: +#3454624: Scheduler fields for Taxonomy Terms remain hidden when site is configured by script (local drush or Tugboat preview )

Opened #3454624: Scheduler fields for Taxonomy Terms remain hidden when site is configured by script (local drush or Tugboat preview )