Reconsider naming conventions for derivative classes

This wreaks havoc on my vim ctags. I'm +1 to adding 'Derivative' as a suffix

We do have an issue somewhere about reviewing all class names and making sure they follow the standards. There are a ton with problems... this should probably be put as a sub-issue or duplicate of that other issue... Let's see, where is it?
#1809930: [meta] Many core class names violate naming standards

There were already several classes suffixed with "Deriver", which is more correct, since this class does the deriving, it itself is not the derivative.

I forgot there were string references to those files.

Um... What do these "Deriver" classes actually do? The name "Deriver" to me says that the function of the class is to derive something. Is that the case? I really don't know what these classes are. What are they derivatives of or what are they deriving?

Anyway, lines like this just seem very odd to me:

+class AggregatorCategoryBlockDeriver implements DerivativeInterface {

The interface is called "Derivative" and the class is called "Deriver"... Hm...

I intend to open up an issue to rename DerivativeInterface to DeriverInterface.
An example of a Deriver is MenuBlock (now MenuBlockDeriver). It loops through all of the menus on a site, and provides a derivative block plugin, one per menu.
The idea is that you don't need to write a whole new plugin for each menu you add, you have one deriver that has its own logic for determining its derivatives.

OK, then the name Deriver makes sense to me. Can't we do the interface in this same patch?

Sure!

The only slightly weird thing left that I see is that the namespaces still have "Derivative" in them:

+ * Definition of Drupal\Component\Plugin\Derivative\DeriverInterface.
...
+ * Contains \Drupal\aggregator\Plugin\Derivative\AggregatorCategoryBlockDeriver.

The method names also confused me, but when I read the documentation I think I agreed that they should still have Derivative in them, not Deriver. But I will add that the docs are using "id" all over, and I don't think they are talking about psychology (look it up: "id" is like ego, superego, id, and "ID" means identifier, one of my pet documentation peeves, but anyway that is a separate issue.)

This is not really a docs issue.

+++ b/core/lib/Drupal/Component/Plugin/Derivative/DeriverInterface.phpundefined
@@ -2,7 +2,7 @@
- * Definition of Drupal\Component\Plugin\Derivative\DerivativeInterface.
+ * Definition of Drupal\Component\Plugin\Derivative\DeriverInterface.

I had the reverse reaction of jhodgdon. The namespace is Derivative and the interface is Deriver?

Well the classes do the deriving. The derivatives themselves are whatever those classes find, like the values in the database that represent a custom block.
So both the namespace and the interface should be changed...

Wait a minute. The main purpose of a particular Block derivative class is to define/provide a block, correct? So it seems like the class name shouldn't have derivative or deriver or anything else like that in it. It should just be called the FooBarReallyCoolBlock class or something like that? I mean, it's possible that the way that the class manages to provide a block for use in a site is by being a derivative of another class, but class names should tell the purpose of the class (why it exists), not some detail of the internal mechanisms by which it gets its work done, right?

Ah, OK. For the situation in #17, it seems like there should be:
MenuBlock (defines the plugin type for blocks displaying menus)
MenuBlockGenerator (generates a menu block definition for each defined menu)

Generator sounds like a reasonable name to me, but maybe there is a standard computer science term for this pattern that we should use instead?

I don't know of a standard CS term, but would that get confused with the UrlGenerator, which is colloquially just called "the generator"?

(Once again, English has insufficient nouns to keep up with Drupal.)

I am not sure why it would get confused.

Updated issue summary.

Re-roll, and using deriver instead of the current inconsistent derivative and derivative fetcher.

Not all of the files got renamed the same as the classes in that patch. For instance, the first one at the top.

Re-roll.

26: drupal_1875996_26.patch queued for re-testing.

26: drupal_1875996_26.patch queued for re-testing.

26: drupal_1875996_26.patch queued for re-testing.

Re-roll, and I renamed getBasePluginId() to getBaseId() and getDerivativePluginId() to getDerivativeId() on \Drupal\Core\Plugin\Discovery\ContainerDeriverInterface per @EclipseGc's request.

33: drupal_1875996_33.patch queued for re-testing.

40: drupal_1875996_40.patch queued for re-testing.

Hmmz. I can't reproduce any of these errors locally and the code looks fine too.

Meh, wrong .gitignore configuration locally.

46: drupal_1875996_46.patch queued for re-testing.

Re-testing, as I cannot reproduce the failures locally.

Tests pass now.

Can we update the summary? 73k is a lot to review without direction.

I've run out of time today to look at the code but I agree with the documented API changes. +1

46: drupal_1875996_46.patch queued for re-testing.

Re-roll because of changes in \Drupal\Tests\Core\Plugin\Discovery\DerivativeDiscoveryDecoratorTest.

62: drupal_1875996_62.patch queued for re-testing.

Updating with reroll. Please review.

Which parts did not apply?

@xano,

For below.

-----------------------------
diff --git a/core/modules/system/lib/Drupal/system/Plugin/Block/SystemMenuBlock.php b/core/modules/system/lib/Drupal/system/Plugin/Block/SystemMenuBlock.php
index 15cb1d8..983d84f 100644
--- a/core/modules/system/lib/Drupal/system/Plugin/Block/SystemMenuBlock.php
+++ b/core/modules/system/lib/Drupal/system/Plugin/Block/SystemMenuBlock.php
@@ -17,7 +17,7 @@
* id = "system_menu_block",
* admin_label = @Translation("Menu"),
* category = @Translation("Menus"),
- * derivative = "Drupal\system\Plugin\Derivative\SystemMenuBlock"
+ * deriver_class = "Drupal\system\Plugin\Derivative\SystemMenuBlock"
* )
*/
class SystemMenuBlock extends BlockBase {
diff --git a/core/modules/system/lib/Drupal/system/Plugin/Derivative/SystemMenuBlock.php b/core/modules/system/lib/Drupal/system/Plugin/Derivative/SystemMenuBlock.php
index 72a4c49..39cef36 100644
--- a/core/modules/system/lib/Drupal/system/Plugin/Derivative/SystemMenuBlock.php
+++ b/core/modules/system/lib/Drupal/system/Plugin/Derivative/SystemMenuBlock.php
@@ -7,9 +7,9 @@

namespace Drupal\system\Plugin\Derivative;

-use Drupal\Component\Plugin\Derivative\DerivativeBase;
+use Drupal\Component\Plugin\Deriver\DeriverBase;
use Drupal\Core\Entity\EntityStorageControllerInterface;
-use Drupal\Core\Plugin\Discovery\ContainerDerivativeInterface;
+use Drupal\Core\Plugin\Discovery\ContainerDeriverInterface;
use Symfony\Component\DependencyInjection\ContainerInterface;

/**
diff --git a/core/modules/views/lib/Drupal/views/Plugin/Derivative/ViewsBlock.php b/core/modules/views/lib/Drupal/views/Plugin/Derivative/ViewsBlock.php
index 9541124..05c7dc4 100644
--- a/core/modules/views/lib/Drupal/views/Plugin/Derivative/ViewsBlock.php
+++ b/core/modules/views/lib/Drupal/views/Plugin/Derivative/ViewsBlock.php
@@ -8,7 +8,7 @@
namespace Drupal\views\Plugin\Derivative;

use Drupal\Core\Entity\EntityStorageControllerInterface;
-use Drupal\Core\Plugin\Discovery\ContainerDerivativeInterface;
+use Drupal\Core\Plugin\Discovery\ContainerDeriverInterface;
use Symfony\Component\DependencyInjection\ContainerInterface;

/**
diff --git a/core/modules/views/lib/Drupal/views/Plugin/Derivative/ViewsExposedFilterBlock.php b/core/modules/views/lib/Drupal/views/Plugin/Derivative/ViewsExposedFilterBlock.php
index 29439a6..3e08fd0 100644
--- a/core/modules/views/lib/Drupal/views/Plugin/Derivative/ViewsExposedFilterBlock.php
+++ b/core/modules/views/lib/Drupal/views/Plugin/Derivative/ViewsExposedFilterBlock.php
@@ -7,7 +7,7 @@

namespace Drupal\views\Plugin\Derivative;

-use Drupal\Core\Plugin\Discovery\ContainerDerivativeInterface;
+use Drupal\Core\Plugin\Discovery\ContainerDeriverInterface;
use Drupal\Core\Entity\EntityStorageControllerInterface;
use Symfony\Component\DependencyInjection\ContainerInterface;
--------------------------------------

Thanks! Next time you can also use the <code> tag when you paste code, or use http://pastebin.com if the code is more than a few lines.

+++ b/core/lib/Drupal/Component/Plugin/DerivativeInspectionInterface.php
@@ -18,7 +18,7 @@
-  public function getBasePluginId();
+  public function getBaseId();

How come this is a part of this issue? Doesn't seem like a requirement for the derivative > deriver change?

Someone asked me to make that consistent in this issue, as we have getDerivativeId() as well. That person may have been EclipseCG, but I am not sure.

Comment trail points to Kris #1875996-33: Reconsider naming conventions for derivative classes

drupal8-reconsider_naming_conventions_for_derivative_classes-1875996-65.patch no longer applies.

error: patch failed: core/modules/views/lib/Drupal/views/Plugin/Derivative/ViewsLocalTask.php:8
error: core/modules/views/lib/Drupal/views/Plugin/Derivative/ViewsLocalTask.php: patch does not apply

Updating patch with reroll. Please review.

73: drupal8-reconsider_naming_conventions_for_derivative_classes-1875996-73.patch queued for re-testing.

There might be a bot issue. Back to RTBC, as per #74.

The change records that will need to be changed are https://drupal.org/node/2165243 and https://drupal.org/node/2044515.

73: drupal8-reconsider_naming_conventions_for_derivative_classes-1875996-73.patch queued for re-testing.

I also added a change record draft.

Re-roll.

83: drupal_1875996_83.patch queued for re-testing.

83: drupal_1875996_83.patch queued for re-testing.

Re-roll.

Marking this as a DX improvement, because we currently do not properly distinguish between derivative plugins and derivers in terms of jargon.

Re-roll.

91: drupal_1875996_91.patch queued for re-testing.

So I've actively been trying to avoid driving this issue and let the community figure out how to name this. It feels like we're getting to a consensus but before we move to RTBC I want to go ahead and get this out.

+++ b/core/lib/Drupal/Component/Plugin/Deriver/DeriverBase.php
@@ -2,20 +2,15 @@
- * Contains Drupal\Component\Plugin\Derivative\DerivativeBase.
+ * Contains \Drupal\Component\Plugin\Deriver\DeriverBase.

I'm don't think this namespace change is useful. When listening to people talk about plugins outside this discussion, I've heard things like "use Derivatives" and "those are implemented by Derivatives." They aren't talking about the class, they're talking about the concept. Derivers are the tool and that does seem to make the language better, but Derivative is the concept. I don't think we'll probably have any other classes in the namespace, but I do think it would be better to leave the namespace the conceptual term even if it doesn't match the class.

That makes sense.

+1 for #93

Let's also get rid of the _class suffix of deriver_class, as we don't do that anywhere else. For all we know we'll support derivers to be services in the future.

Re-roll. Will fix the naming issues in the following patch.

Fixed the test failure, the namespace, and the annotation property name.

102: drupal_1875996_102.patch queued for re-testing.

Looks good to me. I'd say the change record updates can happen after commit? I could be wrong but going to be bold and RTBC it.

In #2287097: Use block template to render blocks, we're having problems because BlockPluginInterface does not implement DerivativeInspectionInterface.
However, we call it indiscriminately, and BlockBase implements all of the methods.

So instead of cheating in all of those docs, I'l like to see this fixed in place.

Assigning to @neclimdul for final sign off.

There are a ton of @var comments in here. I'm not sure we need all them but we don't have guidelines so what ever.

1. +++ b/core/lib/Drupal/Component/Plugin/Discovery/DerivativeDiscoveryDecorator.php
   @@ -19,14 +19,25 @@ class DerivativeDiscoveryDecorator implements DiscoveryInterface {
   +   * Creates a new class instance.
   

   "Creates a new instance" should be sufficient. It confuses me if only because its creating a new object, not a class. #nitpickofthemonth

2. +++ b/core/lib/Drupal/Core/Plugin/Discovery/ContainerDerivativeDiscoveryDecorator.php
   @@ -14,22 +14,22 @@ class ContainerDerivativeDiscoveryDecorator extends DerivativeDiscoveryDecorator
   +        if (is_subclass_of($class, '\Drupal\Core\Plugin\Discovery\ContainerDeriverInterface')) {
   +          /** @var \Drupal\Core\Plugin\Discovery\ContainerDeriverInterface $class */
   +          $this->derivers[$base_plugin_id] = $class::create(\Drupal::getContainer(), $base_plugin_id);
            }
   

   I'm going to make an exception for this @var which seems a bit useless. I'm aware phpstorm doesn't parse that which is unfortunate but its one method call and its really obvious what's happening so I don't see the point. It doesn't actually document anything useful.

3. +++ b/core/modules/search/src/Plugin/Derivative/SearchLocalTask.php
   @@ -7,15 +7,15 @@
   +class SearchLocalTask extends \Drupal\Component\Plugin\Derivative\DeriverBase implements ContainerDeriverInterface {
   

   Fully qualified?

4. +++ b/core/modules/system/src/Plugin/Derivative/SystemMenuBlock.php
   @@ -17,7 +17,7 @@
   +class SystemMenuBlock extends \Drupal\Component\Plugin\Derivative\DeriverBase implements ContainerDeriverInterface {
   

   Fully qualified?

5. +++ b/core/modules/system/tests/modules/menu_test/src/Plugin/Derivative/LocalTaskTest.php
   @@ -6,9 +6,9 @@
   +class LocalTaskTest extends \Drupal\Component\Plugin\Derivative\DeriverBase {
   

   Again...

I'm going to make an exception for this @var which seems a bit useless. I'm aware phpstorm doesn't parse that which is unfortunate but its one method call and its really obvious what's happening so I don't see the point. It doesn't actually document anything useful.

It does, as it lets PhpStorm (and perhaps other IDEs) know the variable type. Without the comment, PhpStorm wouldn't know, and it would mark subsequent method calls as invalid.

Again...

I blame the system...

Here is the same patch, but with renames, so it should be slightly smaller and easier to read.

It does, as it lets PhpStorm (and perhaps other IDEs) know the variable type. Without the comment, PhpStorm wouldn't know, and it would mark subsequent method calls as invalid.

So, like I said, there are no guidelines but I still want to be clear. My view is your IDE is there to help the developer read code and we don't need help reading is_subclass_of.

I guess I see this as doing something like this which would never fly.

if (is_bool($foobar)) {
  // This is bool!
  return $foobar;
}

I'm not going to stop this issue on such a unclear doc issue though, RTBC.

This is indeed not part of our docs. However, PhpStorm doesn't use is_subclass_of() for type hints, so I tend to add @var comments to help me see if the code is correct in the blink of an eye. I've found doing this helps me find problems much more quickly.

straight reroll

a patch with some content...

Looks good to me!

Good first step so committed to 8.x. Thanks everyone! This patch didn't rename Drupal\block_content\Plugin\Derivative\BlockContent. Per the original issue summary, do we still want to do that, since there's also a Drupal\block_content\Entity\BlockContent? Would generically adding a Deriver suffix to all deriver classes make sense?

Follow-up: #2295571: Bogus CustomBlock class in repo

(This is not the follow-up so not removing the "needs follow-up" tag.)