The annotations reader crashes when parsing entries like :

/**
 * @Plugin(
 *   id = "foo",
 *   label = @Translation("Foo"),
 *   foobar = { }
 * )
 */

or (nested)

/**
 * @Plugin(
 *   id = "foo",
 *   label = @Translation("Foo"),
 *   foobar = {
 *     baz = {
 *     }
 *   }
 * )
 */

--> "Doctrine\Common\Annotations\AnnotationException: [Syntax Error] Expected PlainValue, got '}' at position N in class Foo"

This limits the expressiveness of annotations. We're currently storing "formatter settings" with their default values in there, and array() is a totally legit 'default value'.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

Priority: Major » Normal

This is by design in the upstream library. Haven't you worked around it?

yched’s picture

TBH I don't remember what was the specific thing I was working on back then.

But not being able to include empty arrays as a value in annotations does sound like an issue. Of course, the exact level of WTFness depends on what kind of stuff exactly we put in annotations - discovery-only stuff, or arbitrary metadata (settings, etc...). But the exact content of annotations is basically up to each plugin type, so it's hard to say what the exact use cases will be anyway.

Xano’s picture

I am experiencing the same issue with a constraint I am writing that requires no options. Passing on an empty array is not possible. Passing on an array with items is not desired (the constraint should and does work fine without options). Passing on a scalar overrides the default value.

This is by design in the upstream library. Haven't you worked around it?

Can you elaborate on this, or perhaps provide a source?

dawehner’s picture

Xano’s picture

The issue had to be moved to doctrine/annotations. See https://github.com/doctrine/annotations/pull/8 instead.

swentel’s picture

So apparently this has been fixed in https://github.com/doctrine/annotations/pull/8, should we pull in the new doctrine ?

swentel’s picture

Priority: Normal » Critical

So this is critical as we're hitting this in converting the field types for options and entity reference.

Xano’s picture

Assigned: Unassigned » Xano
Xano’s picture

Assigned: Xano » Unassigned

Houston, we have a challenge. Core depends on Doctrine/Common 2.3, which has not received any love for a year now, so it is unlikely that we can have the empty array fix committed to that branch. When work on Doctrine/Common 2.4 started, the Annotations component was split off to Doctrine/Annotations 1.0, which is still in dev and is the only code base that contains the fix.

catch’s picture

I don't see a problem with pulling in Annotations dev particularly. We've still got some time for that to get to a stable release before we do.

Xano’s picture

#9 contains some incorrect information: Doctrine/Annotations 1.1.1 has been released, so the project is stable, but there is no release that includes the fix yet. However, as the project is stable and actively maintained, I suspect we can use the dev branch until the next release comes out, which will likely be long before Drupal 8.0 is released.

If that is okay, I will roll a patch tonight. This means that we will have two versions of the Annotations component in core, though.

catch’s picture

That also seems fine.

Are we using anything else in Doctrine/Common - if not then we should remove that at the same time.

Xano’s picture

Assigned: Unassigned » Xano

I'm in touch with the Doctrine\Annotations maintainer about creating a new minor release. Will work on a patch for this issue once the new release has been made.

Xano’s picture

Title: Annotations reader crashes on "empty array" entries » Upgrade to Doctrine\Common 2.4
Assigned: Xano » Unassigned
Status: Active » Needs review
FileSize
551.14 KB

We need Doctrine\Annotations 11.2, which was released earlier today, and Doctrine\Reflection, which are both part of Doctrine\Common 2.4. Let's upgrade and see what trouble I'm getting myself into.

dawehner’s picture

Please also provide a diff which just contains the changes of the composer file and maybe needed fixes here and there.

Xano’s picture

Certainly, sir.

amateescu’s picture

Status: Needs review » Reviewed & tested by the community

I don't think we have anything else to do here, and this is badly needed for field type conversions.

Xano’s picture

On IRC I was asked to give a more in-depth response to the following quote:

Are we using anything else in Doctrine/Common - if not then we should remove that at the same time.

So until the Doctrine folks split off Reflection (it's really just one silly class we need :') ) we'll have to pull Common in its entirety. Even if we'd manage to pull just the Reflection part of Common, there is no knowing whether or not it will depend on other components in Common in the future,

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

Status: Fixed » Closed (fixed)

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