Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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'.
Comment | File | Size | Author |
---|---|---|---|
#16 | drupal_1848570_16_composer.json_.patch | 412 bytes | Xano |
#14 | drupal_1848570_14.patch | 551.14 KB | Xano |
Comments
Comment #1
tim.plunkettThis is by design in the upstream library. Haven't you worked around it?
Comment #2
yched CreditAttribution: yched commentedTBH 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.
Comment #3
XanoI 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.
Can you elaborate on this, or perhaps provide a source?
Comment #4
dawehnerSee https://github.com/doctrine/common/pull/270 for an issue.
Comment #5
XanoThe issue had to be moved to doctrine/annotations. See https://github.com/doctrine/annotations/pull/8 instead.
Comment #6
swentel CreditAttribution: swentel commentedSo apparently this has been fixed in https://github.com/doctrine/annotations/pull/8, should we pull in the new doctrine ?
Comment #7
swentel CreditAttribution: swentel commentedSo this is critical as we're hitting this in converting the field types for options and entity reference.
Comment #8
XanoComment #9
XanoHouston, 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.
Comment #10
catchI 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.
Comment #11
Xano#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.
Comment #12
catchThat also seems fine.
Are we using anything else in Doctrine/Common - if not then we should remove that at the same time.
Comment #13
XanoI'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.
Comment #14
XanoWe 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.
Comment #15
dawehnerPlease also provide a diff which just contains the changes of the composer file and maybe needed fixes here and there.
Comment #16
XanoCertainly, sir.
Comment #17
amateescu CreditAttribution: amateescu commentedI don't think we have anything else to do here, and this is badly needed for field type conversions.
Comment #18
XanoOn IRC I was asked to give a more in-depth response to the following quote:
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,
Comment #19
catchCommitted/pushed to 8.x, thanks!