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
Agree on the proposed standard to adopt, if possibleAdd documentation to the API documentation and comments coding standards page.The proposed documentation from this Issue Summary was added.Review the documentation addition to the API documentation and comments coding standards page. The above change was reviewed.Make a patch for code that isn't using the adopted standard (on related issue #3207734: Fix Drupal.Commenting.InlineVariableComment).Add this standard to Coder.Sniff `Drupal.Commenting.InlineVariableComment` was added to Coder in #3163106: Add sniff for @var inline variable type declarationsOpen a follow-up to discuss multi-line inline@var
comments as proposed in #66.
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:
- 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).
- 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.
- 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.
Comment | File | Size | Author |
---|---|---|---|
#73 | coder-inline_var-9_1_x.txt | 58.23 KB | Arkener |
Comments
Comment #1
drunken monkeyMuch in favor of this, thanks for opening!
You had a small mistake in your first example though (wrong order after all).
Comment #2
heddnComment #3
pfrenssenComment #4
pfrenssenRight! 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
Comment #5
heddnComment #6
sunSingle-line doc comments for
@var
type IDE hinting should be properT_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_COMMENT
s in/*...*/
).PSR-5 defines it that way, too: https://github.com/phpDocumentor/fig-standards/blob/master/proposed/phpd...
Comment #7
tim.plunkettThe "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.
Comment #8
pfrenssenIf this is a fig-standards proposal then I absolutely agree. I was looking for some documentation on this today but didn't find anything.
Comment #9
pfrenssenComment #10
pfrenssenComment #11
pfrenssenComment #12
heddnComment #13
sunFYI: 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.Comment #14
jhodgdonThis issue should not be in the Documentation project. We normally discuss coding standards in the Drupal Core project queue.
Comment #15
jhodgdonYeah, 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.
Comment #16
tim.plunkettPHPStorm supports all three proposals in the OP.
When autogenerating, it uses #2.
Comment #17
sunPSR-5 only gives orientation here, but is not the driving factor (yet).
To decipher the substantial factors:
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:
@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:
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:
→ Variant 2), it is.
Comment #18
heddnComment #19
heddnComment #20
jhodgdonUm... 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.
Comment #21
sunGiven 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.
Comment #22
jhodgdonAnd 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.
Comment #23
Crell CreditAttribution: Crell commentedI 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. :-)
Comment #24
sunYes, 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).
Again, the current PSR-5 draft is fully in line with variant (2).
@var is an alias for @type, which is defined as:
→ PSR-5
@type
=== #17 | Variant (2)Comment #25
jhodgdonCan 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?
Comment #26
drunken monkeyThe 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.)
Comment #27
pfrenssenI have tested this on Netbeans and Eclipse (with the PDT plugin). I also tried Geany but it doesn't support it at all.
/* @var $node NodeInterface */
/** @var NodeInterface $node */
/** @type NodeInterface $node */
Edit: also added PHPStorm to the table, since people are reporting all cases work for this IDE.
Comment #28
Crell CreditAttribution: Crell commentedDo NetBeans and Eclipse handle /** openings on the first option? Or are they really that picky?
Comment #29
pfrenssenI 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 */
.Comment #30
sunDrupal 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:
Eclipse:
Status quo regarding both:
/**
doesn't work.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.
Comment #31
jhodgdonsun: 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.
Comment #32
markhalliwell@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.
Comment #33
jhodgdonOK, 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.
Comment #34
tim.plunkettYou 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.
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!"...
Comment #35
jhodgdonI 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. :)
Comment #36
sunYes, what I tried to express was: We should adopt a policy that is syntactically correct and future-proof.
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).Comment #37
sunFYI, PSR-5 is about to remove @type altogether and revert it back to @var:
https://github.com/phpDocumentor/fig-standards/pull/55
Comment #38
jhodgdonIt would be nice if that had been at least pointed out or discussed on the PHP-FIG mailing list. :(
Comment #39
jhodgdonCoding standards changes are now part of the TWG
Comment #40
pwolanin CreditAttribution: pwolanin commentedSeems that PR for psr-5 was merged and it's using:
Comment #41
tizzo CreditAttribution: tizzo commentedComment #42
ciss CreditAttribution: ciss as a volunteer commentedEclipse 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?
Comment #43
pfrenssenWell the PSR-5 adoption makes it kind of official. I guess we just need to write the text now.
Comment #44
dawehnerHere is one suggestion:
Comment #45
pfrenssenGreat 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".
Comment #46
jmuzz CreditAttribution: jmuzz commented+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:
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.
Comment #47
dawehner@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.
Comment #48
jhodgdonAt 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.
Comment #49
dawehnerI copied the current proposal to the issue summary ...
Comment #50
jhodgdonHas 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.
Comment #51
pfrenssenComment #52
pfrenssenThis 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:
after:
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?
Comment #53
pwolanin CreditAttribution: pwolanin as a volunteer and at SciShield commented+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."
Comment #54
pfrenssenThat 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 ofNodeStorage
and, depending on how that is populated, it might be known by the IDE.Maybe something like this?
Comment #55
pfrenssenComment #56
pwolanin CreditAttribution: pwolanin as a volunteer and at SciShield commentedYes, that example seems a little more evident
Comment #57
pfrenssenThanks for the feedback! Updating the example.
Comment #58
tizzo CreditAttribution: tizzo commentedThis was announced for final discussion on December 6, 2016.
Comment #59
tizzo CreditAttribution: tizzo commentedThis 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.
Comment #60
tizzo CreditAttribution: tizzo commentedOops! Forgot to RTBC!
Comment #61
xjmTo 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?
Comment #65
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commentedStumbled 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 */
...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)
Comment #66
donquixote CreditAttribution: donquixote commentedIf 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:
In cases where a description is not needed, a one-line doc can be used instead.
Comment #69
lawxen CreditAttribution: lawxen commentedMy 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 */
Comment #71
xjmRestoring tag.
Comment #73
Arkener CreditAttribution: Arkener as a volunteer commentedCreated 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.Comment #75
quietone CreditAttribution: quietone as a volunteer commentedWork 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?
Comment #76
quietone CreditAttribution: quietone as a volunteer commentedSpoke 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.
Comment #78
xjm@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!
Comment #79
quietone CreditAttribution: quietone as a volunteer commented@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.
Comment #80
SpokjeCurrently the documentation page shows this for the
@var
inline comment:This is completely inline with the sniff 'Drupal.Commenting.InlineVariableComment' used in Coder.
RTBC for me.
Comment #81
SpokjeSince 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.
Comment #82
pfrenssenThanks!
Comment #83
donquixote CreditAttribution: donquixote commentedFor 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:
/** @var TYPE $varname */
, so the same as described in "Proposed resolution".@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?
Comment #84
quietone CreditAttribution: quietone as a volunteer commentedAdded details of when this was approved to the IS in the proposed resolution,
Comment #85
SpokjeThanks 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.
Comment #86
SpokjeComment #88
quietone CreditAttribution: quietone as a volunteer commentedFollowup created #3250306: Allow multi-line @var inline variable type declarations
That is the final task here, so return status to fixed.
Comment #90
angelgarciaco CreditAttribution: angelgarciaco commentedExcellent info what you have shared. Thanks.