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

Currently, in PanelsDisplayVariant, we've copied a bunch of code from Page Manager's BlockDisplayVariant in a futile attempt to avoid depending on Page Manager. (Unfortunately, we still ended up depending on it - here is another issue to fix that: #2511576: Remove dependency on Page Manager)

However, ideally, we just extend BlockDisplayVariant rather than having to copy code from it every time there is a new Page Manager release. :-)

And we can do that after it's moved into CTools per #2511566: Add an abstract version of Drupal\page_manager\Plugin\DisplayVariant\BlockDisplayVariant from Page Manager

CommentFileSizeAuthor
#17 interdiff.txt960 bytestim.plunkett
#17 panels-no-pm-2511582-17.patch10.56 KBtim.plunkett
#15 interdiff.txt2.53 KBtim.plunkett
#15 panels-no-pm-2511582-15.patch10.68 KBtim.plunkett
#11 panels-no-pm-2511582-11.patch8.96 KBtim.plunkett
#11 interdiff.txt2.04 KBtim.plunkett
#8 panels-no-pm-2511582-8.patch8.2 KBdsnopek
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
CreditAttribution: dsnopek commented
Status: Active » Postponed

Postponed on #2511566: Add an abstract version of Drupal\page_manager\Plugin\DisplayVariant\BlockDisplayVariant from Page Manager

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
CreditAttribution: dsnopek commented
Issue summary: View changes
rlmumford’s picture

rlmumford
Primary language English
Location Manchester
CreditAttribution: rlmumford as a volunteer commented

Adding #2561963: Display Config Entity as a related issue. I'm guessing that when that gets in we won't want to extend BlockDisplayVariant any more.

rlmumford’s picture

rlmumford
Primary language English
Location Manchester
CreditAttribution: rlmumford as a volunteer commented
Related issues: +#2561963: Display Config Entity
tim.plunkett’s picture

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

#2571957: PageAwareVariantInterface is gone is not worth fixing in isolation. Panels is broken until it is, and we need to fix it here.

tim.plunkett’s picture

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

This can be done now, AFAIK.

saltednut’s picture

saltednut
she/her
Primary language English
Location Chicago
CreditAttribution: saltednut at Acquia commented
Issue summary: View changes

Updated issues summary to mention PanelsDisplayVariant so this is easier to find.

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
CreditAttribution: dsnopek commented
Status: Active » Needs review
FileSize
panels-no-pm-2511582-8.patch8.2 KB

Here's a patch that implements this! And since we've already gone this far, it also pulls out the page_manager dependency per #2511576: Remove dependency on Page Manager which we can just close if this patch is committed :-)

Note: there is still some c/p code from page_manager, but it's mostly stuff that will need to end up in the Builder plugin (ie like old "renderer"), so I think it's fine for now.

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
CreditAttribution: dsnopek commented
Status: Needs work » Needs review

Heh, I think the test error is because there are no tests. :-) Setting back to NR.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented
FileSize
interdiff.txt2.04 KB
panels-no-pm-2511582-11.patch8.96 KB

I think this is probably RTBC. buildRegions() is still copy/paste from old page_manager code, but that didn't go into CTools. It's still in p_m, but with newer cacheability changes. That can be a follow-up though.

dsnopek’s picture

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

Thanks! The changes in #11 look good to me, so taking the leap to RTBC :-)

buildRegions() is still copy/paste from old page_manager code, but that didn't go into CTools. It's still in p_m, but with newer cacheability changes. That can be a follow-up though.

Yeah, I think we should address that either as part of #2340999: Implement "display renderer" plugin type (now called "display builder") or immediately after it, since the building should really occur in a Builder plugin anyway.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented
Status: Needs work » Needs review
FileSize
panels-no-pm-2511582-15.patch10.68 KB
interdiff.txt2.53 KB

Now with a basic test to prevent DrupalCI from crashing, which also revealed a mistake.

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
CreditAttribution: dsnopek commented 
+++ b/src/Plugin/DisplayVariant/PanelsDisplayVariant.php
@@ -38,11 +24,7 @@
+class PanelsDisplayVariant extends BlockDisplayVariant implements ConditionVariantInterface, ContainerFactoryPluginInterface {

We should remove ConditionVariantInterface too, it's also on BlockDisplayVariant.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented
FileSize
panels-no-pm-2511582-17.patch10.56 KB
interdiff.txt960 bytes

And ContainerFactoryPluginInterface too.

dsnopek’s picture

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

Just manually tested #15 and #17 - they work for me!

g089h515r806’s picture

g089h515r806 CreditAttribution: g089h515r806 commented

#17 - work for me!
Fix issue: Fatal error: Interface 'Drupal\page_manager\Plugin\ContextAwareVariantInterface' not found in(https://www.drupal.org/node/2584699)

  • japerry committed 4f2d154 on 8.x-3.x authored by tim.plunkett 
    Issue #2511582 by tim.plunkett, dsnopek: Extend BlockDisplayVariant...
japerry’s picture

japerry
Location KVUO
CreditAttribution: japerry as a volunteer commented
Status: Reviewed & tested by the community » Fixed

Thanks! Fixed,

Now time to get the ctools and page manager releases out and we should have it stable again on RC1.

Status: Fixed » Closed (fixed)

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