In #2636478: IPE shouldn't depend on Page Manager, we decouple the Panels IPE from Page Manager, so that it can work with other modules that integrate with Panels (ie. Panelizer).

However, once that's merged, then Page Manager will need to have explicit integration with the IPE! This issue is to add that. :-)

CommentFileSizeAuthor
#13 interdiff.txt10.54 KBdsnopek
#13 page_manager-ipe-2642452-13.patch13 KBdsnopek
#12 page_manager-ipe-2642452-plugins.txt12.78 KBdsnopek
#11 interdiff.txt1.17 KBtim.plunkett
#11 2642452-page_manager-ipe-11.patch12.29 KBtim.plunkett
#10 interdiff.txt12.12 KBtim.plunkett
#10 2642452-page_manager-ipe-10.patch12.4 KBtim.plunkett
#8 interdiff.txt7.61 KBdsnopek
#8 page_manager-ipe-2642452-8.patch13.16 KBdsnopek
#7 interdiff.txt550 bytesdsnopek
#7 page_manager-ipe-2642452-7.patch5.99 KBdsnopek
#6 interdiff.txt1.79 KBdsnopek
#6 page_manager-ipe-2642452-6.patch6.09 KBdsnopek
#4 interdiff.txt1.08 KBdsnopek
#4 page_manager-ipe-2642452-4.patch5.93 KBdsnopek
#3 interdiff.txt4.37 KBdsnopek
#3 page_manager-ipe-2642452-3.patch5.9 KBdsnopek
#2 page_manager-ipe-2642452-2.patch4.65 KBdsnopek

Comments

dsnopek created an issue. See original summary.

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
commented
Status: Active » Needs work
StatusFileSize
new page_manager-ipe-2642452-2.patch4.65 KB

Here's the first pass at this! This needs some work - basically, just cleaning things up and properly injecting the right entity manager to load page variants (rather than using the static method).

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
commented
Status: Needs work » Needs review
StatusFileSize
new page_manager-ipe-2642452-3.patch5.9 KB
new interdiff.txt4.37 KB

Alright! I did the clean-up I wanted to do and tried to do the appropriate thing for all the error conditions. This depends on #2636478: IPE shouldn't depend on Page Manager getting committed first, but I think it's at least ready for initial reviews!

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
commented
StatusFileSize
new page_manager-ipe-2642452-4.patch5.93 KB
new interdiff.txt1.08 KB

Here's a version that makes the Panels storage service private, and specifies an explicit name rather than using the service id.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented 
  1. +++ b/src/Form/PageVariantFormBase.php
@@ -156,13 +157,26 @@ abstract class PageVariantFormBase extends EntityForm {
+    // Make sure the Panels storage is set correctly before saving!

    Please switch from ! to . 

  2. +++ b/src/Form/PageVariantFormBase.php
@@ -156,13 +157,26 @@ abstract class PageVariantFormBase extends EntityForm {
     $form_state->setValue('variant_settings', $variant_plugin_values->getValues());
-
     parent::submitForm($form, $form_state);

    Can you put back that newline? :) 

  3. +++ b/src/Form/PageVariantFormBase.php
@@ -156,13 +157,26 @@ abstract class PageVariantFormBase extends EntityForm {
+    if (is_a($variant_plugin, '\Drupal\panels\Plugin\DisplayVariant\PanelsDisplayVariant', TRUE)) {

    I really wanted to say "this should use instanceof" and "this should use the ::class constant" but both of those would break due to loading a possibly-non-existent class. Let's put a comment here explaining that, so no one accidentally "fixes" it later. 

  4. +++ b/src/PanelsStorage.php
@@ -0,0 +1,99 @@
+   * @return \Drupal\page_manager\PageVariantInterface
...
+    if ($page_variant = $this->loadPageVariant($id)) {
+      /** @var \Drupal\panels\Plugin\DisplayVariant\PanelsDisplayVariant $panels_display */

    Can't we just document this here? 

  5. +++ b/src/PanelsStorage.php
@@ -0,0 +1,99 @@
+}
\ No newline at end of file

    Missing newline

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
commented
StatusFileSize
new page_manager-ipe-2642452-6.patch6.09 KB
new interdiff.txt1.79 KB

@tim.plunkett: Thanks!

I addressed all points except for #5.4, which I don't entirely understand..

It sounds like you're saying that we should type hint on the loadPageVariant()'s return value, however, that type hint is for the value returned by PageVariantInterface::getVariantPlugin() which normally returns VariantInterface, however, we know that it's going to be a PanelsDisplayVariant because we do the instanceof check in a couple lines.

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
commented
StatusFileSize
new page_manager-ipe-2642452-7.patch5.99 KB
new interdiff.txt550 bytes

Chatted with @tim.plunkett on IRC and got to the bottom of what he meant! Removed the unnecessary @var statement.

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
commented
StatusFileSize
new page_manager-ipe-2642452-8.patch13.16 KB
new interdiff.txt7.61 KB

Here's some tests for this! They should fail because they need Panels. Hopefully, #2647014: Add Panels to test_dependencies so we can test PanelStorage will be able to fix this.

Status: Needs review » Needs work

The last submitted patch, 8: page_manager-ipe-2642452-8.patch, failed testing.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Needs work » Needs review
StatusFileSize
new 2642452-page_manager-ipe-10.patch12.4 KB
new interdiff.txt12.12 KB

I wish I'd been able to review \Drupal\panels\Storage\PanelsStorageInterface before it went in, we should never use the bare \Exception class anywhere :(

Reworked the tests to use Prophecy.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
StatusFileSize
new 2642452-page_manager-ipe-11.patch12.29 KB
new interdiff.txt1.17 KB

As per http://php.net/manual/en/internals2.opcodes.instanceof.php#108164 and my own testing https://3v4l.org/1YoDo, I'm nixing the is_a call.

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
commented
StatusFileSize
new page_manager-ipe-2642452-plugins.txt12.78 KB

Here's how this patch would look if we made PanelStorage into plugins per #2648620: PanelsStorage should be plugins :-/. This feels like it might be worse DX for the implementor of the different PanelsStorage's. :-/

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
commented
StatusFileSize
new page_manager-ipe-2642452-13.patch13 KB
new interdiff.txt10.54 KB

Here's a for real patch which is based on #2648620: PanelsStorage should be plugins :-/ from Panels. The interdiff is from #11 to this patch (we'll just pretend my half patch on #12 didn't happen ;-)).

This will probably fail tests because it depends on the Panels changes in #2648620. But the tests are passing for me locally!

Status: Needs review » Needs work

The last submitted patch, 13: page_manager-ipe-2642452-13.patch, failed testing.

The last submitted patch, 13: page_manager-ipe-2642452-13.patch, failed testing.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Needs work » Fixed

Committed

  • tim.plunkett committed 4911574 on 8.x-1.x authored by dsnopek 
    Issue #2642452 by dsnopek, tim.plunkett: Integrate Page Manager with the...

Status: Fixed » Closed (fixed)

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

damienmckenna’s picture

damienmckenna
Location TN, USA
commented
Parent issue: » #2787419: Plan for Page Manager 8.x-4.0 release