[PP-1] Make doctrine/lexer:^3.0 compatible with \Drupal\Component\Annotation\Doctrine.

Created on 17 March 2024, about 1 year ago

Problem/Motivation

In πŸ› Annotation component has an undeclared dependency on doctrine/lexer 2 RTBC 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

πŸ“Œ Task
Status

Active

Version

11.0 πŸ”₯

Component
BaseΒ  β†’

Last updated about 2 hours ago

Created by

πŸ‡³πŸ‡±Netherlands spokje

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @spokje
  • Status changed to Postponed about 1 year ago
  • Status changed to Active about 1 year ago
  • πŸ‡³πŸ‡±Netherlands spokje
  • πŸ‡³πŸ‡ΏNew Zealand quietone
  • πŸ‡«πŸ‡·France andypost

    still would be great to have this compatibility in 11.2

  • πŸ‡¨πŸ‡¦Canada Charlie ChX Negyesi 🍁Canada

    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 dropped ArrayAccess I can't even imagine, it was four one line methods.

    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.

  • πŸ‡¬πŸ‡§United Kingdom catch

    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.

  • πŸ‡¬πŸ‡§United Kingdom longwave UK

    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.

  • Merge request !12088Draft: Resolve #3429849 "Test only" β†’ (Open) created by longwave
  • Pipeline finished with Success
    1 day ago
    Total: 742s
    #493032
  • Pipeline finished with Success
    1 day ago
    Total: 923s
    #493033
  • πŸ‡¨πŸ‡¦Canada Charlie ChX Negyesi 🍁Canada

    > can we make it compatible with the object way of doing things,

    It's Turing complete, nothing stops you :P

    Can is not the problem, want is. It's a bit painful to do it.

  • πŸ‡¬πŸ‡§United Kingdom longwave UK

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

  • πŸ‡«πŸ‡·France andypost

    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

Production build 0.71.5 2024