Updated: Comment 0

Problem/Motivation

If you write any kind of plugin you need to put this two classes into the file in order to make it working.

Before


use Drupal\Core\Entity\Annotation\EntityType;
use Drupal\Core\Annotation\Translation;

/**
 * @EntityType(
 */

Proposed resolution

Add the @Translation and @{$PluginType} automatically, so the use statement is not needed anymore.

Some more information: http://docs.doctrine-project.org/projects/doctrine-common/en/latest/refe...

After


/**
 * @EntityType(
 */

Remaining tasks

Should we remove all the use statements all over core in this issue

User interface changes

API changes

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

Status: Active » Needs review
FileSize
6.26 KB

There we go, manually installing standard works for that.

dawehner’s picture

The magic code which makes this possible is in DocParser:

            if ($this->namespaces) {
                foreach ($this->namespaces as $namespace) {
                    if ($this->classExists($namespace.'\\'.$name)) {
                        $name = $namespace.'\\'.$name;
                        $found = true;
                        break;
                    }
                }

The reason why I had to switch to SimpleAnnotation is because the normal AnnotationReader class does not allow you to specify the namespaces.

larowlan’s picture

Issue tags: +Prague DX smackdown
dawehner’s picture

Issue tags: -Prague DX smackdown
FileSize
2.56 KB
5.98 KB

Removed the changes in doctrine itself.

damiankloip’s picture

+++ b/core/vendor/doctrine/annotations/lib/Doctrine/Common/Annotations/AnnotationReader.php
@@ -94,14 +94,14 @@ static public function addGlobalIgnoredName($name)
+    protected $preParser;
...
+    protected $phpParser;

@@ -315,4 +315,27 @@ private function collectParsingMetadata(ReflectionClass $class)
+
...
+

I really like this patch, I think just being able to use the core annotation classes is great, and the various plugin implementations using their own (Like Views, ViewsDisplayPlugin etc..) can almost be enforced (kind of).

I guess this is the only issue... :) Have you created an upstream patch?

dawehner’s picture

FileSize
1005 bytes
5.99 KB

Chx suggested to use that piece of code to get the namespace.

chx’s picture

Status: Needs review » Reviewed & tested by the community

So. Nice!

damiankloip’s picture

Oh, you don't even need to. Great. Looks good!

Edit: I cross posted in #5 before I saw the patch after it.

tim.plunkett’s picture

This is insanely awesome.

EDIT:
To clarify to committers, this only removes a handful of "use" annotation lines to keep the patch down. But this essentially means we'll never need to "use" an annotation in any file.

dawehner’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
8.06 KB
3.75 KB

Let's move the drupal core specific code into the core namespace.

EclipseGc’s picture

+++ b/core/lib/Drupal/Component/Plugin/Discovery/AnnotatedClassDiscovery.phpundefined
@@ -7,9 +7,9 @@
+use Doctrine\Common\Annotations\SimpleAnnotationReader;

This isn't currently used in the Component at all.

+++ b/core/lib/Drupal/Component/Plugin/Discovery/AnnotatedClassDiscovery.phpundefined
@@ -51,6 +58,19 @@ function __construct($plugin_namespaces = array(), $plugin_definition_annotation
+    if (!isset($this->annotationReader)) {
+      $this->annotationReader = new AnnotationReader();
+    }
+    return $this->annotationReader;

I really expected to see the SimpleAnnotationReader here. It's strange that we then override this completely in the Core portion of the code. I expected the namespace of the pluginDefinitionAnnotationName to be added to the reader here and have the Core portion of the code call parent::getAnnotationReader().

+++ b/core/lib/Drupal/Core/Plugin/Discovery/AnnotatedClassDiscovery.phpundefined
@@ -57,6 +58,23 @@ function __construct($subdir, \Traversable $root_namespaces, $plugin_definition_
+  protected function getAnnotationReader() {
+    if (!isset($this->annotationReader)) {
+      $this->annotationReader = new SimpleAnnotationReader();
+
+      // Add the Core annotation classes like @Translation.
+      $this->annotationReader->addNamespace('Drupal\Core\Annotation', array(DRUPAL_ROOT . '/core/lib/Drupal/Core/Annotation'));
+
+      // Add the namespaces from the main plugin annotation, like @EntityType.
+      $namespace = substr($this->pluginDefinitionAnnotationName, 0, strrpos($this->pluginDefinitionAnnotationName, '\\'));
+      $this->annotationReader->addNamespace($namespace);
+    }
+    return $this->annotationReader;

Wondering if we can get a sanity check from donquixote on this with regards to PSR-4. If that all checks out, then I'm on board, but I really do think that the substr()/addNamespace() combo here belongs in the Component.

Otherwise I generally like the basic approach here.

Eclipse

EclipseGc’s picture

Status: Needs review » Needs work

oops

Eclipse

dawehner’s picture

Status: Needs work » Needs review
FileSize
2.7 KB
7.81 KB

My idea was to make the component as generic as possible, but sure we can just do it like that.

Status: Needs review » Needs work
Issue tags: -DX (Developer Experience)

The last submitted patch, plugins-2090353-13.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
Issue tags: +DX (Developer Experience)

#13: plugins-2090353-13.patch queued for re-testing.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

Great points @EclipseGc, great work @dawehner.

+++ b/core/lib/Drupal/Component/Plugin/Discovery/AnnotatedClassDiscovery.php
@@ -63,10 +86,8 @@ public function getDefinition($plugin_id) {
-    // Prevent @endlink from being parsed as an annotation.
-    $reader->addGlobalIgnoredName('endlink');
-    $reader->addGlobalIgnoredName('file');

I love that we get to skip this completely.

EclipseGc’s picture

++ing the RTBC

Eclipse

webchick’s picture

Title: Don't require to put the use statement into plugin classes » Change notice: Don't require to put the use statement into plugin classes
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

HOLY CRAP I LOVE THIS PATCH SO MUCH!!!!

Committed and pushed to 8.x. YEAH!

Needs a change notice.

dawehner’s picture

Here is one: [#2096117]
I am wondering whether we should change all the existing ones for all the various plugin ones.

EclipseGc’s picture

Yeah, I mean, low priority, but it seems like the sort of thing we should go ahead and begin implementing in our plugins.

Eclipse

swentel’s picture

jibran’s picture

Just a silly question Can we do this for contirb as well?

dawehner’s picture

Just a silly question Can we do this for contirb as well?

This works for all plugin annotations you define, if your plugin manager supports it specify the plugin annotation. In other
words core modules are just a special case of modules, so contrib modules will just work as well.

ParisLiakos’s picture

is there another way to do #1862202-172: Objectify the language system without putting the full namespace there?
not even the current namespace of the class is allowed:(
also other use statements in your class are not imported, which used to

Edit opened #2097585: Allow plugin managers to register namespaces to the annotation reader for this

ParisLiakos’s picture

Issue summary: View changes

blub

webchick’s picture

Title: Change notice: Don't require to put the use statement into plugin classes » Don't require to put the use statement into plugin classes
Issue summary: View changes
Status: Active » Fixed

This has a change notice, so I think we're good here?

webchick’s picture

Status: Fixed » Closed (fixed)

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

xjm’s picture

Issue tags: -Needs change record
klausi’s picture

This is currently being discussed to be reverted as IDEs such as PHPStorm give you useful hints when the use statement is there, see #2689243: Import classes used in annotations.

yonailo’s picture