Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Possible BC break introduced in doctrine/annotations dependency because it has no upper boundary #6706

Closed
alcohol opened this issue Dec 12, 2022 · 5 comments
Labels

Comments

@alcohol
Copy link

alcohol commented Dec 12, 2022

Bug report

$ ./php-cs-fixer -vvv fix Test.php
PHP CS Fixer 3.13.0 Oliva by Fabien Potencier and Dariusz Ruminski.
PHP runtime: 8.1.4
Loaded config polyestershoppen.nl from "/srv/.php-cs-fixer.dist.php".
Using cache file ".php-cs-fixer.cache".
Paths from configuration file have been overridden by paths provided as command arguments.
E                                                                                                                                                                                                                                                                                                                                              1 / 1 (100%)
Legend: .-no changes, F-fixed, S-skipped (cached or empty file), I-invalid file syntax (file ignored), E-error

Fixed all files in 0.012 seconds, 14.000 MB memory used

Files that were not fixed due to errors reported during fixing:
   1) /srv/Test.php

                                    
        [ErrorException]            
        Undefined array key "type"  
                                    

      {closure}()
        in /srv/tools/php-cs-fixer/vendor/friendsofphp/php-cs-fixer/src/Doctrine/Annotation/Tokens.php at line 74
      PhpCsFixer\Doctrine\Annotation\Tokens::createFromDocComment()
        in /srv/tools/php-cs-fixer/vendor/friendsofphp/php-cs-fixer/src/AbstractDoctrineAnnotationFixer.php at line 62
      PhpCsFixer\AbstractDoctrineAnnotationFixer->applyFix()
        in /srv/tools/php-cs-fixer/vendor/friendsofphp/php-cs-fixer/src/AbstractFixer.php at line 75
      PhpCsFixer\AbstractFixer->fix()
        in /srv/tools/php-cs-fixer/vendor/friendsofphp/php-cs-fixer/src/Runner/Runner.php at line 173
      PhpCsFixer\Runner\Runner->fixFile()
        in /srv/tools/php-cs-fixer/vendor/friendsofphp/php-cs-fixer/src/Runner/Runner.php at line 114
      PhpCsFixer\Runner\Runner->fix()
        in /srv/tools/php-cs-fixer/vendor/friendsofphp/php-cs-fixer/src/Console/Command/FixCommand.php at line 309
      PhpCsFixer\Console\Command\FixCommand->execute()
        in /srv/tools/php-cs-fixer/vendor/symfony/console/Command/Command.php at line 312
      [ ... ]

This looks to be caused by something of a BC break because the dependency on doctrine/annotations has no upper boundary; a quick peek shows that https://github.com/PHP-CS-Fixer/PHP-CS-Fixer/blob/master/src/Doctrine/Annotation/Tokens.php#L61 only does a null check, but https://github.com/doctrine/annotations/blob/1.14.x/lib/Doctrine/Common/Annotations/DocLexer.php#L137 forces an array to be returned, even if the parent class returned null (thus resulting in an empty array). The latter was not the case for doctrine/annotations version v1.13.x because in that version it inherited the peek method from https://github.com/doctrine/lexer/blob/2.0.x/src/AbstractLexer.php which does return null (1.13.x uses lexer 1.x, but the peek implementation has not changed).

Code snippet that reproduces the problem

<?php declare(strict_types=1);

namespace Test;

use Doctrine\ORM\Mapping as ORM;

/**
 * @ORM\Table()
 * @ORM\Entity()
 */
class Test
{
    /**
     * @ORM\Column(name="id", type="integer")
     * @ORM\Id
     * @ORM\GeneratedValue
     */
    private int $id;
}
@icanhazstring
Copy link

Possible open PR already, but should probably be adjusted:
#6705

@smichaelsen
Copy link

Pinning doctrine/annotations at version 1.13.3 does the job as a temporary workaround.

@icanhazstring
Copy link

Changing the range is stupid with any update service (renovate/dependabot). Conflict would be more feasible.

@derrabus
Copy link
Contributor

Did doctrine/annotations#463 fix the issue?

@alcohol
Copy link
Author

alcohol commented Dec 12, 2022

After adding a conflict explicitly for 1.14.0 and receiving an upgrade to 1.14.1, I can confirm that the issue is resolved (at least on my end).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants