Problem/Motivation

For a variety of reasons, @param and @return documentation should include the data type. Although this is mentioned in coding standards, it is not mandated. This issue started with a patch to add data types to all the @param and @return lines in the includes/file.inc file, but it has subsequently morphed into a discussion of the question, "Should all @param and @return data types be documented?"

In favor of the standards change:
  1. chx
  2. Lars Toomre
  3. webchick
  4. Crell
  5. catch
  6. pillarsdotnet
  7. sun
  8. mikey_p
  9. xjm
  10. sdboyer
  11. boombatower
  12. DjebbZ
  13. plach
  14. rafamd
Opposed to the standards change:
  1. Dave Reid
  2. joachim
Thinks the standards change is desirable but impractical:

Note that the above was gathered by inference; Feel free to revise if you know differently.

Proposed resolution

The proposed patch adds parameter and return data type documentation to the includes/file.inc file.

Remaining tasks

  1. The patch in this issue needs to be reviewed, approved, and committed.
  2. Similar patches should be generated for the rest of Drupal core.
  3. The Documenting functions and methods documentation should be rewritten to reflect that:
    • Type hinting in @param and @return lines is now required rather than merely allowed.
    • The mixed type-hint is now deprecated and should be replaced with the allowed types separated by the vertical-bar character, as string|false|null.
    • Using bool is preferred over boolean, according to webchick and PEAR.
    • Likewise, int is preferred over integer.
  4. A patch should be written to create a D8 version of Coder that warns if such type-hinting is missing or mixed.

User interface changes

None.

API changes

None; this is a documentation change only. Adding data types to actual function arguments should be considered in separate issues.

Original issue

Per jhodgdon in #1201024-32: drupal_realpath() should describe when it should be used:

maybe rewrite it as:

@param string $uri

And in #35:

Having data types in @param statements is a relatively new doc standard. There is no real reason why it shouldn't be adopted here, just because it hasn't been adopted everywhere else yet.

Files: 
CommentFileSizeAuthor
#43 includes--file_inc-add-type-hint-docs-1290258-43.patch34.22 KBpillarsdotnet
#1 data-type-docs-1290258-1.patch33.74 KBpillarsdotnet
PASSED: [[SimpleTest]]: [MySQL] 33,058 pass(es).
[ View ]

Comments

pillarsdotnet’s picture

Title:Functions in includes/file.inc should specify data types in their @param documentation.» Functions in includes/file.inc should specify data types in their @param and @return documentation.
Status:Active» Needs review
StatusFileSize
new33.74 KB
PASSED: [[SimpleTest]]: [MySQL] 33,058 pass(es).
[ View ]

Patch adds data types to @param and @return docs, ensures a blank line before @return, and corrects a few other errors that I saw along the way:

Fix incomplete sentence and wrap at 80 characters:
  *   Returns a new stream wrapper object appropriate for the given $scheme.
- *   For example, for the public scheme a stream wrapper object
- *   (DrupalPublicStreamWrapper).
- *   FALSE is returned if no registered handler could be found.
+ *   For example, for the public scheme a DrupalPublicStreamWrapper stream
+ *   wrapper object is returned. FALSE is returned if no registered handler
+ *   could be found.
  */
Perl has references; PHP has variables:
- * @param $directory
- *   A string reference containing the name of a directory path or URI. A
+ * @param string $directory
+ *   A string variable containing the name of a directory path or URI. A
Specify both possible return values, for clarity:
- * @return
- *   TRUE if the URI is allowed.
+ * @return boolean
+ *   TRUE if the URI is allowed; FALSE otherwise.
Reference the correct manual page in the user's preferred language:
- * @param $context
- *   Refer to http://php.net/manual/en/ref.stream.php
+ * @param resource $context
+ *   Refer to http://php.net/manual/stream.contexts.php
pillarsdotnet’s picture

Issue tags:+Novice

Tagging Novice because this will likely need a few re-rolls as conflicting patches get committed, and re-rolling docs-only patches is tedious but not technically difficult.

jhodgdon’s picture

Status:Needs review» Postponed

We are planning to wait on patches of this size (that affect everything in a file and would make many patches need rerolls) until the sprint the week of November 1st, after the big patch that will move all the files into core.

pillarsdotnet’s picture

@jhodgdon

patches of this size (that affect everything in a file and would make many patches need rerolls)

To avoid delay, I could submit a separate patch for each function header. That's more work for both of us, but I'm sure we're all willing to make sacrifices to speed development, right?

jhodgdon’s picture

That is not really an improvement to have 50 separate issues (or however many there are) -- it would still make all patches related to file.inc need a reroll.

Please just wait until November. We do plan to have a big "fix up the docs" sprint the first week.

pillarsdotnet’s picture

We do plan to have a big "fix up the docs" sprint the first week.

It would be nice if drupal.org (or groups.drupal.org) featured an event calendar that listed such issue-postponing events, so those of us who are results-oriented can avoid working on issues before their appointed time.

Whoah! An event calendar? On a CMS support website? What a novel concept!

jhodgdon’s picture

There is an event calendar on gdo. I just haven't posted this event to it. Planning to post to gdo in the documentation-team group in mid-October.

pillarsdotnet’s picture

Link? I searched and didn't find it. Probably used the wrong search terms.

jhodgdon’s picture

See #7 "I haven't posted...yet".

pillarsdotnet’s picture

(sigh) I was asking for a link to the event calendar. Nevermind; I'm sure I can find it eventually.

jhodgdon’s picture

Oh, sorry! Just go to groups.drupal.org and click on "Events" in the top nav. To add an event, go to the group you want to add it to, and add new content of type "Event". :)

pillarsdotnet’s picture

catch’s picture

Status:Postponed» Needs review

While I think it'd be great to try to schedule patches like this for that rough period, it'd also be good to get more reviews on this one. For example does api.drupal.org handle this properly?

pillarsdotnet’s picture

@catch:

Sorry; didn't mean to mislead you. In this case, "postponed" doesn't mean, "Looks good; let's commit it later." The patch has never been in "RTBC" status and is not approved for a pre-scheduled commit. In this case, "postponed" means that Jennifer won't even consider it for review until after November 1st.

However, if someone else (like yourself) wants to start reviewing patches in the documentation component, I certainly won't complain.

I'm fairly sure that api.drupal.org does handle typed parameters. They're mentioned (but not mandated) by Coding Standards, and already used in a fair number of Drupal core functions:

bobvin@bowie:~/www/8.x$ egrep -r '@param [^$]' includes modules | wc -l
305
catch’s picture

Sorry I should've clarified more as well.

I think it makes tonnes of sense to try to get patches like this in around the week or two after November 1st - that way the issue queue massacre happens all at once.

However any patch that should go in for that window really needs to be reviewed in advance - since if it starts getting reviewed around November 1st it could end up not being ready until the end of that month, at which point people will already be trying to re-roll patches again.

I think there's a reason (not necessarily a good reason but some kind of reason) why we don't have data types in @params but I can't remember what it is. This is the sort of issue that might be worth cross posting in the coding standards group and or g.d.o/core to get some more reviews.

catch’s picture

And... that doesn't mean the patch needs to be re-rolled, but we should agree the coding standards change or not (or maybe we're just breaking the coding standard everywhere already, I didn't look but this information is missing in the issue ;).

pillarsdotnet’s picture

The relevant part of the current Coding standards says:

If the data type of a parameter or return value is not obvious or expected to be of a special class or interface, it is recommended to specify the data type in the @param or @return directive:

(example code deleted for brevity)

Primitive data types, such as int or string, do not need to be specified. It is recommended to specify classes and interfaces. If the parameter or return value is an array, or (anonymous/generic) object, you can specify the type if it would add clarity to the documentation. If for any reason, a primitive data type needs to be specified, use the lower-case data type name, e.g. "array". Also, make sure to use the most general class/interface possible. E.g., document the return value of db_select() to be a SelectQueryInterface, not a particular class that implements SelectQuery.

In the issue referenced by the summary, Jennifer was suggesting that I change @param $uri to @param string $uri and I balked because the change would be inconsistent with just about every other function in that file.

Then I posted this issue mainly to demonstrate that if we're going to start documenting parameter types, we should do it consistently and all at once rather than function-by-function as the mood strikes.

I do agree that if we set this precedent, the Coding Standards should be updated to reflect the policy change.

On the other hand, feel free to close (won't fix) this if you feel that the standard should not change.

catch’s picture

OK so to me it seems sensible just to document all data types of parameters. We often end up saying int/string/bool in the description anyway and could drop those. But I'd like to see more people chime in on this issue either way.

pillarsdotnet’s picture

Posted: Should all @param data types be documented?

Feel free to crosspost to g.d.o/core if you're a member; I'm not.

Crell’s picture

Very +1 on documenting all return and parameter types, unconditionally. Doxygen is the only major PHP documentation format I know of that does not expect it. All IDEs I've used provide them in their template auto-generated docblocks. It's useful information for anyone trying to understand an API, and also, I believe, can help us encourage putting a useful description in the "description" part of the docblock, rather than just "an int" or "an id", which is a largely useless docblock.

I think some parts of the database layer already do that, but I'm not certain.

We already verified in another issue that NetBeans, at least, and I presume Eclipse, is surprisingly smart about figuring out what to do with parameter and return types, even multiple-choice ones like int|SomeClass.

Once again, huge +1 here.

Lars Toomre’s picture

I am a strong believer that all @param data types should be documented.

Since this patch will apply to both D8 and D7, shouldn't this patch (and all other documentation patches like it) be applied (when RTBC) before the great divergence due to the moving to /core file structure patch gets applied?

Should I bother reviewing this patch now or wait until it is re-rolled after November 1st?

boombatower’s picture

Yea, I used to document it, but ended up trying to fit the standard...completely agree with documenting them all. What catch said in #18 is true as well and more reason to do so.

DjebbZ’s picture

It's obvious that all @param should be documented. It's faster to look at one place to know precisely what data types are needed. One undocumented param maybe obvious and easy to guess for someone and not for someone else. Good docs are exhaustive.
I already started to do this in my code and noticed my IDE (NetBeans 7) didn't know what to do with a data type in @return though (it was fine with @param).

pillarsdotnet’s picture

Issue tags:-Novice

Removing tag which obviously no longer applies.

Lars Toomre’s picture

I have gone through and reviewed the patch and the doc changes all look appropriate.

I would mark this RBTC except for confusion on my part about when to use '@return mixed' vis-a-vis say '@return string|boolean'. I thought the latter example is preferred when there are only two possible return types so that IDEs knew what to expect. Any guidance on when to use each @return alternative?

Lars Toomre’s picture

Issue summary:View changes

Update status.

pillarsdotnet’s picture

Title:Functions in includes/file.inc should specify data types in their @param and @return documentation.» The data type should be explicitly stated for all Drupal core @param and @return documentation.

Updated summary.

webchick’s picture

Title:The data type should be explicitly stated for all Drupal core @param and @return documentation.» Functions in includes/file.inc should specify data types in their @param and @return documentation.

catch implied there might be some ambiguity around this point, so I'm here to clarify. This patch, despite its unquestionable awesomeness, is not the kind of thing we can backport to D7 since it's in effect changing the coding standards. Since 7.0 didn't enforce a rule of specifying data types, we can't introduce such a rule in 7.9.

Huge +1 for D8 though!

webchick’s picture

Title:Functions in includes/file.inc should specify data types in their @param and @return documentation.» The data type should be explicitly stated for all Drupal core @param and @return documentation.

Sorry. Unintentional cross-post.

webchick’s picture

Issue summary:View changes

Update summary.

webchick’s picture

Also, tagging as DX. Having these datatypes will be super helpful.

pillarsdotnet’s picture

@webchick

This patch, despite its unquestionable awesomeness, is not the kind of thing we can backport to D7 since it's in effect changing the coding standards.

So the Coding standards should be split into 7.x and 8.x versions? Or do we update it for 8.x and just live with the fact that 7.x violates the current standard?

webchick’s picture

Yeah, update the coding standards and live with it. That's what we've done in the past. There's no harm in specifying the 8.x coding standards early in D7 for contributed modules.

catch’s picture

We did the same thing for string concatenation. I'm not sure if coder actively discourages adding data type now but if it does it'd be worth an issue/patch to coder to stop doing so for D7, then people can follow this if they want to in their modules without triggering warnings.

pillarsdotnet’s picture

Status:Needs work» Needs review
Issue tags:-Needs change record

@Lars Toomre

I would mark this RBTC except for confusion on my part about when to use '@return mixed' vis-a-vis say '@return string|boolean'.

The latter example is not mentioned by the coding standards nor used anywhere in core:

$ egrep -r '@(param|return) [^ |$]+ \|' includes modules | wc -l
0

A quick glance through the API module reveals that it doesn't really care what happens after @param or @return, so I guess there's no technical reason we shouldn't support such a standard.

Lars Toomre’s picture

@webchick re #27 - I think there is some confusion in the community about coding standards and what is applicable and when. For instance, @sun in #1159356-9: Better error handling asserts:


Drupal coding standards are version-independent and "always-current". All new code should follow the current standards, regardless of (core) version. Existing code in older versions may be updated, but doesn't necessarily have to be. Especially for larger code-bases (like Drupal core), updating the code of a previous version for current standards may too huge of a task. However, code in current versions should follow the current standards. Lastly, when following this practice, make sure to not squeeze coding standards updates/clean-ups into otherwise unrelated patches.

Based on your comment #27 above, does this mean patches for D7 will be rejected if they now include @param and @return type hinting? Or will D7 have some combination of the old and newer coding standards? If the latter is true, I would argue that this is a 'nice to have' patch that will help D7 DX now (while D8 is in development for the next several years).

pillarsdotnet’s picture

Issue tags:+Needs change record

@catch:

I'm not sure if coder actively discourages adding data type now

Searching through coder, I can't find where it does anything with the text following '* @param' other than ensuring a blank line before the @return line.

If this policy change goes through, I should probably write a D8 coder patch to issue a warning if the datatype is absent. I'm maintaining a D8 version locally anyway.

pillarsdotnet’s picture

Issue summary:View changes

Fixed tag nesting error.

Lars Toomre’s picture

@pillarsdotnet re: "when to use '@return mixed' vis-a-vis say '@return string|boolean'."

I cannot find the issue now, but there was discussion about the @return format like @Crell referenced in #20 above. Perhaps @Crell or @jhodgdon can give some guidance here?

pillarsdotnet’s picture

Getting a bit offtopic, but we even have a weird @param ... pattern to indicate a variable number of arguments fetched via func_get_args().

I'd add that pattern to the standards, but I lack the necessary privileges.

Posted: #1295576: Document the de-facto standard of using "@param ..." to indicate a variable number of function arguments.

joachim’s picture

> I believe, can help us encourage putting a useful description in the "description" part of the docblock, rather than just "an int" or "an id", which is a largely useless docblock.

I don't think it does that. We do want people to write more than 'an int' in the description, absolutely. But making them do this first is fixing the wrong thing.

Furthermore, I think that adding a type like 'string' or 'integer' tells you nothing. You still have to read the description to know what the integer represents, or what kind of strings are valid. And as for arrays... in Drupal, you *definitely* need to read the documentation to know what to put in an array parameter ;)

So adds no useful information, but creates noise in the documentation. I'm a -1 on this.

Crell’s picture

I have never once viewed type information in a docblock as noise. I've viewed it as a developer who cared enough to leave me breadcrumbs to figure out what is going on.

This is the issue I mentioned previously: #711918: Documentation standard for @param and @return data types. In it we opted to favor int|string over "mixed", and determined that NetBeans, at least, is really damned good with @return data types.

The current coding standards say that the @param type is not required, but it's also not disallowed. I see no problem with patches going forward, starting right now, for D7 or D8, specifying types.

joachim’s picture

  * @param $foo
  *  Description lorem ipsum...
  * @param $bar
  *  Description lorem ipsum...

  * @param string $foo
  *  Description lorem ipsum...
  * @param int $bar
  *  Description lorem ipsum...

It's a variable-length piece of text that means my eye can't predictably find the place to scan down for the names of the variables.

It adds little useful meaning, as I still have to read the description to know what the int or string should actually be.

Hence I call it noise. But I could say a drop in readability for little added meaning.

Lars Toomre’s picture

@pillarsdotnet - re: #25 - Based on #39 and what is defined in Documenting functions and methods, I can confirm now that the use of '@return mixed' is deprecated. Instead, the form should be '@return x|y' where x and y are types like string and boolean. This patch will need to be re-rolled for the preferred @return doc.

pillarsdotnet’s picture

Status:Needs review» Needs work

Okay, I missed this bit in the doc page before.

* @return Parser|false
*   A class instance object of type Parser, or Boolean FALSE.

Can someone explain to me why that says Parser|false rather than Parser|FALSE or even Parser|bool?

pillarsdotnet’s picture

Issue summary:View changes

Add coder patch to "Remaining tasks" list.

pillarsdotnet’s picture

Issue summary:View changes

Updated to reflect that 'mixed' is now deprecated, as per #41.

pillarsdotnet’s picture

Replaced all instances of mixed with foo|true|false|null as appropriate. Would really appreciate an answer to #42 before changing status to CNR, because this just feels wrong to me, despite being compliant with the current standards (which we are discussing changing anyway).

pillarsdotnet’s picture

Another question: should it be @param bool or @param boolean? I see examples of each in core.

bobvin@bowie:~/www/8.x$ grep -r '@param bool ' includes modules | wc -l
7
bobvin@bowie:~/www/8.x$ grep -r '@param boolean ' includes modules | wc -l
12
Lars Toomre’s picture

Status:Needs review» Needs work
Issue tags:+Needs change record

@pillarsdotnet - re: bool|boolean - Heck, if I know which is appropriate. However, that core discourages abbreviations in variable and function names, I would recommend using 'boolean'.

Perhaps we should open a new issue for 's/@param bool /@param boolean /'? My fear is that such a documentation correction will not be implemented before November 1st nd the re-rolling of all D8 patches.

pillarsdotnet’s picture

pillarsdotnet’s picture

Issue summary:View changes

Fixed another tag nesting error and changed "Remaining tasks" to an ordered list.

pillarsdotnet’s picture

Updated summary to reflect that bool is preferred over boolean for type-hinting.
See #1296208: Standardize on either '@param bool $foo' or '@param boolean $foo' and document the decision.

jhodgdon’s picture

Issue tags:+coding standards

There have been about 30 comments or more since yesterday on this issue....

Have you made changes to the coding standards? If so, what were they and can they be shown in the issue summary? And have they been added to the coding standards docs?

AND why didn't this issue have the "coding standards" tag if coding standards were going to be discussed????? We now have a list of standard issue tags (see link below the tags field and its sub-pages). This is a standard tag that should be on ALL coding standards issues... sigh.

pillarsdotnet’s picture

@jhodgdon:

Have you made changes to the coding standards?

No; I don't have permission. I can't even view the source to properly roll a patch.

AND why didn't this issue have the "coding standards" tag if coding standards were going to be discussed?????

Ah. I suspected there was a relevant tag but was too lazy to look it up; thanks.

Lars Toomre’s picture

I am confused about when @param and @return type hinting should be used.

Say we have a patch that needs to be applied to both D8 and D7. As I understand now, the D8 patch should include the type hinting. Assuming that the patch also applies to D7 without change, should a separate patch nonetheless be rolled removing all of the type hinting documentation for D7?

webchick’s picture

Yes, that's correct. It's the same as the concatenation differences between D6 and D7+.

pillarsdotnet’s picture

@#50

Assuming that the patch also applies to D7 without change, should a separate patch nonetheless be rolled removing all of the type hinting documentation for D7?

No. But if the patch happens to touch (but not modify) other lines that lack such type-hinting, or if the patch applies type-hinting to some functions in a file but not others, the patch submitter should resist "fixing what ain't broke".

At least, that is my interpretation of the guidelines as they now stand. I will gladly receive correction if anyone knows otherwise.

EDIT: retracted; sorry about the contradictory crosspost.

pillarsdotnet’s picture

@#51:

If we're going to be rolling different D8 and D7 patches based on different coding standards, then we really need to fork the coding standards into separate version-specific pages. Should that be discussed here, or in a separate issue?

jhodgdon’s picture

So. Has everyone (except me of course) already agreed that this is a good change do docs standards for D8? Is someone actually planning to go through every single core function and method header and put these things in? And have the key developers agreed that they are going to follow this for every new function and every change to every function? I personally think this is not a realistic idea unless the core dev team has bought into it.

So I don't have time to read this discussion, sorry. Busy day. Can someone please summarize who agreed to this and whether it's actually been agreed to? And not to mention why it's so necessary all of a sudden, when in d7 it was like pulling teeth to even get people to agree that we could add it as a "suggested good idea" rather than even close to a mandated standard?

pillarsdotnet’s picture

@jhodgdon

Has everyone (except me of course) already agreed that this is a good change do docs standards for D8?

No; joachim is also opposed, and several thousand developers have neglected to chime in on this issue.

Is someone actually planning to go through every single core function and method header and put these things in?

I volunteered to tackle the includes/*.inc files; you're recruiting a list of volunteers for the rest, so yes.

Can someone please summarize who agreed to this, and ... why it's so necessary all of a sudden

I'll update the Issue summary.

Lars Toomre’s picture

@jhodgdon - I am sorry that you feel like you have not been included. However, based upon your comment #711918-52: Documentation standard for @param and @return data types, it never even occurred to me that you might object to type hinting being implemented for D8 docblocks. I thought you were proposing that all of the docblocks be updated. Is this no longer how you feel?

jhodgdon’s picture

I am *not* planning to recruit volunteers for this task. I am planning on recruiting volunteers to do some more basic cleanup, like making sure there is a return between @param and @return, and that every function has a docblock, and the summaries are one line less than 80 characters. This is a much more involved task that requires a lot more inspection and thought. It cannot just be whipped out in a short session by novices.

And it's not that I feel that I wasn't included. I'm just trying to find out what you all think has been decided without reading through the 30 comments I missed between yesterday and today (which I have NO TIME TO READ, sorry), so I asked for a summary and so far no one has provided one :(

jhodgdon’s picture

Issue summary:View changes

Document preference of 'bool' over 'boolean'.

pillarsdotnet’s picture

Issue summary:View changes

Added a voting list in response to jhodghdon asking "Was this approved? By whom?"

pillarsdotnet’s picture

@jhodghdon:

I am *not* planning to recruit volunteers for this task.

Fine; I'll roll the entire patch myself; it's not hard once there is a standard to follow. Let me know whether you'd prefer one giant patch, one patch per file, patch per module, etc. I'm tired of arguing about such things.

I asked for a summary and so far no one has provided one :(

Summary updated, but I'll copy it here:

In favor:
  1. chx
  2. Lars Toomre
  3. webchick
  4. Crell
  5. catch
  6. pillarsdotnet
  7. sun
  8. mikey_p
  9. xjm
  10. sdboyer
  11. boombatower
  12. DjebbZ
Opposed:
  1. Dave Reid
  2. joachim
  3. jhodgdon

EDIT: Based on recent comments I conclude that jhodgdon is definitely opposed, which kinda makes this whole issue a non-starter.

Lars Toomre’s picture

@webchick - re: #51 - Thanks for your clarification. For D7 patches, there should not be any type hinting for @param or @return doc statements.

Your response also clarifies that @sun's assertion in #1159356-9: Better error handling is not true. This means one should not follow @sun's statement, "Drupal coding standards are version-independent and 'always-current'. All new code should follow the current standards, regardless of (core) version. Existing code in older versions may be updated, but doesn't necessarily have to be."

pillarsdotnet’s picture

Issue summary:View changes

Converted votes to ordered lists.

pillarsdotnet’s picture

Issue summary:View changes

Add "int" versus "integer" to the mix.

pillarsdotnet’s picture

For D7 patches, there should not be any type hinting for @param or @return doc statements.

I would tend to disagree. The published standard for D7 was and is:

If the data type of a parameter or return value is not obvious or expected to be of a special class or interface, it is recommended to specify the data type in the @param or @return directive

and

Primitive data types, such as int or string, do not need to be specified. It is recommended to specify classes and interfaces. If the parameter or return value is an array, or (anonymous/generic) object, you can specify the type if it would add clarity to the documentation. If for any reason, a primitive data type needs to be specified, use the lower-case data type name, e.g. "array". Also, make sure to use the most general class/interface possible. E.g., document the return value of db_select() to be a SelectQueryInterface, not a particular class that implements SelectQuery.

In other words, type-hinting is "recommended" in certain cases; "not needed" in others. It is never forbidden nor required. Removing all type-hinting from D7 core would be nearly as big a patch as adding it to all of D8 core.

Personally, I find the current standard to be ambiguous because I lack the particular variety of "common sense" that appears to be assumed here.

jhodgdon’s picture

I do not see comments here by all of those people stating they are in favor of fixing all of core and volunteering to review/roll the patches? I started at the top of the list, and there is no comment from chx on this issue at all?

jhodgdon’s picture

And I want to state that I think the standard is a good idea in the abstract, I just do not think (as I explained on the other issue -- why are we discussing this in two places??!? I am getting confused) it is practical to apply to all of core/contrib Drupal code as a standard that suddenly all code will be out of date on and need to be patched. This is not an automatic patch. It requires thought to write, and thought to review. It's not like "oh we need a space around ." or "we need a blank line between @param and @return sections".

pillarsdotnet’s picture

I do not see comments here by all of those people

No; I had to read through three separate issues. If you like, I can reference or copy/paste the quotes that led me to that conclusion, but I fear you won't have time to read it anyway.

stating they are in favor of fixing all of core and volunteering to review/roll the patches?

I'm sorry if I gave the mistaken impression that the people listed under "In favor" were volunteering to roll patches. I will change the text to "In favor of the standards change" and "Opposed to the standards change".

I started at the top of the list, and there is no comment from chx on this issue at all?

No. (searching...) chx commented here and was quoted here as being in favor of a wholesale change, broken up by module, into "a zillion sub-issues", pending your blessing.

pillarsdotnet’s picture

Issue summary:View changes

I'd say jhodgdon is definitely opposed.

pillarsdotnet’s picture

Status:Needs work» Closed (duplicate)

why are we discussing this in two places?

You're correct. Closing as duplicate.

pillarsdotnet’s picture

Issue summary:View changes

Clarifying what "In favor" and "Opposed" means, and moving jhodgdon to the "In favor" list as per her most recent comment.

webchick’s picture

Status:Closed (duplicate)» Needs work

Just to clarify something...

"And not to mention why it's so necessary all of a sudden, when in d7 it was like pulling teeth to even get people to agree that we could add it as a "suggested good idea" rather than even close to a mandated standard?"

In D7 it was like pulling teeth because of the timing of the initiative to introduce it (post code freeze), since it effectively represented an "API change." In D8 it's completely acceptable to revamp coding standards recommendations. Just as we introduced a different concatenation separator in D7.

webchick’s picture

Status:Needs work» Closed (duplicate)

Oops.

webchick’s picture

Issue summary:View changes

Re-read jhodgdon's comment -- she's neither in favor nor opposed; she just thinks it is not practical.

Crell’s picture

Status:Closed (duplicate)» Needs work

Boy this issue got busy...

Re Webchick in #51, I would agree with #60. I don't see anything in the current coding standards that forbids the use of a primitive type in a docblock, and core has some usage of param types scattered around. Various contrib modules (like, any I've written at least) already have them. I don't see why we would start not allowing them in D7 patches.

Going forward, I don't believe we need to have one massive uber-patch. This is the sort of thing that we can work on over time, hell even as part of other issues. (Changing this function? OK, fix its docblock, too, since I'm changing the function signature anyway.) Then we can do another walk-through later once we're done changing the code being documented to make sure any missing @param types are filled in.

I am not volunteering to write a mega-patch, but I have every intention of putting types on all of my @params going forward. I already do, but I intend to keep doing so. :-)

pillars: Um, Dave Reid hasn't commented in this issue that I see. Why do you have him listed as having an opinion?

Crell’s picture

Status:Needs work» Closed (duplicate)

Jebus these threads are moving fast...

pillarsdotnet’s picture

Status:Closed (duplicate)» Needs work

Um, Dave Reid hasn't commented in this issue that I see. Why do you have him listed as having an opinion?

From his comments here and here, I got the impression that he doesn't care either way; he just doesn't like change.

Again, feel free to make corrections. jhodgdon asked for a poll, so I did the best I could.

pillarsdotnet’s picture

Status:Needs work» Closed (duplicate)

And at any rate, further discussion should take place at #711918: Documentation standard for @param and @return data types

sun’s picture

sun’s picture

Issue summary:View changes

Should have used preview; sigh.

plach’s picture

Issue summary:View changes

plach +1

rafamd’s picture

For us coming from strongly typed languages (ex. java), this is very nice to have. Reading ahead *might* be necessary to understand the variable, but it's a bit of information that you can rely on (you know it's going to be there) and together with a meaningful name is a very nice starting point.

+1

EDIT: sorry, this is the dupe, re-posting to the active issue.

rafamd’s picture

Issue summary:View changes

Adding myself as a supporter for the change.

xjm’s picture

Issue summary:View changes
Issue tags:-Needs change record