So, PanelsStorage is using tagged services specifically because I had thought there was a problem with plugins implementing interfaces that don't exist causing fatal errors even if the module that defines the plugin manager isn't present. I swear that I encountered this problem, but now I can't reproduce it.

On the one hand this is great, because it means Drupal 8 doesn't have this terribly broken problem that I thought it had!

On the other hand, I totally messed up PanelsStorage. :-/

There isn't really anything wrong with using a tagged service, except that a factory pattern (like plugins) would be a more natural fit to what we're trying to do, and it's an architectural decision with no real reason (ie. a reason actually existing in the real world and not my head). :-)

Ideally, we'd fix this sooner rather than later...

CommentFileSizeAuthor
#6 interdiff.txt12.44 KBdsnopek
#6 panels-storage-plugins-2648620-6.patch14.69 KBdsnopek
#4 panels-storage-plugins-2648620-4.patch5.19 KBdsnopek
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dsnopek created an issue. See original summary.

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
CreditAttribution: dsnopek commented

So, the question is:

Do we ...

  1. Try to convert to plugins before anyone notices or uses this API, which has the advantage that no one (other than ourselves) is using this API yet, or
  2. Leave things as they are because a tagged service isn't really the end of the world, so that we can move on and get to the business of implementing the massive pile of other stuff we need to implement?

I'm torn. I'd like to do #1 because I'm super embarrassed to have built the API like this for no reason. But if someone else had written this code, I'd very likely be arguing for #2 so we don't get off-track.

tim.plunkett’s picture

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

I vote for #1. Page Manager hasn't even used this yet, no one else will have.

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
CreditAttribution: dsnopek commented
Status: Active » Needs review
FileSize
panels-storage-plugins-2648620-4.patch5.19 KB

Barring documentation changes, this is the minimum number of changes to make this work. However, looking at how the Page Manager plugin would look... I'm a little bit unsure about the plugin approach, because we're basically ignoring most of the stuff that plugins do.

Here's how the plugin declaration for the Page Manager plugin would look:

https://gist.github.com/dsnopek/611cea4581b2b4d25156

Notice how the annotation has nothing but the 'id' in it, and how we don't descend of PluginBase (or any base), and we ignore all the configuration stuff.

Maybe tagged services wasn't such a bad idea? :-)

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented 
  1. +++ b/src/Annotation/PanelsStorage.php
@@ -0,0 +1,29 @@
+class PanelsStorage extends Plugin {
...
+  public $id;

    This should extend PluginID, and can drop the property 

  2. +++ b/src/Storage/PanelsStorageManager.php
@@ -26,11 +29,23 @@ class PanelsStorageManager implements PanelsStorageManagerInterface {
+    parent::__construct('Plugin/PanelsStorage', $namespaces, $module_handler, 'Drupal\panels\Storage\PanelsStorageInterface', 'Drupal\panels\Annotation\PanelsStorage');

    Remember, you can do PanelsStorageInterface::class and PanelsStorage::class :)

And then you can change the annotations from

+ * @PanelsStorage(
+ *   id = "page_manager"
+ * )

to
+ * @PanelsStorage("page_manager")

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
CreditAttribution: dsnopek commented
FileSize
panels-storage-plugins-2648620-6.patch14.69 KB
interdiff.txt12.44 KB

Sorry, I just can't stop thinking about this issue. :-)

This patch makes the changes recommended by Tim on #5, adds a plugin base class (just in case we want to put something in there later) and updates the documentation.

This should be much closer to being actually committable!

EclipseGc’s picture

EclipseGc CreditAttribution: EclipseGc commented 
  1. +++ b/src/Storage/PanelsStorageManager.php
@@ -26,11 +30,23 @@ class PanelsStorageManager implements PanelsStorageManagerInterface {
+    parent::__construct('Plugin/PanelsStorage', $namespaces, $module_handler, PanelsStorageInterface::class, PanelsStorage::class);

    Pretty

  • EclipseGc committed 48e6e2f on 8.x-3.x authored by dsnopek 
    Issue #2648620 by dsnopek: PanelsStorage should be plugins :-/
EclipseGc’s picture

EclipseGc CreditAttribution: EclipseGc commented
Status: Needs review » Fixed

Ok, I committed this. It's the beginnings of an API we want modules like page_manager to use and since that code's not landed yet, and we want to get this right, I'm paving the way.

Eclipse

Status: Fixed » Closed (fixed)

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