Problem/Motivation

Core and contrib have been using inline variable type declarations in the wild. In the past there were several competing standards, but in recent months and years this has settled on one single standard which has gained widespread adoption (ref. original issue summary, and the discussion in the comments up to #40).

We need to document this standard.

Proposed resolution

This policy was approved, #86, and the following added to API documentation and comments coding standards page.

Add documentation to the API documentation and comments coding standards page, in a new section titled "Drupal standard for inline variable type declarations", to be placed underneath the existing section Drupal standards for in-line code comments.

The @var tag can document variables in addition to class properties. This can be used when you know more about the type of a variable than is self-evident from the code, such as when you load a specific entity type, or a specific service. In this case you can typehint inline, which improves Developer Experience and Tools Experience.

Syntax example:

/** @var \Drupal\node\NodeInterface $node */
$node = $this->entityTypeManager->getStorage('node')->load(123);

When documenting variables with the @var tag note that you should use the special /** */ delimiters with a double opening asterisk. Always use the fully qualified namespace of the class or interface, and include the name of the variable at the end.

Remaining tasks

User interface changes

None.

API changes

None, but new coding standards will be adopted.

Original report by @pfrenssen

There are two three standards for declaring variable types inline currently in use in core:

There are two standards for declaring local variable types inline currently in use in core, and a third standard that is under discussion as part of PSR-5. The Drupal project currently does not have a stated standard on how to document these variables; it should.

The three possible standards are:

  1. Using a single asterisk and putting the variable name before the type. From \Drupal\node\Tests\NodeTokenReplaceTest::testNodeTokenReplacement:
     /* @var $node \Drupal\node\NodeInterface */
    

    This format is supported by Eclipse, Netbeans, PHPStorm and Zend Studio. If /** is substituted, orif the order is reversed it does not work in Netbeans (not sure about the other IDEs) as of this time (July 2014).

  2. Using a double asterisk and putting the variable name after the type. From \Drupal\comment\CommentForm::submit():
      /** @var \Drupal\comment\CommentInterface $comment */
    

    This format is only supported by PHPStorm, as of July 2014.

  3. Using a double asterisk and @type instead of @var, and optionally followed by a description. This is not yet used in core, so here's an arbitrary example:
      /** @type \Drupal\comment\CommentInterface $comment The comment */
    

    This is the proposed standard for PSR5, which has not yet been finalized as of July 2014.

There is a difference of opinion on whether we should be pragmatic and adopt #1 because it actually works now, or #2/#3, because they are more correct and consistent. If we adopt #1, we will most likely need to revise this standard if #3 or something similar is adopted as PSR-5, after IDEs adapt. If we adopt #2 or #3, then people who use some IDEs will not be able to benefit from it.

CommentFileSizeAuthor
#73 coder-inline_var-9_1_x.txt58.23 KBArkener
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

drunken monkey’s picture

Issue summary: View changes

Much in favor of this, thanks for opening!

You had a small mistake in your first example though (wrong order after all).

heddn’s picture

Issue summary: View changes
pfrenssen’s picture

Issue summary: View changes
pfrenssen’s picture

Right! Found a better example :)

I also filed an issue to fix these throughout core: #2305641: Use the "standard" format for @var inline variable type declarations

heddn’s picture

sun’s picture

Issue tags: +Coding standards

Single-line doc comments for @var type IDE hinting should be proper T_DOC_COMMENT tokens (/**...*/).

I wasn't aware that there is a difference between IDEs, but PhpStorm is correct about only interpreting those (and not arbitrary T_COMMENTs in /*...*/).

PSR-5 defines it that way, too: https://github.com/phpDocumentor/fig-standards/blob/master/proposed/phpd...

tim.plunkett’s picture

The "PHPStorm only" version (proposed by PSR-5) actually makes more sense. It matches @param, where the typehint is in the middle and the variable name is at the end.

So I'd suggest adopting the PSR-5 version officially, and the IDEs can support it at their leisure.

pfrenssen’s picture

If this is a fig-standards proposal then I absolutely agree. I was looking for some documentation on this today but didn't find anything.

pfrenssen’s picture

Issue summary: View changes
pfrenssen’s picture

Issue summary: View changes
pfrenssen’s picture

Issue summary: View changes
heddn’s picture

Issue summary: View changes
sun’s picture

FYI: We shouldn't use/adopt @type at this point, because due to some unexpected incompatibility issues, it may be possible that PSR-5 will revert @type back to @var. The fate of @type is still under discussion.

jhodgdon’s picture

Title: Set a standard for @var inline variable type declarations » [policy] Set a standard for @var inline variable type declarations
Project: Documentation » Drupal core
Version: » 8.x-dev
Component: Missing documentation » documentation

This issue should not be in the Documentation project. We normally discuss coding standards in the Drupal Core project queue.

jhodgdon’s picture

Yeah, PSR-5 is not adopted yet. Do any IDEs support the proposed PSR-5 standard at this time? Because the I think the only reason we are even adding these @var lines in the middle of code is for IDEs, for type hinting as an aid to code writing, right?

So if option 1 currently in the issue summary is what most IDEs currently support, my opinion is that we should adopt option 1 for now.

Then, if in the future, IDEs move to supporting the eventual PSR-5 standard that is adopted, we can adopt that standard, and should be able to write a quick shell script to change all existing lines using our adopted standard to the new standard.

But adopting a standard that IDEs don't currently support is kind of pointless, if the only point of these comments is to help people out who are using IDEs anyway.

tim.plunkett’s picture

PHPStorm supports all three proposals in the OP.

When autogenerating, it uses #2.

sun’s picture

PSR-5 only gives orientation here, but is not the driving factor (yet).

To decipher the substantial factors:

Syntax

Inline variable type hints are PHPDoc. PHPDoc is notated in doc comments, not [multi-line] comments. Not only IDEs but also PHP code documentation generators are parsing all doc comments.

A doc comment is a native PHP language construct, defined as the T_DOC_COMMENT token by the Tokenizer core extension. A doc comment starts with /** and ends with */.

This is clearly defined in PHP for ages already. It's not clear why some IDEs are not aware of that, but if so, that's a bug in those IDEs. The only sensible syntax is:

/** ... */
$foo = Arbitrator::negotiate('id');
Tag

@var is universally understood by IDEs, because it has been the informal but de-facto standard for type-hinting class properties and inline variable type hints.

The current PSR-5 draft proposes to replace @var with @type, but that is not a safe bet yet, so @type is not an option for adoption at this point. Note that PSR-5 does not remove @var, its usage is only deprecated (again, in the current draft proposal).

Syntax + Tag:

/** @var ... */
$foo = Arbitrator::negotiate('id');
Tag details/signature

Inline variable @var type hints have the same purpose as @param and @return PHPDoc tags. Therefore, common sense dictates the same tag signature.

The @param signature is @param [type] [local-name].

Thus:

/** @var \Some\Class $foo */
$foo = Arbitrator::negotiate('id');

→ Variant 2), it is.

heddn’s picture

Issue summary: View changes
heddn’s picture

Issue summary: View changes
jhodgdon’s picture

Um... So sun you say "variant (2) it is", and I agree with your arguments.

However, from the issue summary, it appears that variant (2) is not supported by most IDEs. That doesn't seem great.

I agree that in the ideal world, these types of doc comments should probably be /** */. However, I also think that the main purpose of these particular doc comments is to provide hints to IDEs so that when you're coding, the methods etc. come up on those variables. And if most of the IDEs don't support the "correct" syntax, it is kind of pointless for us to enforce this syntax.

sun’s picture

Given that the primary goal of PSR-5 is to come up with a standard for exactly these kind of questions, and given that all major IDE vendors are tracking the PHP-FIG efforts (especially the progress of PSR-5), and given that variant 2) is in line with PSR-5, and given that variant 2) is used in most other PHP projects/packages, I'd say it's fair and easy to predict that IDE vendors who do not support variant 2) yet will update their software to be compatible with PSR-5 very shortly.

In the end, adopting the right policy for Drupal code is just one more incentive for IDEs to support it.

jhodgdon’s picture

And adopting a policy that IDEs do not currently support is also a recipe for making many Drupal developers, in the meantime, unhappy.

I personally do not use an IDE, so I don't care much about this at all... but... I think this deserves a little more thought on the "practical now" vs. "ideal" idea.

Crell’s picture

I am generally inclined to agree with jhodgon in #15: Go with what's widely supported for now, switch to PSR-5 as soon as feasible (with whatever PSR-5 ends up being on this point). Using the double ** in any case makes sense and should be supported by any IDE that matters.

I generally agree that following @param order makes more sense, but, meh. That's something to take up with the PSR-5 team. :-)

sun’s picture

Using the double ** in any case makes sense and should be supported by any IDE that matters.

Yes, that's variant (2), the most sensible option, and is 100% in line with PSR-5.

Others noted that the DocComment syntax (/**) is not recognized by some IDEs (not verified), except PhpStorm.

I'm not using an IDE either (yet), but to my knowledge, PhpStorm is the most widely used IDE for Drupal (8).

following @param order makes more sense, but, meh. That's something to take up with the PSR-5 team.

Again, the current PSR-5 draft is fully in line with variant (2).

@var is an alias for @type, which is defined as:

@type ["Type"] [element_name] [<description>]

→ PSR-5 @type === #17 | Variant (2)

jhodgdon’s picture

Can we get some definitive tests of #2 from people who actually use IDEs other than PHPStorm then, before we decide? I do not know what IDEs people in the Drupal community actually use. The issue summary mentions: Eclipse, Netbeans, Zend Studio. Are there others? Can people who use these please test #2 and report back here?

drunken monkey’s picture

Can we get some definitive tests of #2 from people who actually use IDEs other than PHPStorm then, before we decide? I do not know what IDEs people in the Drupal community actually use. The issue summary mentions: Eclipse, Netbeans, Zend Studio. Are there others? Can people who use these please test #2 and report back here?

The issue originated in #2286813-8: Test FieldsProcessorPluginBase, where pfrenssen confirms that only the exact syntax of (1) is recognized, at least in Netbeans. Eclipse and Zend Studio are probably the same, but that would still need to be confirmed.

In any case, I agree with jhodgon here, standardizing on something that all IDEs support should be the primary goal here, since anything else would just be shooting ourselves in the foot DX-wise. Once PSR-5 lands and is adopted, we should of course switch, but until then, let's use something that works.
(I, too, am using PhpStorm, and it does seem the most popular, but still we should aim to let as many developers as possible profit from these type hints, even if it means a bit more work for PhpStorm users.)

pfrenssen’s picture

I have tested this on Netbeans and Eclipse (with the PDT plugin). I also tried Geany but it doesn't support it at all.

Type declaration format Netbeans 8.0 Eclipse 4.4 PHPStorm
/* @var $node NodeInterface */ Yes Yes Yes
/** @var NodeInterface $node */ No No Yes
/** @type NodeInterface $node */ No No Yes

Edit: also added PHPStorm to the table, since people are reporting all cases work for this IDE.

Crell’s picture

Do NetBeans and Eclipse handle /** openings on the first option? Or are they really that picky?

pfrenssen’s picture

I don't have Eclipse installed on this machine, but at least in Netbeans 8.0 using a /** opening doesn't work. Also when a description is put after the type declaration it doesn't work any more. It has to be exactly in the format /* @var $variable Type */.

sun’s picture

Drupal is not the only piece of PHP code on this planet.

Did someone check whether there are existing bug reports for NetBeans and Eclipse?

Quickly searched and found:

NetBeans:

  1. https://netbeans.org/bugzilla/show_bug.cgi?id=146248 introduced @var code completion; users already started to raise syntax concerns back then.
  2. https://netbeans.org/bugzilla/show_bug.cgi?id=187317 complained.
  3. https://netbeans.org/bugzilla/show_bug.cgi?id=226308 complained again.
  4. https://netbeans.org/bugzilla/show_bug.cgi?id=164696 most recent bug report that fully matches #17. (reopen it?)

Eclipse:

  1. https://bugs.eclipse.org/bugs/show_bug.cgi?id=238505 introduced @var code completion.
  2. https://bugs.eclipse.org/bugs/show_bug.cgi?id=182693 complained.
  3. https://bugs.eclipse.org/bugs/show_bug.cgi?id=437792 currently open bug report that fully matches #17.

Status quo regarding both:

  1. Users did already complain that /** doesn't work.
  2. Previous bug reports did not always mention the reversed/inconsistent parameter order of inline @var tags.
  3. None of the bug reports supplied a convincing reasoning for why users expect it to work that way. Therefore, many of them were closed as invalid/works as designed.

I'm fairly sure that the issue can be addressed in both, especially if users of those IDEs would ensure that the corresponding bug reports have sufficient + solid background information, so as to give some more incentive to the IDE maintainer/vendor. Lastly, both issue trackers allow you to vote on issues.

jhodgdon’s picture

Issue summary: View changes

sun: Pursuing bug reports for NetBeans and Eclipse, or waiting for PSR-5 solidification/adoption in order to have more ammunition for doing this, sounds like a great idea -- but a long-term idea.

In the meantime, it sounds like since the Drupal project's main/only reason for adopting a standard for putting @var comments on local variables is for IDE hinting, we need to adopt proposal #1, since it is the only possibility that is supported by the current IDEs people use.

When IDEs are updated, we can then update our standard, and it should be relatively simple to do a regular expression search-and-replace to update our code for the new standard.

I have just updated the issue summary with this proposal, which needs review to be adopted.

markhalliwell’s picture

sun: Pursuing bug reports for NetBeans and Eclipse, or waiting for PSR-5 solidification/adoption in order to have more ammunition for doing this, sounds like a great idea -- but a long-term idea.

@sun is correct.

We must think long-term. Introducing code that, in effect, enables IDEs to not change/fix themselves is kind of akin to why IE stayed so stagnant over the years. If we constantly adopt policies for things that "currently work" we will have less progress in other areas. Granted, these areas not really under our purview, however its effects can often be felt for years by a decision that we introduce now (ie: IDEs will push back saying that "no one uses it").

Regardless, we have already gone down the PSR rabbit hole. To adopt coding documentation standards because IDEs haven't yet implemented them seems silly and wrong. We should be helping drive IDE development, not the other way around. I think it would be more beneficial if we actually move forward with [the current] PSR-5 recommendation. If/when PSR standards change then we can do what is suggested and update our code via regex find and replace.

jhodgdon’s picture

Issue summary: View changes

OK, it sounds like we have a major difference of opinion here:

Camp Pragmatic: Adopt a current standard that will work in current IDEs, and plan to revise the standard when PSR-5 is actually adopted and IDEs have adapted.

Camp Principled: Adopt a standard that is "Correct", but will not currently work in IDEs, in an effort to provoke them to fix their broken IDEs.

I do not think we are going to get community agreement on either strategy, based on the current discussion. It is pointless to have some of us saying "we need to be pragmatic" and others saying "we need to be principled". That is not going to get us to agreement, repeating this "We should do it this way" over and over.

Therefore I propose that we continue to let people put these @var comments into code for their own convenience, whichever way works for the person who is actually trying to use their IDE to write code, without adopting a standard.

I therefore think we should mark this "postponed" until PSR-5 is adopted and IDEs adapt.

tim.plunkett’s picture

but will not currently work in IDEs

You mean "but will not currently work in IDEs except the single most popular IDE among D8 developers which happens to do the right thing and Just Works™".

I'm not okay with picking a *wrong* standard because of Eclipse/Netbeans.

Therefore I propose that we continue to let people put these @var comments into code for their own convenience, whichever way works for the person who is actually trying to use their IDE to write code, without adopting a standard.

If that's our new approach to coding standards, then great. We should add a section saying "anything not covered in coding standards, do whatever you want!"...

jhodgdon’s picture

I am sorry, I did not realize that one IDE was more popular than the others...

Anyway, I am not going to say any more. If the community agrees on something, I will of course go along with it -- it won't be the first time I didn't agree with a standards decision. :)

sun’s picture

Yes, what I tried to express was: We should adopt a policy that is syntactically correct and future-proof.

  1. PhpStorm gets it right. It is the most popular IDE for D8 right now. >80% of IDE users will be happy with variant (2).
  2. Eclipse has an open ticket to address the issue, and they seem to be positive and willing to fix it. Just a matter of time/popularity/pressure.
  3. NetBeans… PHP product maintainer seems to be stubborn about his own invention of "VARDoc" (though, granted, most comments are from ~2009). Convincing this maintainer is a matter of copy/pasting #17 into a bug report, because there's little that can be argued against it. Just a matter of will (and end-users that do actually care about this IDE).

If there's just one thing that PHP-FIG has taught us: We're not living in a silo, we can communicate with each other, upstream, side-stream, down-stream, and we can work together to change the environment. Just do it.

Let's move forward with variant (2).


We don't need to wait for PSR-5, because variant (2) is valid in PSR-5. If PSR-5 would be released in its current form, then it would only contain a recommendation to use @type in favor of @var, so @var is still valid, just discouraged. And, once more, there's a good chance that @type may be removed from PSR-5. Hence, variant (2).

sun’s picture

FYI, PSR-5 is about to remove @type altogether and revert it back to @var:
https://github.com/phpDocumentor/fig-standards/pull/55

jhodgdon’s picture

It would be nice if that had been at least pointed out or discussed on the PHP-FIG mailing list. :(

jhodgdon’s picture

Project: Drupal core » Drupal Technical Working Group
Version: 8.0.x-dev »
Component: documentation » Code

Coding standards changes are now part of the TWG

pwolanin’s picture

Seems that PR for psr-5 was merged and it's using:

/** @var \Sqlite3 $sqlite */
tizzo’s picture

Project: Drupal Technical Working Group » Coding Standards
Component: Code » Coding Standards
ciss’s picture

Eclipse seems to support variant 2 as of PDT 3.6.0 (https://bugs.eclipse.org/bugs/show_bug.cgi?id=474332). Will that be enough to move this issue forward?

pfrenssen’s picture

Well the PSR-5 adoption makes it kind of official. I guess we just need to write the text now.

dawehner’s picture

Here is one suggestion:

On top of documentation class properties @var can be also used to document normal variables. This can be used when you can assume more about the types, than what the computer can know about it. In this case you can typehint inline, which improves Developer Experience and Tools Experience. Here is an example:

/** @var \Drupal\node\NodeInterface */
$node = $this->entityStorage->load(123);
pfrenssen’s picture

Great start! Two suggestions:

* The "$node" is missing from the example.
* I would say "In addition to documenting class properties" instead of "On top of documentation class properties".

jmuzz’s picture

+1 for variant 2 and @pfrenssen's suggestions.

The text could be more concise and consistent with the rest of the coding standards documentation. The PSR mentions an additional MUST that could be included too. Possibly:

The @var tag can document variables in addition to class properties. You can use this when you know more about the types than the computer does. In this case you can typehint inline, which improves Developer Experience and Tools Experience.

Syntax example:

/** @var \Drupal\node\NodeInterface $node */
$node = $this->entityStorage->load(123);

When using the @var tag this way it MUST contain the name of the variable it documents.

I tried to remove the passive voice too. I don't know if that's correct since "The ___ tag is used..." appears many times in the documentation, but the content style guide does recommend against the passive voice.

dawehner’s picture

Status: Active » Needs review

@jmuzz
This works for me.

This is one of those standards which are used widely across the board in core, contrib and custom world, so documentation it would just phase the reality.

jhodgdon’s picture

At the moment, anyone who wants to comment on this issue will need to read most of the 47 comments to figure out what is going on.

dawehner’s picture

Issue summary: View changes
Issue tags: -Needs issue summary update

I copied the current proposal to the issue summary ...

jhodgdon’s picture

Has anyone tested IDEs more recently than 2014 to see if the recommended proposal works in them? In the issue summary, it doesn't look like it's very well supported yet.

pfrenssen’s picture

Issue summary: View changes
pfrenssen’s picture

Issue summary: View changes

This issue was also discussed at today's meeting of the Coding Standards Committee, and we think this is ready to mark for final discussion.

Addressing the latest comments, I have installed Netbeans 8.2 and tested it and the recommended standard now works fine in it. It was already reported by @ciss in comment #42 that Eclipse also supports it since 3.6.0. So the 3 most important IDE's all support it now. In addition, this is adopted by PSR-5 and there is broad approval by the community. Finally, there are currently 1205 instances in core alone that use this pattern, so it's safe to say it is already the de facto standard.

I propose a small change to the current wording. In general we try to avoid the formal language that is used in the PSR proposals, most specifically the pattern of using capitalized words like "MUST" and "SHOULD NOT". There have been some cases in the past where this language has crept into our coding standards, but these are very rare. Overall our coding standards are written in a very friendly and conversational manner, and these "shouting words" are at odds with that.

So I propose to not use MUST, and also to mention that the fully qualified namespace should be used, and draw attention to the special format with the double asterisk.

before:

When using the @var tag this way it MUST contain the name of the variable it documents.

after:

When documenting variables with the @var tag note that you should use the special /** */ delimiters with a double opening asterisk. Always use the fully qualified namespace of the class or interface, and include the name of the variable at the end.

I'm also not entirely sure about "knowing more than the computer does" and "Developer Experience and Tools Experience", maybe it's better to simply mention that this helps with IDE integration and autocompletion? Does anyone have a good suggestion for this?

pwolanin’s picture

+1 from me to getting this settled soon.

re: language:

"when you know more about the types than the computer does"

I agree that's not perfect. Maybe more like:

"when you know more about the type of a variable than is self-evident from the code, such as when you load a specific entity type, or a specific service."

pfrenssen’s picture

Issue summary: View changes

That sounds really good to me. I'll update the proposed language.

Maybe it would also be good to actually provide an example where the variable type is definitely ambiguous? The current code example actually implies that $this->entityStorage is an instance of NodeStorage and, depending on how that is populated, it might be known by the IDE.

Maybe something like this?

/** @var \Drupal\node\NodeInterface $node */
$node = $this->entityTypeManager->getStorage('node')->load(123);
pfrenssen’s picture

Issue summary: View changes
pwolanin’s picture

Yes, that example seems a little more evident

pfrenssen’s picture

Issue summary: View changes

Thanks for the feedback! Updating the example.

tizzo’s picture

tizzo’s picture

Project: Coding Standards » Drupal core
Version: » 8.3.x-dev
Component: Coding Standards » documentation
Issue tags: -final discussion +Approved Coding Standard, +needs documentation updates, +d8dx

This proposal was approved during the December 20, 2016 coding standards committee meeting.

Sending to core for approval and implementation as there may be some non-compliant instances of variable docblock comments in core.

tizzo’s picture

Status: Needs review » Reviewed & tested by the community

Oops! Forgot to RTBC!

xjm’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs issue summary update

To approve this standard, I think we need some more information on #2305641: Use the "standard" format for @var inline variable type declarations. Can we get documentation of the coder check to run and a summary of the existing violations in core?

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

andrewmacpherson’s picture

Stumbled on this, because I didn't know whether there was a standard for it yet. I was curious how many instances of /* @var $name \Type */ we have versus /* @var \Type $name */...

$> ack -h '\* \@var \$' core | wc 
    114     570    7463
$> ack -h '\* \@var \\' core | wc 
   5876   20688  354268

Turns out the existing D8 codebase overwhelming says the type first, which agrees with what is proposed here and in the PSR5 draft.

(As of 66f7227 on 8.6.x branch)

donquixote’s picture

If I understand correctly, the current proposal assumes these are always single-line docs, without any description text.

I would propose to also allow multi-line docs, if a description would add clarity:

/**
 * @var array[][][] $instancesss
 *   Format: $[$entity_type][$bundle][$field_name] = $field_instance
 */
$instances = [];

In cases where a description is not needed, a one-line doc can be used instead.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

lawxen’s picture

My Phpstorm version is newest 2019.1.3
It can't support auto complete of /* @var
But support /**
If I typed in /** and a blank space, The Phpstrom can auto help me complete to:
/** @var $currency_repository */

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

xjm’s picture

Restoring tag.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Arkener’s picture

FileSize
58.23 KB

Created an issue on the Coder project to add a rule for this standard #3163106: Add sniff for @var inline variable type declarations. This rule currently checks if the inline var type declaration is defined within a T_DOC_COMMENT and if the variable name is placed after the type declaration. This rule currently reports 126 violations on the 9.1.x branch.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

quietone’s picture

Work on this seems to have gotten out of order. There is an issue to include this standard in core, #3207734: Fix Drupal.Commenting.InlineVariableComment, which shows all the existing violations. The coder fix was committed and available for your reading pleasure.

xjm tagged this for an issue summary update but I can't figure out what needs to be changed. Nor do I understand what more information is needed on #2305641: Use the "standard" format for @var inline variable type declarations.

I do know that the docs need to be updated.

What is the next step here?

quietone’s picture

Issue summary: View changes
Issue tags: -Needs issue summary update

Spoke with Spokje in Slack, who pointed out what I missed in the IS. The remaining tasks are updated.

It should be said that the 'standard' here has already become the de-facto standard in core.

We propose that the following happens. Close #2305641: Use the "standard" format for @var inline variable type declarations as outdated. Update documentation in #2305593: [policy] Set a standard for @var inline variable type declarations which un-postpones #3207734: Fix Drupal.Commenting.InlineVariableComment, which was RTBC. That all depends on getting the approval that xjm says is needed in #61 even though it tagged as approved in #58.. Rather confusing.

xjm credited Spokje.

xjm’s picture

@quietone Yeah, it is a somewhat confusing/convoluted process. The process is documented on the project page: https://www.drupal.org/project/coding_standards/

In that process, after the coding standards working group signs off, committers also have to review and sign off on the change. And part of a committer's job is making sure that a change is not too disruptive, that the documentation gate is applied, etc. Back in Jan. 2017 we didn't have enough info on the issue to review it.

For what it's worth, based on the issues @Spokje and @quietone linked, and all the comments about IDE support since 2017, I've verified that we have a rule to enforce the standard, and that it only changes 170 lines so it is not too disruptive to the core codebase. So we just need to wait on documentation of the standard.

Thanks!

quietone’s picture

Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs documentation updates

@xjm, thank you for the quick response and approval! I have read, well skimmed, the coding standard project page a couple of times. I think convoluted is a good word for it and it will have to learn it by experience.

I have done ahead and added the recommended text, with minor format changes, to Drupal standards for in-line code comments. I am tagging this NR for someone to check those changes.Then this can be marked fixed.

Spokje’s picture

Status: Needs review » Reviewed & tested by the community

Currently the documentation page shows this for the @var inline comment:

The @var tag can document variables in addition to class properties. This can be used when you know more about the type of a variable than is self-evident from the code, such as when you load a specific entity type, or a specific service. In this case you can typehint inline, which improves Developer Experience and Tools Experience. For example:

/** @var \Drupal\node\NodeInterface $node */
$node = $this->entityTypeManager->getStorage('node')->load(123);

When documenting variables with the @var tag note that you should use the special /** */ delimiters with a double opening asterisk. Always use the fully qualified namespace of the class or interface, and include the name of the variable at the end.

This is completely inline with the sniff 'Drupal.Commenting.InlineVariableComment' used in Coder.

RTBC for me.

Spokje’s picture

Status: Reviewed & tested by the community » Fixed

Since there's nothing to commit here, and we have the blessing of @xjm as a Core Committer, I feel pretty save marking this issue as Fixed.

pfrenssen’s picture

Thanks!

donquixote’s picture

For people coming from a web search (and for me), can we summarize (in the issue summary?) what was done in the issue, and where people would find the up-to-date convention?

I see that:

  • The convention is already described in https://www.drupal.org/docs/develop/standards/api-documentation-and-comm....
  • The convention we agreed on is /** @var TYPE $varname */, so the same as described in "Proposed resolution".
  • There is no written convention yet for multi-line inline @var docs, e.g. to document multiple variables in one block, or to add description text. Perhaps this needs a new issue? -> See my comment #66.

But to me this conclusion is not immediately obvious when looking at the issue summary. I see the proposed resolution, but how would I know this is also what was agreed on?

quietone’s picture

Issue summary: View changes

Added details of when this was approved to the IS in the proposed resolution,

Spokje’s picture

Issue summary: View changes

Thanks for the feedback @donquixote

Updated Remaining tasks, is this how you would like to see it?

The only thing that remains from #83 is to open a follow-up to discuss multi-line inline @var comments as proposed in #66.

Running out of time at the moment, so putting this on Needs Work until I/somebody opens up that follow-up.

Until then I don't think this is a blocker for actually turning the (one-line-only) sniff on in #3207734: Fix Drupal.Commenting.InlineVariableComment.
Please correct me if I'm wrong on that one.

Spokje’s picture

Issue summary: View changes
Status: Fixed » Needs work

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

quietone’s picture

Issue summary: View changes
Status: Needs work » Fixed

Followup created #3250306: Allow multi-line @var inline variable type declarations

That is the final task here, so return status to fixed.

Status: Fixed » Closed (fixed)

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

angelgarciaco’s picture

Excellent info what you have shared. Thanks.