Problem/Motivation

PSR-5 specifies annotations should use FQSEN and we have >400 covers annotations with a fully qualified path and 7 without. So either to be in line with the PSR or more importantly just to be consistent we should update these.

CommentFileSizeAuthor
#12 fix_coversdefaultclass-2626676-12-testonly.patch1.5 KBneclimdul
#12 fix_coversdefaultclass-2626676-12.patch8.27 KBneclimdul
coversdefault-fqsen.patch6.78 KBneclimdul

Comments

neclimdul created an issue. See original summary.

neclimdul’s picture

neclimdul
He/Him
commented
Priority: Normal » Minor

This is the minorist of minors. Doesn't affect documentation sites, ides, coverage reports, anything. Just fixing consistency.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

dawehner’s picture

dawehner
Primary language German
commented

Do you think this list is still uptodate?

neclimdul’s picture

neclimdul
He/Him
commented

Wow um... I wrote this? I have no memory of this place...

Seems unlikely it is up to date but I can investigate.

neclimdul’s picture

neclimdul
He/Him
commented

So, after review yes I don't see any additional covers related annotations that are off and these still apply.

There are probably hundreds of @return, @param and other annotations that are off or questionable but I think I left that out of scope because our requirements are unclear and there are varying reasons for the weirdness.

  • Some are because a use statement included the type though this is generally inconsistent(and not correct by any standard as far as I can tell)
  • Some I think are because of this unclear line in our coding standard "If only one data type is possible, just use its name."
  • These annotations fall under the requirements "Type" and so are covered by a provision of PSR-5 that is more lenient about their definition. "Thus this may be either a Fully Qualified Class Name (FQCN) or if present in a namespace a local name." I'd prefer if we required a FQSEN in this case but that's a separate standards issue.

Useful grep for finding at least some of these questionable annotations. 

git grep -E '@(param|uses|return|covers|coversDefaultClass) [A-Z][a-zA-Z0-9]+'
dawehner’s picture

dawehner
Primary language German
commented

Well yeah I agree for now its enough to fix just the instances of @coversDefaultClass

neclimdul’s picture

neclimdul
He/Him
commented

Are there any actions you can think of blocking this from being RTBC?

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Reviewed & tested by the community

Those changes look great, but yeah I was wondering whether we could file a CS standard or so.

neclimdul’s picture

neclimdul
He/Him
commented

Not sure if there's really a standard around annotation other then the way they currently are phpunit can't resolve or use them so they're broken.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work

We should adjust the \Drupal\Tests\Listeners\DrupalStandardsListener to test for this so we don't introduce regressions. This exists to ensure we don't break coverage reports...

neclimdul’s picture

neclimdul
He/Him
commented
StatusFileSize
new fix_coversdefaultclass-2626676-12.patch8.27 KB
new fix_coversdefaultclass-2626676-12-testonly.patch1.5 KB

So I dug into it a little and it looks like coverage works without the fully qualified namespace. So really this is just standardizing our usage and bringing our @uses usage in line with PSR-5.

Tests only is the interdiff.

neclimdul’s picture

neclimdul
He/Him
commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 12: fix_coversdefaultclass-2626676-12-testonly.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs work » Reviewed & tested by the community

Nice work!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed 6c73dc1 and pushed to 8.1.x and 8.2.x. Thanks!

I decided to commit the initial patch instead of the patch with the test. I was wrong to ask for the test because this would start failing contrib modules for no good reason. This check belongs in coding standards because it is about style and not correctness. And then modules can choose to opt in to the standard when they are ready.

  • alexpott committed c2cbf73 on 8.2.x 
    Issue #2626676 by neclimdul: Fix coversDefaultClass annotations to be...
dawehner’s picture

dawehner
Primary language German
commented

I decided to commit the initial patch instead of the patch with the test. I was wrong to ask for the test because this would start failing contrib modules for no good reason. This check belongs in coding standards because it is about style and not correctness. And then modules can choose to opt in to the standard when they are ready.

alex++++++

Status: Fixed » Closed (fixed)

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