Problem/Motivation

In #3429602: Annotation component has an undeclared dependency on doctrine/lexer 2 we're adding a dependency on doctrine/lexer:^2.0 to both the Annotation component and core itself.

doctrine/lexer:^3.0 currently throws a fatal error:

Steps to reproduce

$ composer update doctrine/lexer
$ drush cr
PHP Fatal error:  Uncaught Error: Cannot use object of type Doctrine\Common\Lexer\Token as array in drupal/core/lib/Drupal/Component/Annotation/Doctrine/DocParser.php:619

Proposed resolution

Make doctrine/lexer:^3.0 compatible with \Drupal\Component\Annotation\Doctrine.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#14 3429849-nr-bot.txt91 bytesneeds-review-queue-bot

Issue fork drupal-3429849

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

Spokje created an issue. See original summary.

spokje’s picture

spokje’s picture

Title: [PP-1] Make doctrine/lexer:^3.0 compatible with \Drupal\Component\Annotation\Doctrine. » Make doctrine/lexer:^3.0 compatible with \Drupal\Component\Annotation\Doctrine.
Status: Postponed » Active
quietone’s picture

andypost’s picture

still would be great to have this compatibility in 11.2

ghost of drupal past’s picture

DocParser was written for Lexer 1 when token was an array, 2 added an object with ArrayAccess support and 3 dropped this support finalizing the move to object. "Luckily" the class is final so there is no easy way to add ArrayAccess back for the short(?) time annotations are still supported before everything becomes an attribute. (I will crosslink this issue from the @final issue as a textbook example of why final is bad.) Also, why they deprecated and later dropped ArrayAccess I can't even imagine, it was four one line methods. Another example of upstream being downright hostile to users. Neither the first, nor the last.

Recommendation: do not upgrade. Let this be until it's removed.

If that's not desirable I guess I can write a rector rule to convert every array access to an object access unless the variable is called $metadata and pray it works.

catch’s picture

I agree upstream is hostile, every time we've raised anything like this it's been immediately won't fixed.

Two concerns with not upgrading though:

This will still be a dependency in Drupal 12, which we will be supporting until some time in 2030, so if there is a single security release in the next five years it will be annoying. But there is not really security surface area here, and if necessary we could fork.

Similarly, do we think this will support every PHP version released between now and at least 2028 or 2029? Given we hope nothing will actually be using annotation parsing at that point, PHP deprecations would probably be OK but a hard break would not.

longwave’s picture

2 added an object with ArrayAccess support and 3 dropped this support finalizing the move to object.

Given we control \Drupal\Component\Annotation\Doctrine\DocParser and it's already final, can we make it compatible with the object way of doing things, and allow ^2 || ^3 in composer.json, but stay pinned to ^2 (for now) in core-recommended? This adds forward compatibility for anyone who wants to upgrade for other reasons, but stays conservative otherwise - but also opens the door to finishing the upgrade in Drupal 12 if we want to. If the token arrays/objects are not leaked outside of the implementation this seems like it might be workable.

ghost of drupal past’s picture

OMG you already converted it?? Does that work??

longwave’s picture

Status: Active » Needs review

It passes tests, but I'm not sure how much we exercise the code any more, given we have few annotations left in core...

MR!12087 is the conversion but remains pinned to 2.x.
MR!12088 is the conversion plus upgrade to 3.x to ensure it works on both versions.

andypost’s picture

Status: Needs review » Reviewed & tested by the community

Looks it should be simple rector rule as there's only value, type and position are actually used by core as test-only MR showing all is compatible

needs-review-queue-bot’s picture

Status: Reviewed & tested by the community » Needs work
StatusFileSize
new91 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

andypost’s picture

Status: Needs work » Needs review

rebased and cleaned up the drupal/core-recipe-unpack

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

Seems pretty straight forward, don't see a reason no

catch’s picture

Status: Reviewed & tested by the community » Needs work

Needs another rebase.

andypost’s picture

Status: Needs work » Needs review

rebased

catch’s picture

Status: Needs review » Reviewed & tested by the community
mondrake’s picture

Status: Reviewed & tested by the community » Needs work

The composer lock test fail on PHP 8.3 seems a legit one.

andypost’s picture

Status: Needs work » Needs review

sorry, should be solved now, messed in conflict resolution

mondrake’s picture

Status: Needs review » Reviewed & tested by the community

back to rtbc

  • catch committed 5a658211 on 11.x
    Issue #3429849 by longwave, andypost, spokje: Make doctrine/lexer:^3.0...
catch’s picture

Status: Reviewed & tested by the community » Patch (to be ported)

Committed/pushed to 11.x, thanks!

This will need a backport MR for 11.2.x, although I also don't think it hurts if it's 11.3.x-only?

catch’s picture

Version: 11.x-dev » 11.2.x-dev

andypost’s picture

Status: Patch (to be ported) » Needs review

created MR for backport

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

Seems like a good 11.2 backport.

catch’s picture

Version: 11.2.x-dev » 11.x-dev
Status: Reviewed & tested by the community » Fixed

Thought about this some more and discussed with @longwave. If we were going to backport this to 10.x then it would mitigate against a Doctrine 2.x security release, but that seems unlikely, at least not in 10.5.x, and also there is not really much security surface with this code. So let's leave it in 11.3.x - moving back to fixed.

Status: Fixed » Closed (fixed)

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