Update AnnotationReader's metadata parser to use list of known ignored annotation by sanmai · Pull Request #235 · doctrine/annotations

sanmai · 2019-01-23T05:53:44Z

A parser used to collect parsing metadata basically ignores a known list of globally ignored annotation names, goes through loops checking annotations that do not need any checking. Let's fix that by communicating the list during initialization.

E.g. these checks should not be evaluated for known ignored annotations:

annotations/lib/Doctrine/Annotations/DocParser.php

Lines 682 to 690 in 386d3bb

    
           } elseif ( ! isset($this->ignoredAnnotationNames[$name]) 
        
               && isset($this->imports['__NAMESPACE__']) 
        
               && $this->classExists($this->imports['__NAMESPACE__'] . '\\' . $name) 
        
           ) { 
        
               $name  = $this->imports['__NAMESPACE__'].'\\'.$name; 
        
               $found = true; 
        
           } elseif (! isset($this->ignoredAnnotationNames[$name]) && $this->classExists($name)) { 
        
               $found = true; 
        
           }

Please let me know if this change needs a test. One way to test is to rig an additional autoloader that would fail on attempt to load a class named as one of the commonly-ignored annotations like @see.

Ocramius

Requires test case additions, backing the OP claims.

Also needed:

reword issue title to match changes
reword commit message to contain detailed reasoning of changes

sanmai · 2019-01-23T06:42:25Z

No problem, I'll add a test later. Meanwhile I'm open to suggestions about exact wording.

sanmai · 2019-01-23T07:40:00Z

@Ocramius please see if it is better now.

Ocramius

Looking good. @doctrine/doctrinecore can you please also check?

alcaeus

Nice catch. Thanks @sanmai!

lcobucci

Just some teeny tiny things (quite small to be honest). Nice catch indeed 👍

lcobucci · 2019-01-24T09:31:31Z

        self::assertEmpty($reader->getPropertyAnnotations($class->getProperty('foo')));
    }

+    public function testGloballyIgnoredAnnotationNotIgnored()


: void missing

I'm not sure how's that going in line with all other code in this file. Isn't it better to put these things aside? Like there's #177 which is supposed to do exactly what you're asking, but all over all code.

@sanmai I understand your frustration here, these are cosmetic things and don't have much impact on the behaviour of the code. However we have an agreement among maintainers to try as much as possible to make sure our CS is applied everywhere, even when the project isn't following it properly (that's why we have automated incremental checks on ORM and DBAL, for example).

We haven't had the chance of applying the incremental checks here and modifying the entire code base is a bit too much (as explained in #177), hence my comments.

I'm sorry for the confusion and will completely understand if you decide to not apply the suggestions in the comments, I'm just trying to ensure that we (Doctrine team) follow our agreements. A maintainer is able to edit your commit on your fork (if you hadn't changed the option while creating the PR).

I hope you'll understand the motivation and I thank you for your contribution.

Well, that's just me that spend a good amount of time to make sure that my code looks just like every other code, but if there's a consensus to write newer code in a more appropriate style, I'm good with that too. I'd change on my part but I'd like if these new guidelines would be somewhere, at least. Then anyone else wouldn't need to spend the time so useless as I did. Otherwise put, this discussion shouldn't have happen in the first place. Are we on the same page with this?

Also, all other folks now would need to review the thing again. Bummer.

@lcobucci anything else?

I'd change on my part but I'd like if these new guidelines would be somewhere, at least.

I believe that one is documented here: https://www.doctrine-project.org/projects/doctrine-coding-standard/en/latest/reference/index.html#project-level-ruleset ("Always add native types where possible")

Well, that's not exactly obvious, what do you think? How am I supposed to find it?..

A link to these guidelines from the README.md, or even from CONTRIBUTING.md, would be best. Nevertheless, I will know better now, thanks.

I think you're right! I might make PRs to change that if you do not beat me to it.

@greg0ire see #239

…d annotation A parser used to collect parsing metadata basically ignores a known list of globally ignored annotation names, goes through loops checking annotations that do not need any checking. Let's fix that by communicating the list during initialization.

lcobucci · 2019-01-28T10:02:27Z

@sanmai @Ocramius @Majkl578 @greg0ire I've found an interesting thing while backporting this to 1.7: the test passes even if $this->preParser->setIgnoredAnnotationNames(self::$globalIgnoredNames); isn't called 👍

Also interesting to mention it fails for SimpleAnnotationReaderTest, because it wrongly parses a lowercase annotation (@version).

Shall we mark it as an fix to v2.0 only?

Relevant discussion: doctrine#235 (comment)

sanmai · 2019-01-29T03:31:35Z

Actually I had this issue on a released version. Let me check what it was...

sanmai · 2019-01-30T06:31:18Z

@lcobucci I got this fix backported to 1.6, see #240

Relevant discussion: doctrine#235 (comment)

Ocramius suggested changes Jan 23, 2019

View reviewed changes

Ocramius added bug missing tests labels Jan 23, 2019

Ocramius assigned sanmai Jan 23, 2019

sanmai changed the title ~~Update AnnotationReader~~ Update AnnotationReader's metadata parser to use list of known ignored annotation Jan 23, 2019

sanmai force-pushed the patch-1 branch from 19a8fbc to 6a04cfd Compare January 23, 2019 07:38

Ocramius suggested changes Jan 23, 2019

View reviewed changes

Comment thread tests/Doctrine/Tests/Annotations/AbstractReaderTest.php

Ocramius added BC-Break and removed missing tests labels Jan 23, 2019

sanmai added a commit to sanmai/annotations that referenced this pull request Jan 23, 2019

Use a separate test for the issue doctrine#235

41691df

sanmai force-pushed the patch-1 branch from 41691df to 26f24fa Compare January 23, 2019 08:18

Ocramius previously approved these changes Jan 23, 2019

View reviewed changes

Ocramius removed the BC-Break label Jan 23, 2019

Ocramius added this to the 1.7.0 milestone Jan 23, 2019

Ocramius unassigned sanmai Jan 23, 2019

Ocramius requested review from Majkl578, alcaeus and lcobucci January 23, 2019 08:18

alcaeus previously approved these changes Jan 23, 2019

View reviewed changes

stof previously approved these changes Jan 23, 2019

View reviewed changes

SenseException previously approved these changes Jan 23, 2019

View reviewed changes

lcobucci suggested changes Jan 24, 2019

View reviewed changes

sanmai dismissed stale reviews from SenseException, stof, alcaeus, and Ocramius via 3eed959 January 25, 2019 03:42

greg0ire reviewed Jan 28, 2019

View reviewed changes

Comment thread tests/Doctrine/Tests/Annotations/AbstractReaderTest.php Outdated

greg0ire previously approved these changes Jan 28, 2019

View reviewed changes

lcobucci dismissed greg0ire’s stale review via 0485434 January 28, 2019 09:30

lcobucci force-pushed the patch-1 branch from 3007675 to 0485434 Compare January 28, 2019 09:30

lcobucci approved these changes Jan 28, 2019

View reviewed changes

lcobucci self-assigned this Jan 28, 2019

lcobucci merged commit 21e7da9 into doctrine:master Jan 28, 2019

sanmai deleted the patch-1 branch January 29, 2019 03:26

sanmai added a commit to sanmai/annotations that referenced this pull request Jan 29, 2019

Mention coding standard requirements in the README

5a1fa23

Relevant discussion: doctrine#235 (comment)

sanmai mentioned this pull request Jan 29, 2019

Mention coding standard requirements in the README #239

Merged

sanmai mentioned this pull request Jan 30, 2019

Update AnnotationReader's metadata parser to use list of known ignored annotation #240

Closed

alcaeus modified the milestones: v1.7.0, v1.8.0 Sep 17, 2019

alcaeus mentioned this pull request Sep 17, 2019

@phpcsSuppress annotation is not being ignored. #277

Closed

phpeek mentioned this pull request Sep 24, 2019

Backporting adding global ignored annotations to DocParser. #281

Merged

alcaeus pushed a commit to alcaeus/annotations that referenced this pull request May 7, 2020

Mention coding standard requirements in the README

2e19a48

Relevant discussion: doctrine#235 (comment)

	} elseif ( ! isset($this->ignoredAnnotationNames[$name])
	&& isset($this->imports['__NAMESPACE__'])
	&& $this->classExists($this->imports['__NAMESPACE__'] . '\\' . $name)
	) {
	$name = $this->imports['__NAMESPACE__'].'\\'.$name;
	$found = true;
	} elseif (! isset($this->ignoredAnnotationNames[$name]) && $this->classExists($name)) {
	$found = true;
	}

Conversation

sanmai commented Jan 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Ocramius left a comment

Choose a reason for hiding this comment

Uh oh!

sanmai commented Jan 23, 2019

Uh oh!

sanmai commented Jan 23, 2019

Uh oh!

Uh oh!

Ocramius left a comment

Choose a reason for hiding this comment

Uh oh!

alcaeus left a comment

Choose a reason for hiding this comment

Uh oh!

lcobucci left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lcobucci Jan 24, 2019

Choose a reason for hiding this comment

Uh oh!

sanmai Jan 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lcobucci Jan 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sanmai Jan 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sanmai Jan 28, 2019

Choose a reason for hiding this comment

Uh oh!

greg0ire Jan 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sanmai Jan 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

greg0ire Jan 28, 2019

Choose a reason for hiding this comment

Uh oh!

sanmai Jan 29, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

lcobucci commented Jan 28, 2019

Uh oh!

sanmai commented Jan 29, 2019

Uh oh!

sanmai commented Jan 30, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

sanmai commented Jan 23, 2019 •

edited

Loading

lcobucci left a comment •

edited

Loading

sanmai Jan 24, 2019 •

edited

Loading

lcobucci Jan 24, 2019 •

edited

Loading

sanmai Jan 25, 2019 •

edited

Loading

greg0ire Jan 28, 2019 •

edited

Loading

sanmai Jan 28, 2019 •

edited

Loading