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

This is about moving the following non-controversial parts of Page Manager into CTools:

  • Drupal\page_manager\Plugin\BlockVariantInterface (and Trait)
  • Drupal\page_manager\Plugin\ConditionVariantInterface (and Trait)
  • Drupal\page_manager\Plugin\BlockPluginCollection

TODO

  • Create the new files in CTools, renaming all the namespaces
  • Copy and port any automated tests for these files into CTools as well
  • Make sure the tests are all passing

This issue DOESN'T include changing Page Manager to use this code in CTools (rather than it's versions), that's going to be in #2511574: Refactor after non-controversial code (issue #2511556) is moved to CTools, although, it would be good to have a working patch there before committing this one.

CommentFileSizeAuthor
#17 interdiff.txt1.45 KBdsnopek
#17 ctools-move-pm-classes-2511556-17.patch14.15 KBdsnopek
#16 2511556.patch14.02 KBrlmumford
#14 ctools-move-pm-classes-2511556-14.patch19.14 KBdsnopek
#11 ctools-move-pm-classes-2511556-11.patch25.18 KBdsnopek
#9 interdiff-2511556-5-9.txt48.45 KBmatthewmessmer
#9 ctools-move-pm-classes-2511556-9.patch25.58 KBmatthewmessmer
#5 ctools-move-pm-classes-2511556-5.patch25.21 KBMichelle
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
Issue tags: +D8panels

I'm adding to the sprint board because a Panels issue is blocked on this one: #2511576: Remove dependency on Page Manager

dsnopek’s picture

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

Er, VariantAwareInterface wasn't meant to be here, that got it's own issue: #2511560: Add Drupal\page_manager\Plugin\VariantAwareInterface and VariantCollection from Page Manager

dsnopek’s picture

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

Michelle
she/her
Primary language English
Location Wisconsin, USA
CreditAttribution: Michelle as a volunteer commented
Assigned: Unassigned » Michelle

Just noting that I am working on this at Drupalcorn.

Michelle’s picture

Michelle
she/her
Primary language English
Location Wisconsin, USA
CreditAttribution: Michelle as a volunteer commented
Assigned: Michelle » Unassigned
Status: Active » Needs review
FileSize
ctools-move-pm-classes-2511556-5.patch25.21 KB

I took a crack at this but I know nothing about testing and am hung up there. I moved the two tests that seemed likely but could not figure out how to run the tests. If I do --file [testfile] it comes back with "ERROR: No valid tests were specified.".

Unassigning this from myself because I'm heading home from Drupalcorn and honestly don't see me being able to get back to this in the next 2 weeks. Hope it helps some. :)

Status: Needs review » Needs work

The last submitted patch, 5: ctools-move-pm-classes-2511556-5.patch, failed testing.

Status: Needs work » Needs review

brantwynn queued 5: ctools-move-pm-classes-2511556-5.patch for re-testing.

matthewmessmer’s picture

matthewmessmer CreditAttribution: matthewmessmer commented
Assigned: Unassigned » matthewmessmer

Working on this at MWDS

matthewmessmer’s picture

matthewmessmer CreditAttribution: matthewmessmer commented
FileSize
ctools-move-pm-classes-2511556-9.patch25.58 KB
interdiff-2511556-5-9.txt48.45 KB

I moved some the tests from /src/Tests/src/Unit to /tests/src/Unit. They should execute and pass now.

dsnopek’s picture

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

Code looks good to me! Marking RTBC :-)

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
CreditAttribution: dsnopek commented
Status: Reviewed & tested by the community » Needs review
FileSize
ctools-move-pm-classes-2511556-11.patch25.18 KB

Er, hang on! This patch isn't totally well formed, it has the path wrong. Here's one that should be right!

dsnopek’s picture

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

If the tests pass, I think this is still RTBC.

EclipseGc’s picture

EclipseGc CreditAttribution: EclipseGc commented
Status: Reviewed & tested by the community » Needs work 
+++ b/src/Plugin/ConditionVariantInterface.php
@@ -0,0 +1,92 @@
+interface ConditionVariantInterface extends VariantInterface {

+++ b/src/Plugin/ConditionVariantTrait.php
@@ -0,0 +1,108 @@
+trait ConditionVariantTrait {

Both of these seem completely unnecessary to me. We're moving Page Manager variants to a config entity and those will always have selection criteria handling at the entity level and don't need this abstraction. Remove these from the patch and the rest looks pretty good to me.

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
CreditAttribution: dsnopek commented
Status: Needs work » Needs review
FileSize
ctools-move-pm-classes-2511556-14.patch19.14 KB

Here's a new version of the patch with the ConditionVariantInterface and ConditionVariantTrait removed!

EclipseGc’s picture

EclipseGc CreditAttribution: EclipseGc commented
Status: Needs review » Needs work 
+++ b/src/Plugin/BlockVariantTrait.php
@@ -0,0 +1,120 @@
+  public function getRegionNames() {
+    return [
+      'top' => 'Top',
+      'bottom' => 'Bottom',
+    ];
+  }

I really don't like that we're implementing this in a trait. I'd much rather see us provide a BlockVariantInterface implementing class that provides these regions instead of having them on the trait.

So, this feels like two issues to me. Can we break apart the EntityView block & deriver from the BlockVariantInterface & Plugin collection and stuff? One of these is totally applicable to core's uses and the others are support for panels. I think it'll make review easier, I made it to the deriver class and was like "wait am I still reviewing the entity view block patch?" and indeed I was.

I think both of these can probably go in, but I have no doubt about the block and the deriver and I have some additional review and effort I want to see on the variant stuff, so lets simplify a bit and get some of this committed now.

Eclipse

rlmumford’s picture

rlmumford
Primary language English
Location Manchester
CreditAttribution: rlmumford as a volunteer commented
Title: Add EntityView block, BlockVariantInterface (and Trait), ConditionVariantInterface (and Trait), and BlockPluginCollection from Page Manager » Add BlockVariantInterface (and Trait), ConditionVariantInterface (and Trait), and BlockPluginCollection from Page Manager
Issue summary: View changes
Status: Needs work » Needs review
Related issues: +#2568741: Add EntityView block from Page Manager
FileSize
2511556.patch14.02 KB

Split the EntityView block into another issue. #2568741: Add EntityView block from Page Manager

Patch is the same but with the Entity View block and deriver removed.

Setting back to needs review so we can begin sorting out the block view variant.

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
CreditAttribution: dsnopek commented
Title: Add BlockVariantInterface (and Trait), ConditionVariantInterface (and Trait), and BlockPluginCollection from Page Manager » Add BlockVariantInterface (and Trait), and BlockPluginCollection from Page Manager
FileSize
ctools-move-pm-classes-2511556-17.patch14.15 KB
interdiff.txt1.45 KB

Here's a patch that addresses #15 by making getRegionNames() abstract. Hopefully, this one will finally be ready!

Status: Needs review » Needs work

The last submitted patch, 17: ctools-move-pm-classes-2511556-17.patch, failed testing.

mpotter’s picture

mpotter CreditAttribution: mpotter commented

Added #2573413: Fix wizard tests to address the failing Wizard tests that don't seem to have anything to do with this patch.

dsnopek’s picture

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

Forcing back to NR - the test failures are unrelated

japerry’s picture

japerry
Location KVUO
CreditAttribution: japerry as a volunteer commented
Status: Needs review » Closed (duplicate)

Marking as a duplicate to the fixed #2511566: Add an abstract version of Drupal\page_manager\Plugin\DisplayVariant\BlockDisplayVariant from Page Manager issue.