Config dependencies are not properly calculated

I am fine merging this myself without a review, and will do before 3.0.0 release if necessary.

I snuck in some boilerplate and comment changes.

@fago: this grew more complicated than I thought, and I need a review of this issue. Apologies for the boilerplate code changes that are now in here.

The complication:

• default CE displays can be used for building CEs in non-default view modes (that have no own CE display)
• If the default CE display has "use layout builder" enabled, then the 'build method' also depends on that view mode's Entity View Display (if it is enabled AND if there is no own CE display)
• I think those entity view displays should be config dependencies.

I would like to add the resulting code as a last minute addition to EntityCeDisplayInterface: getDependencyEntityViewDisplays($allSupportedModes = FALSE), because

• the default ($allSupportedModes = FALSE which returns a single display) can also be used on a 'non-default' EntityCeDisplay, to see whether its relevant 'use layout builder' option is in the specific or the default entity view display. This is non-trivial code. (Not very long, but the methods to use are strange.)
• I believe this method can replace EntityViewDisplay::collectRenderDisplays() (see MR comment)
• I have another use case for this method with $allSupportedModes = TRUE. (Finding out whether "Use layout builder" on the default CE display is relevant for any view modes, in the UI form, i.e. whether I should show the option. I'll use it in 🐛 Improve checkboxes visibility in UI Needs work .)

Review is mostly for concept / interface addition. Of course you can review all code details, but I'm not worried about it. I tested.

thanks, good find!

This seems pretty good, but is quite some new logic/code we have to take care of. Given that, I think we should also add some - at least basic - test coverage to it. E.g. let's calculate dependencies of a couple of our config entities in tests and simply verify the result is as it should be in tests?

@fago summary of changes / recent MR comments:

• I was making things too complicated. The method is now only about current calculated config dependencies (method parameter was dropped).
• The 'view mode === default' case, which you said needed test coverage, is unchanged. The "non-default" code has changed.
• I do believe my way of calculating dependencies dynamically, is correct / what Drupal wants us to do. If I misunderstand, please tell me.

Given the point 3, I request a re-review, and would like to test coverage afterwards. Note that I tested manually and I'm confident of the current implementation....... provided I do not misunderstand (per point 3). I would be fine with doing this in a followup if we want to release v1 now.

(v1 would need this interface addition though.)

I do believe my way of calculating dependencies dynamically, is correct / what Drupal wants us to do. If I misunderstand, please tell me.

yeah, I think it's correct. I was thinking we are not able to cover some edge cases where this might not be auto-updated, but I think this is how it's generally working for config dependencies: If some other config changes, our dependencies might change, and I assume core would trigger the re-calculation of every config upon export. Anyway, I think the current MR is good here!

Thanks for the update, the MR is now very clear and clean, code is good to follow. I agree with merging this now and adding test-coverage in a follow-up. I added a small comment about the interface addition though, but imo that's not a biggie, so please decide on how to move on there and let's get this in!

Thank you! Merged.

Followup created.

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

For completeness:

I made a judgment error / caused unnecessary diversion just-pre-3.0... because it turns out that I could not reuse EntityCeDisplay:: getConfigDependencyEntityViewDisplays() anyway. (The logic turned out to be more complicated and confusing in the form.)

This means I did not need the short-lived public-in-EntityCeDisplayInterface method, so I removed it from the interface and made it protected in #3475342-4: Improve CE UI form options visibility and messages → . (Reasoning: people in theoretical inheriting classes could still call it.)

This is however technically not good: there could be theoretical 'outside' code trying to call the now-protected method. So I propose to do it technically-correctly, and deprecate the public method before we do a new release, in 📌 Deprecate two EntityCeDisplay methods Active .