There's a bunch of documentation that needs to happen. The discovery interface doesn't even have any. :(

This is a meta issue to track people trying to help document things.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

neclimdul’s picture

Assigned: Unassigned » neclimdul
cosmicdreams’s picture

Can I help in this effort even though I don't have intimate knowledge of the code? Are there any simple tasks that need completed?

effulgentsia’s picture

Priority: Normal » Critical

Upping to critical, because there's no way the system can be committed to core without decent documentation.

@cosmicdreams: thanks for the offer. I don't know yet, but will post ideas on how to proceed once I have them. Maybe neclimdul or someone else with intimate knowledge will post their ideas here as well.

effulgentsia’s picture

Some of the work here might benefit from us first refining some terminology in #1560138: Refine terminology ?. Adding the link here so people can go "follow" that issue.

neclimdul’s picture

It might. Its mostly what to call the meta data that defines a plugin implementation and what to call the string that identifies it.

The docs have gotten a lot better since I posted this but we might take a quick pass and see how much of any code is missing docblocks or param types which I know I am sometimes lazy about. It'd be great if we can do a good job on this but there are people in the core queue that are very good at reviewing these sorts fo things so we can lean on that some too.

neclimdul’s picture

Issue tags: +Novice

the docs review we needed to kick this off. http://drupal.org/node/1497366#comment-6078154

Novice because I think a lot of those can probably be knocked out without a deep understanding of all the crazy things plugins are doing. Feel free to ping me on irc if you have concept questions ;)

Niklas Fiekas’s picture

Moving things over here, as sun suggested:

I've been going backwards through the patch, superficially looking for coding style issues, to get a first idea of what this does. Thanks all! This looks pretty good.

Except some docblocks that are already marked with todos I've noticed these nitpicky things:

+++ b/core/lib/Drupal/Core/Cache/Cache.phpundefined
@@ -0,0 +1,25 @@
+/**
+ *
+ */

:)

+++ b/core/lib/Drupal/Core/Plugin/Discovery/HookDiscovery.phpundefined
@@ -0,0 +1,54 @@
+   * @todo

Should have a short description instead.

+++ b/core/modules/image/lib/Drupal/image/Effect.phpundefined
@@ -0,0 +1,31 @@
+ * Definition of Drupal\image\Effect.

This needs to have an @file, and should be seperated from the namespace statement with a newline.

+++ b/core/modules/image/lib/Drupal/image/Effect.phpundefined
@@ -0,0 +1,31 @@
+class Effect extends PluginType {

This class should have a docblock.

+++ b/core/modules/image/lib/Drupal/image/Effect.phpundefined
@@ -0,0 +1,31 @@
+    // Effects include translated strings.

Not sure what this means for us, here.

+++ b/core/modules/image/lib/Drupal/image/Effect.phpundefined
@@ -0,0 +1,31 @@
+  /**
+   *

:)

+++ b/core/modules/system/lib/Drupal/system/Tests/Plugin/PluginTest.phpundefined
@@ -0,0 +1,98 @@
+<?php

This file needs a doc block ala "Defintion of x."

+++ b/core/modules/system/lib/Drupal/system/Tests/Plugin/PluginTest.phpundefined
@@ -0,0 +1,98 @@
+class PluginTest extends PluginTestBase {

This class should have a docblock.

+++ b/core/modules/system/lib/Drupal/system/Tests/Plugin/PluginTest.phpundefined
@@ -0,0 +1,98 @@
+  function testPluginDefintionFetching() {

This method should have a short docblock.

+++ b/core/modules/system/lib/Drupal/system/Tests/Plugin/PluginTest.phpundefined
@@ -0,0 +1,98 @@
+  function testPluginInstanceFetching() {

This method should have a short docblock.

+++ b/core/modules/system/lib/Drupal/system/Tests/Plugin/PluginTest.phpundefined
@@ -0,0 +1,98 @@
+  function testPluginDerivativeDefintionFetching() {

This method should have a short docblock.

+++ b/core/modules/system/lib/Drupal/system/Tests/Plugin/PluginTest.phpundefined
@@ -0,0 +1,98 @@
+  function testPluginInstanceDerivativeFetching() {

This method should have a short docblock.

+++ b/core/modules/system/lib/Drupal/system/Tests/Plugin/PluginTest.phpundefined
@@ -0,0 +1,98 @@
+  function testPluginEmptyDerivativeInstanceFetching() {

This method should have a docblock.

+++ b/core/modules/system/lib/Drupal/system/Tests/Plugin/PluginTest.phpundefined
@@ -0,0 +1,98 @@
+  function testPluginGetDefinitions() {

This method should have a short description doc block, saying "Tests xyz.".

+++ b/core/modules/system/lib/Drupal/system/Tests/Plugin/PluginTestBase.phpundefined
@@ -0,0 +1,25 @@
+<?php

This file needs a docblock, like "Defintion of ...".

+++ b/core/modules/system/lib/Drupal/system/Tests/Plugin/PluginTestBase.phpundefined
@@ -0,0 +1,25 @@
+abstract class PluginTestBase extends UnitTestBase {

This class needs a docblock.

+++ b/core/modules/system/lib/Drupal/system/Tests/Plugin/PluginTestBase.phpundefined
@@ -0,0 +1,25 @@
+    // We're a unit test using classes... don't let the db registry blow up.

Is this still nescessary? If yes, maybe add @todo, so that we find this later.

+++ b/core/modules/system/tests/modules/plugin_test/lib/Drupal/plugin_test/Plugin/Derivative/DerivativeTest.phpundefined
@@ -0,0 +1,38 @@
+<?php

This file needs a docblock.

+++ b/core/modules/system/tests/modules/plugin_test/lib/Drupal/plugin_test/Plugin/Derivative/DerivativeTest.phpundefined
@@ -0,0 +1,38 @@
+class DerivativeTest implements DerivativeInterface {

This class should have a docblock. Btw. with the name DeriavativeTest this looks like a test case on the first glance.

+++ b/core/modules/system/tests/modules/plugin_test/lib/Drupal/plugin_test/Plugin/Derivative/DerivativeTest.phpundefined
@@ -0,0 +1,38 @@
+

Superflous empty line.

+++ b/core/modules/system/tests/modules/plugin_test/lib/Drupal/plugin_test/Plugin/TestPluginInstanceClass.phpundefined
@@ -0,0 +1,21 @@
+namespace Drupal\plugin_test\Plugin;
+use Drupal\Component\Plugin\PluginAbstract;

Put a newline between these?

+++ b/core/modules/system/tests/modules/plugin_test/lib/Drupal/plugin_test/Plugin/TestPluginInstanceClass.phpundefined
@@ -0,0 +1,21 @@
+class TestPluginInstanceClass extends PluginAbstract {

This class should have a docblock.

+++ b/core/modules/system/tests/modules/plugin_test/lib/Drupal/plugin_test/Plugin/Type/TestPluginType.phpundefined
@@ -0,0 +1,37 @@
+namespace Drupal\plugin_test\Plugin\Type;
+use Drupal\Component\Plugin\Discovery\DerivativeDiscoveryDecorator;
+
+use Drupal\Component\Plugin\PluginType;

We usually have a newline between namespace and use, but not between use statements.

+++ b/core/modules/system/tests/modules/plugin_test/lib/Drupal/plugin_test/Plugin/Type/TestPluginType.phpundefined
@@ -0,0 +1,37 @@
+class TestPluginType extends PluginType {

This class should have a docblock.

neclimdul’s picture

Committed and pushed some doc cleanups from eclipse #1497366-79: Introduce Plugin System to Core and some of my own. A number(but probably not all) of these where addressed.

neclimdul’s picture

FileSize
8.8 KB

relevant, the interdiff of eclipse's patch.

effulgentsia’s picture

Yep, what's left to do from #7 is improving PHPDoc for the classes: Drupal\Core\Cache\Cache, Drupal\image\Effect, Drupal\system\Tests\Plugin\*, and Drupal\plugin_test\*.

effulgentsia’s picture

Status: Active » Needs work
FileSize
0 bytes

This includes simple docs for Drupal\Core\Cache\Cache and Drupal\image\Effect. Leaving at "needs work" for test classes. Any volunteers?

effulgentsia’s picture

Ahem. Let's try again.

neclimdul’s picture

Looks good. Keep em coming! :)

effulgentsia’s picture

Status: Needs work » Fixed

#12 was committed. I think test docs are complete in #1638050: Provide real world values for derivative test. So, marking this issue fixed.

neclimdul’s picture

Status: Fixed » Needs work

I see a number of @todo's for docs in the DefaultFactory and DefaultMapper after the patch so going to leave this open still.

effulgentsia’s picture

Status: Needs work » Fixed

No more todos in plugins-next!!

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