Port AnnotatedClassDiscovery to PSR-4, and make it agnostic of directory information.

Options for Doctrine's AnnotationRegistry:

We could subclass the DocParser. But then we'd also have to subclass some other classes (don't remember exactly) which hard-reference DocParser.

We could also use the autoload override mechanic to have our overridden version of AnnotationRegistry.
I don't really like this option, who knows what side effects it would have.

Or we could write our own annotation parsing code.

Or we could talk with the Doctrine people.

Option for the namespace lists:

Of course we could just port the namespace lists to PSR-4..

But I would rather see those lists encapsulated in some way, so that the details of the autoload standard are not all over the place.
Most of the code should deal with namespaces, not with directories.

I am attempting to solve some of that over in
#2033501: Explore Krautoload as a solution for PSR-4 class loading
but I want to have a separate issue to not allow the false assumption that the issues are specific to the Krautoload approach.
We will have to solve this with or without Krautoload.

I am attempting something like this:

// Encapsulated namespace collections, with "arithmetics" ...
$pluginNamespaces = $rootNamespaces->buildFromSuffix("\\Plugin\\$suffix");
$annotationNamespaces = $rootNamespaces->buildFromNamespaces(array('Drupal\Component\Annotation', 'Drupal\Core\Annotation'));

// .. and with discovery capabilities ..
$discoveryAPI = new PluginDiscoveryAPI($this->pluginDefinitionAnnotationName);
$recursive = FALSE;
$pluginNamespaces->apiScanAll($discoveryAPI, $recursive);
$definitions = $discoveryAPI->getDefinitions();

The "$discoveryAPI" implements the operation that is to be executed for each class/file that is found.
Its role is similar to a callback passed around, if you do functional programming.

I am a bit puzzled about the purpose of the annotation namespaces, and the AnnotationRegistry.

The essential places to look are
DocParser->classExists($fqcn)
AnnotationRegistry::loadAnnotationClass($fqcn)

The idea seems to be to decide for a given class name, whether that class can be used for annotations or not.

The odd thing is, if a class is already loaded, then this question will always be answered with "Yes".
This does make the system quite unpredictable.

I wonder which behavior is desirable for us? Do we want to restrict annotations to those relevant for one particular plugin type, or do we always want to allow all?

Re #4:
timplunkett was helpful on IRC.
Conclusion: The "leaking" of annotation classes from previous operations is not a desirable or necessary behavior. On the other hand, it is probably not harmful (says timplunkett). (*)

If I rewrite the AnnotationRegistry, I don't have to reproduce the leaking. Instead, I make only those annotation classes available that are explicitly asked for, for this particular plugin type.

The Doctrine parsing code has so much static variables and static method calls and "final", it's not funny anymore :(

Comments with "#" added by myself.

// # Why final?
final class DocParser
    [..]
    private function collectAnnotationMetadata($name)
    {
        if (self::$metadataParser == null){
            // # We already have an instance, why create a new one and register it static?
            self::$metadataParser = new self();
            [..]
            // # I already mentioned this one..
            AnnotationRegistry::registerFile(__DIR__ . '/Annotation/Target.php');
        [..]
        self::$annotationMetadata[$name] = $metadata;

How am I supposed to subclass that :(
I think the only option is autoload override of AnnotationRegistry. This will not fix the "leaking" I described in #4 / #5, but this has not been regarded as a problem before either.

We can then talk to the Doctrine people, if they plan to change any of it.
When an improved version is available, we can remove the override.

Oh, me stupid!!
We don't have to do any of this, because of this nice method in AnnotationRegistry:

    static public function registerLoader($callable)

That's it, problem solved :)

An attempt to solve this:
#2033501-4: Explore Krautoload as a solution for PSR-4 class loading

1In Drupal\Component\Plugin\Discovery\AnnotatedClassDiscovery

  function __construct(SearchableNamespacesInterface $plugin_namespaces, SearchableNamespacesInterface $annotation_namespaces, $plugin_definition_annotation_name = 'Drupal\Component\Annotation\Plugin') {
    $this->pluginNamespaces = $plugin_namespaces;
    $this->annotationNamespaces = $annotation_namespaces;
    $this->pluginDefinitionAnnotationName = $plugin_definition_annotation_name;
  }

1. What is a Drupal-specific default value doing in a "Component" class? Duh..
2. The protected variable of are not used by the "Core" subclass.

I would suggest to
- make the "Component" version an abstract class.
- make the "getPluginNamespaces" and "getAnnotationNamespaces" abstract methods.
- reduce the constructor to just one argument, $plugin_definition_annotation_name. But without a default value.
- remove the protected variables, except pluginDefinitionAnnotationName.

There could still be a default implementation in "Component", but we can live without that.

And this .. (in Core AnnotatedClassDiscovery)

    $this->subdir = str_replace('/', '\\', $subdir);

Grmm..
if you put namespace separators, then it's no longer a subdir, but a sub-namespace, or namespace suffix.
Misleading variable name.

EDIT:
Besides, there is no protected attribute for $this->subdir.

I really would like to see some feedback here!

I am personally easily tempted to change more rather than less, so I need people to stop me.

Options:

1. Continue to pass around an array of namespace-to-directory mappings, but make it PSR-X instead of PSR-0.
   This means more work when we re-fit those directories for a subdirectory..
2. Pass around a "SearchableNamespaces" collection object with built-in class discovery.
   I think this would be pretty cool, but I want to hear other opinions.
   There is a patch do do this with Krautoload: #2033501-21: Explore Krautoload as a solution for PSR-4 class loading
   But we could also do something Drupal-native instead.
   Or we could wrap the Krautoload SearchableNamespaces thingie into a Drupal-native adapter, so we can swap out the implementation.
3. Plugin managers receive an "AnnotatedClassDiscoveryFactory" object that has the namespace mappings built in, so that plugin managers only have to specify the namespace suffix in the factory method.

Plugin managers and AnnotatedClassDiscovery currently have a few inconsistencies in the signature, this would be a chance to standardize.

Maybe a little preview helps.
This is from EntityManager, following the idea in #11, "2.".

  public function __construct(SearchableNamespacesInterface $root_namespaces, ContainerInterface $container, ModuleHandlerInterface $module_handler, CacheBackendInterface $cache, LanguageManager $language_manager) {
    [..]
    $this->discovery = new AnnotatedClassDiscovery($root_namespaces, 'Core\Entity', 'Drupal\Core\Entity\Annotation\EntityType');
    $this->discovery->addAnnotationNamespace('Drupal\Core\Entity\Annotation');

Changes:
- container.namespaces becomes a SearchableNamespaces object.
- Every time we deal with the container.namespaces in plugin managers, the variable name should be $root_namespaces. (so far this was inconsistent)
- $annotation_namespaces is no longer a constructor argument in AnnotatedClassDiscovery. Instead, you can add those one-by-one.
- To specify an annotation namespace, you no longer need to specify a filesystem location, only the namespace name.
- Instead of a "$subdir" argument, we now have a "$namespace_suffix" argument (in this case it is 'Core\Entity'.
- The $namespace_suffix comes *after* the $root_namespaces argument, because the suffix will be appended to those.

And this would be the constructor change in DefaultPluginManager.

-  public function __construct($subdir, \Traversable $namespaces, $annotation_namespaces = array(), $plugin_definition_annotation_name = 'Drupal\Component\Annotation\Plugin') {
-    $this->subdir = $subdir;
-    $this->discovery = new AnnotatedClassDiscovery($subdir, $namespaces, $annotation_namespaces, $plugin_definition_annotation_name);
+  public function __construct(SearchableNamespacesInterface $root_namespaces, $namespace_suffix, $annotation_namespaces = array(), $plugin_definition_annotation_name = 'Drupal\Component\Annotation\Plugin') {
+    $this->discovery = new AnnotatedClassDiscovery($root_namespaces, $namespace_suffix, $plugin_definition_annotation_name);
+    foreach ($annotation_namespaces as $namespace) {
+      $this->discovery->addAnnotationNamespace($namespace);
+    }

(I will cross-post over at #1903346: Establish a new DefaultPluginManager to encapsulate best practices.)

Tagging.

#15 (Crell)
This is an option we have, but it has nothing to do with AnnotatedClassDiscovery and plugin managers..

If module directory structure switches to PSR-4, then we need not only change class loading, but also class discovery.
This is for
- discovered plugins
- annotation classes (where we need to check whether they exist in a constrained namespace list)
- discovered test classes

tstoeckler (#17):
Yes! So.. which of the options in #11 do you prefer?

Ok.
This needs more thought.

• SearchableNamespaces
  I think we could agree that this is a useful abstraction.
  This object has a list of namespaces (without directory information), *and* a helper object to determine the directories to search for each namespace.
• NamespaceInspector
  This is / would be the object that knows the directory for each namespace, and contains the implementation of the discovery algorithm.
  In the Krautoload patch, the NamespaceInspector instance is identical with the ClassLoader instance. But another implementation is imaginable, where they are different objects.
• NamespaceInspectorAdapter
  In the Krautoload patch, we have a separate object that wraps the NamespaceInspector and adds convenience methods.

Now the problem lies here:

the NamespaceInspector instance is identical with the ClassLoader instance. But another implementation is imaginable, where they are different objects.

Why is this a problem?

In DrupalKernel, we feed module directory information into the ClassLoader, and into the container.namespaces service.

With Krautoload, we only feed this information into the NamespaceInspector, which at the same time is a ClassLoader. The container.namespaces service already has a reference ot the NamespaceInspector, and thus only needs a flat list of namespaces without directory information. Thus, the DrupalKernel does not need to care about PSR-0 vs PSR-4.

If ClassLoader and NamespaceInspector are separate objects, then DrupalKernel needs to feed the namespace directory information into both.

Currently this would need to happen in these places:
- DrupalKernel
- UpdateModuleHandler
- system_register() (called by system_list()) -> why the heck..
- simpletest_classloader_register()

If we want the ClassLoader to be swappable as in #2023325: Add interface for classloader so it can be cleanly swapped, then we need to support both cases.

Solution:
I thought about adding a mediator object in between: An object that is told about currently enabled extensions' directories, and that passes this information on to the ClassLoader or NamespaceInspector or both, or whatever object that needs it.

The ClassLoader/NamespaceInspector could be plugged into the Mediator as an observer.
Or alternatively, the default mediator would be hardcoded to only support the default ClassLoader/NamespaceInspector. Or the NamespaceInspector could even act as the mediator itself.
To replace the ClassLoader, you would have to provide a new mediator that treats ClassLoader and NamespaceInspector as separate objects.

The above places would only deal with the mediator, and they would only tell it:
- A module has been enabled, with the following directory.
- A number of modules have been enabled, with the following directories (or info file paths)
- A module has been disabled.
- A module has moved to a new directory.
- Tests are being activated for the following modules: ...

Does this make sense?

In fact, couldn't we switch container.namespaces to that approach now and fix AnnotatedClassDiscovery and related code accordingly, without even moving any files around or changing autoloaders?

Yes, we can!
It will make a few places slightly more complicated. But maybe that's the wrong time to worry about that.

Just to clarify what this would mean:

// 1. container.namespaces passed around.
// old
  'Drupal\system' => 'core/modules/lib'
// new
  'Drupal\system' => 'core/modules/lib/Drupal/system'

// 2. building sub-namespace list (only within AnnotatedClassDiscovery)
// old
  $namespace . '\Plugin\PluginType' => $dir
// new
  $namespace . '\Plugin\PluginType' => $dir . '/Plugin/PluginType'

// 3. build annotation namespace (happens in a number of plugin managers)
// old
  $annotation_namespaces = array('Drupal\ckeditor\Annotation' => $namespaces['Drupal\ckeditor']);
// new
  $annotation_namespaces = array('Drupal\ckeditor\Annotation' => $namespaces['Drupal\ckeditor'] . '/Annotation');

So, a little more to do, and some potential to do it wrong.
But not really so bad.
It would be more of a thing if we actually had any underscores in class names. But we deliberately don't use them.

I could have gotten to that conclusion earlier .. I was stuck too deep in complex ideas.

Ah.. I think there was one more caveat.
If we actually attempt to support both lib (PSR-0) and src (PSR-4) at the same time, then those lists would need to have an array-valued directory list.
Which some places in the code already assume, but others don't.

So..

// 1. container.namespaces passed around.
// old
  'Drupal\system' => 'core/modules/lib'
// new
  'Drupal\system' => array('core/modules/src', 'core/modules/lib/Drupal/system')

// 3. build annotation namespace (happens in a number of plugin managers)
// old
  $annotation_namespaces = array('Drupal\ckeditor\Annotation' => $namespaces['Drupal\ckeditor']);
// new
  foreach ((array) $namespaces['Drupal\ckeditor'] as $dir) {
    $annotation_namespaces['Drupal\ckeditor\Annotation'][] = $dir . '/Annotation';
  }

Now this is really getting more ugly in the new version.
And I think this was my main motivation to encapsulate.

// encapsulated
  $annotation_namespaces = $namespaces->buildFromNamespaces(array('Drupal\ckeditor\Annotation'));

So most of the code would not have to deal with directories at all.
But there is another way out. (next post)

So, here is the alternative:

// new, but ugly
  foreach ((array) $namespaces['Drupal\ckeditor'] as $dir) {
    $annotation_namespaces['Drupal\ckeditor\Annotation'][] = $dir . '/Annotation';
  }
  $discovery = new AnnotatedClassDiscovery('Plugin/CKEditorPlugin', $namespaces, $annotation_namespaces, 'Drupal\ckeditor\Annotation\CKEditorPlugin');

// new, alternative
  $discovery = new AnnotatedClassDiscovery($namespaces, '\Plugin\CKEditorPlugin', 'Drupal\ckeditor\Annotation\CKEditorPlugin');
  $discovery->addAnnotationNamespace('Drupal\ckeditor\Annotation');

Note: The original CKEditorPlugin does not actually create an AnnotatedClassDiscovery object, but instead calls the parent constructor. However, the arguments are the same.

So, proposed changes:

1. Instead of a $subdir argument, we give the AnnotatedClassDiscovery a namespace suffix. The AnnotatedClassDiscovery will need both (namespace and directory), but it can contruct one from the other.
   But for a plugin manager, a namespace suffix has more meaning than a directory.
2. The namespace suffix argument should go after the list of root namespaces, not before it. It's a suffix.
3. Annotation namespaces removed from constructor signature, and instead we now have addAnnotationNamespace(). This way, the plugin manager does not have to construct annotation namespace directories, and can instead just specify the namespace directly. The directories are then constructed by the AnnotatedClassDiscovery itself.

When undecided, upload one patch for each variation :)

Missed one.

Let's have an intermediate step where the AbstractAnnotatedClassDiscovery is introduced.

1. interdiff from "psr4style" from #25 to the AbstractAnnotatedClassDiscovery patch
2. new intermediate patch to introduce AbstractAnnotatedClassDiscovery
3. interdiff from AbstractAnnotatedClassDiscovery to addAnnotationNamespace in #25.

Having multiple patches at once is confusing.
So, instead I upload the one with the most changes.

If you think this patch goes too far, maybe the history contains something better..
https://github.com/donquixote/drupal/compare/8.x-discovery-addAnnotation...

Stuff fixed.

Lots of rebase on github, so the history now represents the different options of how far we gan go.
https://github.com/donquixote/drupal/compare/8.x-discovery-addAnnotation...

The problem was EntityManager::addNamespaces()
https://github.com/donquixote/drupal/commit/73ed42a226c3a5eea438b979a8c1...
fix squashed into the larger commit

Here is the history, again
https://github.com/donquixote/drupal/compare/8.x-discovery-addAnnotation...

And how would it be if we can register more than one directory per namespace?
Profit: We can already register lib/ and src/ and lib/Drupal/modulename/ as PSR-4! So it will already work when the conversion script fires.
https://github.com/donquixote/drupal/commit/1599698995b9f3fcc79ffc52c58d...

It is only that easy because of the addAnnotationNamespace() in AnnotatedClassDiscovery and DefaultPluginManager.
Otherwise, plugin managers would all have to deal with array-valued namespace paths when building the annotation namespaces.

We/I might be overcomplicating it with the annotation namespaces :)

Afaik, our annotation namespaces are only ever in enabled modules.

So why not simply

  public function loadAnnotationClass($class) {

    if (class_exists($class, FALSE)) {
      return TRUE;
    }

    // Check if the class is in a known annotation namespace.
    foreach ($this->getAnnotationNamespaces() as $namespace => $dirs) {
      if (0 === strpos($class, $namespace)) {
        return class_exists($class);
      }
    }

    return FALSE;
  }

Now we don't need the $dirs at all!

Helper issue at #2083335: Annotation namespaces are stupid.

https://github.com/donquixote/drupal/compare/8.x-discovery-addAnnotation...

Now using the same approach as
#2083335-3: Annotation namespaces are stupid.

diff: https://github.com/donquixote/drupal/commit/405c92d130075ff47b27417c7520...
I could squash this in, but I think it is useful to look at.

History, as always:
https://github.com/donquixote/drupal/compare/8.x-discovery-addAnnotation...

History:
https://github.com/donquixote/drupal/compare/D8-psr4-discovery-2033611-45

https://github.com/donquixote/drupal/compare/D8-psr4-discovery-2033611-46

EDIT: This is part of the larger history at
#2083547-37: PSR-4: Putting it all together

link to related issues, remove Krautoload link