Problem/Motivation

First step in implementing Mini Panels is to create the configuration entity/entities required to store the details. Since these share a number of similarities with Page Manager’s configuration entities, we’ll also abstract those out so semantics and mechanics can be shared between them.

Proposed resolution

  • Create a MiniPanel config entity, analogous to the Page entity
  • Abstract and share code common to them both
  • Abstract the PageVariant config entity into a DisplayVariant config entity that can be used with both MiniPanel and Page entities
  • Abstract and share common interfaces

Remaining tasks

  • Confirm that code being shared is all genuinely common
  • Confirm that having Mini Panel variants is a desired feature
  • Decide on how best to share the code (dsnopek indicated a preference for composition rather than inheritance)
  • Confirm that a common DisplayVariant is both possible and desirable
  • Check that the right methods exist in the right interfaces

User interface changes

Addition of new UIs for Mini Panels that closely resemble Page Manager’s UIs.

API changes

  • Share DisplayVariant between Page and MiniPanel (as opposed to having PageVariant and MiniPanelVariant)
  • Abstract code that would be shared between Page and MiniPanel
  • Common interfaces that PageInterface will extend.

Data model changes

New configuration entities for MiniPanels and DisplayVariant and removal of PageVariant config entity.

CommentFileSizeAuthor
#3 2682449-panels-abstract_display_config-3-do-not-test.patch52.96 KByanniboi
#3 2682449-panels_mini-create_mini_panels_config-3-do-not-test.patch8.93 KByanniboi
#4 2682449-page_manager-refactor_page_manager_config-3-do-not-test.patch88.46 KByanniboi
#7 2682449-panels-create_panels_mini_config-7-do-not-test.patch9.58 KBandrewbelcher
#7 2682449-panels-refactor_display_config-7-do-not-test.patch14.9 KBandrewbelcher
#7 2682449-page_manager-refactory_display_config-7-do-not-test.patch88.18 KBandrewbelcher
#7 2682449-ctools-abstract_display_config-7-do-not-test.patch36.21 KBandrewbelcher
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

andrewbelcher created an issue. See original summary.

andrewbelcher’s picture

andrewbelcher CreditAttribution: andrewbelcher at FreelyGive commented

We felt that having variations on mini panels would be a useful feature as there are times when you would have a mini panels with the same purpose and parameters, but differing variations depending on information from the context. We think this is reasonable as this appears to be the direction other panels eco-system modules are considering (e.g. #1978122: Allow Panelizer variants with fall-back to another variant if access requirements fail).

On the basis of that assumption, we started by mirroring Page/PageVariant for mini panels. In doing this, we found that:

  • MiniPanel/Page share all their code except parts specific to the fact Page is managing an entire page (source of parameters, routes and using the admin theme) and Mini Panels is rendered via a block plugin (source of parameters). We have made both MiniPanels and page extend a new base class; DisplayBase. This may change pending the discussion of composition vs inheritance.
  • PageVariant/MiniPanelVariant were identical with the exception that PageVariant stored a reference to a Page and MiniPanelVariant stored a reference to a MiniPanel. We think that by storing an entity type id and entity id, we can have a common DisplayVariant config entity that works with both Page Manager and Mini Panels.
  • Therefore a lot of the interface could be shared between Page and MiniPanel also (which we’ve put in DisplayInterface.
  • We later found that for Mini Panels, DisplayVariant needed to store the parent display to retrieve context set on it (as config entities are no statically cached), so we have added the ability to store (or lazy load) the parent into DisplayVariant and set it as part of DisplayBase::getVariants(). There are other ways to solve this problem if this is not desireable.
  • The config schema for page and mini_panel are almost identical so have made a common type to contain all the shared elements.

One potential problem with using a shared DisplayVariant is that there is currently code that rebuilds routes when the PageVariant is saved/deleted. We have left that in for now so we don’t break Page Manager, but it will need removal. I think that we can either:

  • Implement hooks in Page Manager to do this
  • Provide a way to inform the parent display entity of changes so it can respond accordingly
yanniboi’s picture

yanniboi CreditAttribution: yanniboi at FreelyGive commented
Status: Active » Needs review
FileSize
2682449-panels-abstract_display_config-3-do-not-test.patch52.96 KB
2682449-panels_mini-create_mini_panels_config-3-do-not-test.patch8.93 KB

Here are some patches :)

We have tried to break things into logical chunks so that there isn’t too much code to review at once.

1 - 2682449-panels-abstract_display_config-3-do-not-test.patch - We have the abstract classes, interfaces, config entities, access and a view builder. Most of this code is identical to page manager, but for the new DisplayBase and DisplayVariant. We also have some schema additions and updates to some tests.

Notes

  • There is also some refactoring of references to ‘page_variant’ in favour of the new ‘display_variant’.
  • As mentioned, when loading variants, the fully loaded display entity with its context is stored on the variant.

2 - 2682449-panels_mini-create_mini_panels_config-3-do-not-test.patch - This is how mini panels is using the abstract classes. MiniPanel simply extends DisplayBase and provides a ConfigEntityType Annotation. We also added some config schema and an info.yml file to the module.

Notes

  • At the moment we are using ‘administer blocks’ for most of our permissions checks, but at some point we may want to add a specific ‘administer mini panels’ permission.

3 - 2682449-page_manager-refactor_page_manager_config-3-do-not-test.patch - This is how we have updated Page Manager to use our abstract classes in Panels. We have stripped out a lot of code from the Page config entity, similarly to mini panels. There are a few extras, for path, parameters and whether the page uses the admin theme. Also included in this patch is us getting rid of PageVariant in favour of DisplayVariant and a LOT of code refactoring all of Page Manager (including tests) to use DisplayVariant.

Notes

  • We haven’t included changes made to page_manager_ui because we are not focussing on UI changes in this issue.
  • We haven’t done anything with translation relating to page_manager_config_translation_info_alter and \Drupal\page_manager\Entity\PageVariantConfigMapper.

General Notes

  • We have made an attempt at getting all tests working, however some tests are not yet 100% there.
yanniboi’s picture

yanniboi CreditAttribution: yanniboi at FreelyGive commented
FileSize
2682449-page_manager-refactor_page_manager_config-3-do-not-test.patch88.46 KB
tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented 
+++ b/config/schema/page_manager.schema.yml
--- a/page_manager.info.yml
+++ b/page_manager.info.yml

+++ b/page_manager.info.yml
@@ -6,3 +6,4 @@ core: 8.x
 dependencies:
   - block
   - ctools
+  - panels

We agreed this would be in CTools, not in Panels.

andrewbelcher’s picture

andrewbelcher CreditAttribution: andrewbelcher at FreelyGive commented

Spoke to tim.plunkett on IRC - I'll get the abstraction patch updated put it in CTools tomorrow morning, but we'll keep it all in this issue for now to make it easier to figure out what's going on. We can break it out into separate issues when we're closer to committable code.

andrewbelcher’s picture

andrewbelcher CreditAttribution: andrewbelcher at FreelyGive commented
FileSize
2682449-ctools-abstract_display_config-7-do-not-test.patch36.21 KB
2682449-page_manager-refactory_display_config-7-do-not-test.patch88.18 KB
2682449-panels-refactor_display_config-7-do-not-test.patch14.9 KB
2682449-panels-create_panels_mini_config-7-do-not-test.patch9.58 KB

Here are the updated patches with the abstracted code moved to CTools.

DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna at Mediacurrent commented
Status: Needs review » Needs work

Sounds like separate issues need to be created and linked for this?

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented

Any reason in minipanels? core has block_content that have fields and allows to apply layouts, also you can add other blocks into this enities

It's really not clear why this functionality added!

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented

You can't use block_content to place actual block plugins.

DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna at Mediacurrent commented
Version: 8.x-3.x-dev » 8.x-4.x-dev