Skip to content

Conversation

jvasseur
Copy link
Contributor

Q A
Doc fix? yes
New docs? no
Applies to 2.3
Fixed tickets

We don't add phpdoc if it doesn't add any meaningful information, so i removed the line saying all methods should be documented. Maybe we should replace it by a phrase explaining what should be documented ?

@javiereguiluz
Copy link
Member

I agree with your second idea. Instead of removing the phrase, let's improve it:

Add PHPDoc blocks for all classes, methods, and functions, except when the comments are trivial (e.g. basic getter and setter methods). 
@jvasseur
Copy link
Contributor Author

Updated with the phrase you suggested.

@soullivaneuh
Copy link
Contributor

I'm 👎 for this for one reason:

except when the comments are trivial (e.g. basic getter and setter methods). 

This is not accurate at all. We should precise when the comment should be here, and when not.

For example, basic getter could / should have a phpdoc to precise the type. This is used by most of the IDE.

@wouterj
Copy link
Member

wouterj commented Feb 6, 2016

I think almost all methods (even simple getters and setters) are documented in Symfony core. I agree that it isn't enforced as strict as other standards, but I still think we should keep the standard as it is.

However, Symfony tends to seperate public API from internal methods/properties/classes more and more. Many methods that don't have any PHPdoc are internal methods (e.g. event listener methods). Maybe we should update this to "Add PHPDoc blocks for all classes, methods, and functions that are part of the public API;" What do you think?

@xabbuh
Copy link
Member

xabbuh commented Feb 7, 2016

Well, actually we indeed usually do reject adding "useless" PHP doc.

@TomasVotruba
Copy link

What would be really helpful and clear to ready would be some examples.

E.g.

Correct commenting

// type is obvious from typehint - no need for that to duplicate public function setPerson(Person $person) { // ... } // param is unknown scalar type - add new information /**  * @param int $count  */ public function setCount($count) { } // returns unknown scalar type - add new information /**  * @return bool  */ public function isReady() { // ... }

Incorrect commenting

Usecases you are talking about...

@javiereguiluz
Copy link
Member

What should we do with this proposal? I don't think we can put this in words with absolute precision because adding or not some PHPdoc is sometimes subjective. I'd merge it as is.

@weaverryan
Copy link
Member

I've merged this at sha: 241dfde and tweaked the wording at sha: 0aa25f8

Thanks!

@weaverryan weaverryan closed this Jul 20, 2017
@jvasseur jvasseur deleted the standards-update branch July 20, 2017 12:18
xabbuh added a commit that referenced this pull request Jul 20, 2017
* 2.7: Simplified the requirements article [#6030] Simplifying and showing code Clearify behaviour of Blank and NotBlank validator [#5838] Tweaking comment - the phpdoc policy is not concrete Update standards to match actual practices
xabbuh added a commit that referenced this pull request Jul 20, 2017
* 2.8: Simplified the requirements article [#6030] Simplifying and showing code Clearify behaviour of Blank and NotBlank validator [#5838] Tweaking comment - the phpdoc policy is not concrete Update standards to match actual practices Updated the Requirements article for Symfony 2.8
xabbuh added a commit that referenced this pull request Jul 20, 2017
* 3.2: [#8195] fix requirement checker binary Simplified the requirements article [#6030] Simplifying and showing code Clearify behaviour of Blank and NotBlank validator [#5838] Tweaking comment - the phpdoc policy is not concrete Update standards to match actual practices Updated the Requirements article for Symfony 3.2 Updated the Requirements article for Symfony 2.8
xabbuh added a commit that referenced this pull request Jul 20, 2017
* 3.3: [#8195] fix requirement checker binary Simplified the requirements article [#6030] Simplifying and showing code Clearify behaviour of Blank and NotBlank validator [#5838] Tweaking comment - the phpdoc policy is not concrete Update standards to match actual practices Updated the requirements article for Smyfony 3.3 Updated the Requirements article for Symfony 3.2 Updated the Requirements article for Symfony 2.8
xabbuh added a commit that referenced this pull request Jul 20, 2017
* 3.4: [#8195] fix requirement checker binary Simplified the requirements article [#6030] Simplifying and showing code Clearify behaviour of Blank and NotBlank validator [#5838] Tweaking comment - the phpdoc policy is not concrete Update standards to match actual practices Updated the Requirements article for Symfony 3.4 Updated the requirements article for Smyfony 3.3 Updated the Requirements article for Symfony 3.2 Updated the Requirements article for Symfony 2.8
weaverryan added a commit that referenced this pull request Jul 21, 2017
* origin/3.3: [#8195] fix requirement checker binary Simplified the requirements article [#6030] Simplifying and showing code Clearify behaviour of Blank and NotBlank validator [#5838] Tweaking comment - the phpdoc policy is not concrete Update standards to match actual practices Updated the requirements article for Smyfony 3.3 Updated the Requirements article for Symfony 3.2 Updated the Requirements article for Symfony 2.8 removed Charles Updated the Core Team information added CVE 2017-11365 added URL where to ask for a CVE identifier add missing choices_as_values options Update usage.rst
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment