See meta-issue: #2511554: [meta] Move some parts of Page Manager into CTools

VariantAwareInterface is for objects that have a collection of variants. Obviously, Page's from Page Manager fit this description!

However, while nothing else in the Panels eco-system in Drupal 7 has a collection of variants like Page Manager, there really isn't anything special about Pages that says they have to be the only thing. We could one day have some other object which has a collection of variants, and it would be great to reuse the UIs for editing it (or any other code) without having to depend on Page Manager.

One possible user of this could be Panels Everywhere. As discussed with @merlinofchaos on IRC, he never really liked that Panels Everywhere (ab)used Page Manager Pages to store it's variants. Instead we could have Panels Everywhere's storage also implement VariantAwareInterface and then use the same UI from CTools.

Note: A better name for this interface could be VariantCollectionInterface?

This also probably depends on the decision to use or not use Variant config entities.

Remaining tasks

  1. Move VariantAwareInterface (and related tests) into CTools and rename to VariantCollectionInterface
  2. Create a Page Manager issue for refactoring the code to use this interface
  3. Do manual testing that Page Manager still works
  4. Make sure automated tests in both CTools and Page Manager pass
CommentFileSizeAuthor
#15 ctools-variant-collection-2511560-15.patch10.6 KBdsnopek
#13 ctools-variant-collection-2511560-13.patch10.57 KBdsnopek
#8 2511560-add-VariantCollection-7.patch10.54 KBsaltednut
#7 2511560-Add-VariantCollectionInterface-7.patch9.39 KBsaltednut

Comments

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
commented
Issue summary: View changes

Added some notes from an IRC discussion with @merlinofchaos from a couple weeks ago.

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
commented
Issue summary: View changes

Added a TODO to the issue summary

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
commented
Issue summary: View changes

Er, TODOs should be an ordered list!

saltednut’s picture

saltednut
she/her
Primary language English
Location Chicago
commented
saltednut’s picture

saltednut
she/her
Primary language English
Location Chicago
commented
Related issues: +#2511570: Remove Drupal\page_manager\Plugin\PageAwareVariantInterface
saltednut’s picture

saltednut
she/her
Primary language English
Location Chicago
commented
Status: Active » Needs review

Initial pass at this. There is also a page_manager patch at #2550959: Remove VariantAwareInterface, ctools to provide VariantCollection and VariantCollectionInterface instead

saltednut’s picture

saltednut
she/her
Primary language English
Location Chicago
commented
StatusFileSize
new 2511560-Add-VariantCollectionInterface-7.patch9.39 KB
saltednut’s picture

saltednut
she/her
Primary language English
Location Chicago
commented
StatusFileSize
new 2511560-add-VariantCollection-7.patch10.54 KB

Pulling in changes from #2511558: Add Drupal\page_manager\Plugin\VariantCollection from Page Manager to consolidate this refactoring.

VariantCollection moved from page_manager
VariantAwareInterface and VariantAwareTrait renamed and moved from page_manager

The last submitted patch, 7: 2511560-Add-VariantCollectionInterface-7.patch, failed testing.

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
commented
Title: Add Drupal\page_manager\Plugin\VariantAwareInterface from Page Manager » Add Drupal\page_manager\Plugin\VariantAwareInterface and VariantCollection from Page Manager

Re-titling to reflect that #2511558: Add Drupal\page_manager\Plugin\VariantCollection from Page Manager was merged into this issue.

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
commented
Status: Needs review » Reviewed & tested by the community

Code looks good to me! Marking RTBC :-)

eclipsegc’s picture

eclipsegc commented
Status: Reviewed & tested by the community » Needs work 
  1. +++ b/src/Plugin/VariantCollection.php
@@ -0,0 +1,48 @@
+  public function sortHelper($aID, $bID) {

    Does this need to be public? 

  2. +++ b/src/Plugin/VariantCollectionInterface.php
@@ -0,0 +1,54 @@
+interface VariantCollectionInterface {

    No one else in core (in my limited research) has interfaces for plugin collections. Do we need one here?

In general, I'm not entirely sure about these classes. I'll commit them anyway, once we have them to the right state (barring some great argument to the contrary) but I think since this is mostly useful for page_manager and its variants, and the wizard branch of page_manager is increasingly moving toward a solution that includes a variant level config entity, we will ultimately have a 1 to 1 relationship between plugins and config entities. It has been said to me that this might not be the case for a panels_everywhere solution, and I'm wondering if this might be useful for display suite. Let's keep pushing this forward, but I want to make sure we have an actual use case for this code.

Eclipse

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
commented
Status: Needs work » Needs review
StatusFileSize
new ctools-variant-collection-2511560-13.patch10.57 KB

#12.1: Yes. It's overriding a method from the parent which is public AND it's being used as a callback in uasort(), so it has to be public.

#12.2: That interface isn't for the plugin collection, it's for "the thing that has variants". I've made a new patch which renames VariantCollection to VariantPluginCollection, so that it's clearer which bits are the plugin collection, and which aren't.

Status: Needs review » Needs work

The last submitted patch, 13: ctools-variant-collection-2511560-13.patch, failed testing.

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
commented
Status: Needs work » Needs review
StatusFileSize
new ctools-variant-collection-2511560-15.patch10.6 KB

Oops! I missed some renames. We can consider that a test of the unit tests. :-) Here's a better patch!

  • EclipseGc committed 137ede6 on 8.x-3.x authored by dsnopek 
    Issue #2511560 by dsnopek, brantwynn: Add Drupal\page_manager\Plugin\...
eclipsegc’s picture

eclipsegc commented
Status: Needs review » Fixed

Committed 137ede6 and pushed to 8.x-3.x. Thanks!

Status: Fixed » Closed (fixed)

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