Problem/Motivation
I am currently working on a new contrib module to create/manage and reuse layouts for DS/Panels/D8.5 Layout Builder in a UI using the LayoutDiscovery plugin derivatives.
https://www.drupal.org/project/dynamic_layouts
When I deleted a layout, I got the following error in my D8.5 Core layout builder:
(/admin/structure/types/manage/article/display-layout/default)
The "dynamic_layout:my_awesome_layout2" plugin does not exist.
Drupal\layout_builder\Section->getDefaultRegion() (Line: 121)
Drupal\layout_builder\Entity\LayoutBuilderEntityViewDisplay->getDefaultRegion() (Line: 157)
Drupal\Core\Entity\EntityDisplayBase->init() (Line: 143)
Drupal\Core\Entity\EntityDisplayBase->__construct(Array, 'entity_view_display') (Line: 175)
Drupal\Core\Entity\Entity\EntityViewDisplay->__construct(Array, 'entity_view_display') (Line: 322)
Proposed resolution
The layout discovery module makes it possible to implement layouts via:
- *.layout.yml files
- Layout Plugin derivatives
Both of the above cases, can get deleted/renamed. Which will cause errors on the Layout Builder.
I will provide a patch which adds some extra guards, if the layout does not exist anymore and displays a message if a Layout plugin is not found. Also adding red highlighting of the sections where the layout plugin does not exist anymore.
A preview of the patch will be added as a file attachment.
Remaining tasks
- Create a patch for this issue
- Testing
- Code reviewing
Comments
Comment #2
Falco010Created the following patch for the issue described above.
Comment #3
EvaSoek CreditAttribution: EvaSoek commented+1
Comment #4
tim.plunketthttps://cgit.drupalcode.org/dynamic_layouts/tree has no code in it so I can't see how you are computing/storing the derivatives.
This should be fixed via the dependencies system within your plugin/deriver.
Then the only change needed in LB would be this (which we should do anyway).
Comment #5
Falco010Hi @tim.plunkett
Thanks for your quick reaction! Was hoping for a reply from you. Just did a dev release of the dynamic_layouts module so you can have look at how I am using the plugin/deriver in my module. I just followed the standard documentation on:
https://www.drupal.org/docs/8/api/layout-api/how-to-register-layouts#usi...
FYI: the module is still under development :)
Comment #6
Falco010@tim.plunkett When a layout gets renamed/deleted:
Do you agree with me that we also should visually show to the LB that a section layout is not available anymore?
Comment #7
tim.plunkettNo, I do not necessarily agree.
We should use the config dependency system correctly, and the UI where you change the layout derivatives should explain what you are about to affect.
This will add any layout's dependencies to the config. Your module should correctly declare its dependencies on the config entities that define it.
Comment #8
tim.plunkettNote that this has to expand the method that is still slated to be removed in #2939925: Add a getPluginDependencies() method to \Drupal\Core\Plugin\PluginDependencyTrait
Comment #9
tim.plunkettRerolled now that the getPluginDependencies() is fixed.
Comment #10
Falco010Thank you for the patch.
If I understand correctly:
- I need to add config depencies to my layout plugin derivative
- This patch will check if dependencies got removed and it will delete the section (which used the deleted layout plugin) if a dependency got deleted.
I am not really familliar with the config dependencies.
Was reading through the documentation:
https://www.drupal.org/docs/8/api/configuration-api/configuration-entity...
Only the example given there does not really apply for my use case.
How would I get information on which config entities are using my layout plugin? Because Display Suite, Panels & Core LB are able to use my layout plugins.
Comment #11
samuel.mortensonHi @Falco010 - I was looking through the Dynamic Layouts module and I think you would just need to add a dependency to the related Dynamic Layout config entity (https://cgit.drupalcode.org/dynamic_layouts/tree/src/Entity/DynamicLayou...).
You will not need to add dependencies for systems which use your layout like Panels and Display Suite. Your Layout Plugins are derived from Config Entities, so you need to make sure they're dependent on them so that the logic in this patch works.
Comment #12
tim.plunkettI opened #2945083: Declare config dependencies in derived Layout Plugins to help fix Dynamic Layout
Comment #13
Falco010@tim.plunkett Thank you very much! Will do some functional testing ASAP.
Comment #14
tim.plunkettKeep in mind that you will need both that patch AND this patch
Comment #15
tim.plunkettComment #16
Falco010Just tested both of the patches and these solve the issue described above. Thanks for your work @tim.plunket
I will commit & release your Dynamic Layouts patch.
Comment #17
xjmOuchy, this is at least major and might be critical. Thanks @Falco010 for reporting this!
Comment #18
samuel.mortensonMarking as RTBC per my earlier review and #16.
Comment #20
Falco010Comment #21
alexpottIf we remove the entire section should we process it's components? Or should we process the section after the components. Not sure leaving at RTBC and pinging @tim.plunkett in slack.
Comment #22
tim.plunkettThere's no need to process the components after we remove their section.
Interdiff is generated with -w
Comment #23
tim.plunkettTagging as this is currently on our shortlist.
Comment #24
alexpottSince this missed the 8.5.0 ship we need an update path, unfortunately. Something like
views_post_update_table_display_cache_max_age()
to just update all the LayoutBuilderEntityViewDisplay entities. Sorry we didn't get this in earlier.Comment #25
tim.plunkettHere's an update path and update path test.
Comment #27
tim.plunkettMissed @group
Comment #29
tim.plunkettSlowly losing my mind. The PASS patch was correct, the FAIL was not.
Comment #31
tim.plunkettThis is identical to 27-PASS but reuploading so the bot is happy.
Comment #32
alexpottUpdate looks good. The issue of experimental commits during RC was discussed more by @catch & @xjm. At some point in the past we agreed that since experimental module are at a state less than RC we can continue to commit during a RC phase. So once this is RTBC I'll commit to both 8.5.x and 8.6.x.
Comment #33
samuel.mortensonThe interdiffs since my last review look good, marking as RTBC.
Comment #34
alexpottAdding credit for patch reviews.
Comment #35
alexpottCommitted and pushed 2678eabf6e to 8.6.x and bc6f172f55 to 8.5.x. Thanks!
Discussed with @catch and he was +1 to this going in to 8.5.x.
Comment #38
xjmIt looks like this broke HEAD on 8.5.x (but not 8.6.x) Reverting there. Also setting the branch correctly; please set it to the minimum branch that includes an issue.
Comment #40
xjmProbably something else was not backported to 8.5.x and this depends on it.
Comment #41
tim.plunkettThe lack of a backport on #2939925: Add a getPluginDependencies() method to \Drupal\Core\Plugin\PluginDependencyTrait is what caused the fail.
However, that changes non-experimental code, so I'm not sure it's eligible for backport.
Here's a patch with the fix from there copied into the method that is still @todo'd for removal.
Comment #42
EclipseGc CreditAttribution: EclipseGc at Acquia commentedInterdiff looks good to me. RTBC assuming tests pass.
Eclipse
Comment #43
alexpottLet's leave #2939925: Add a getPluginDependencies() method to \Drupal\Core\Plugin\PluginDependencyTrait for 8.6.x it adds a method to a trait so it potentially disruptive.
Comment #44
alexpottSecond time lucky. @tim.plunkett thanks for the quick turn-around. Committed ae5ffa9 and pushed to 8.5.x.