| Project: | API |
| Version: | 6.x-1.x-dev |
| Component: | Parser |
| Category: | feature request |
| Priority: | major |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
Issue Summary
Drupal's docblock format is derived from Doxygen. Doxygen was designed for C++ and other strongly typed languages, where specifying the type of a given parameter or return value was redundant since it was always specified in the language itself.
That is not the case in PHP. In PHP, you don't know what type you're dealing with unless you specify that explicitly in documentation. PHPDoc, therefore, includes the following format for docblocks:
<?php
/**
* Short description here.
*
* @param string $myvar
* Description of $myvar here.
* @return int
* Description of the meaning of the return value here.
*/
?>We should do the same, and optionally allow type definitions on both params and return.
Why?
For one, nearly all code completion tools for PHP auto-generate such values where known so we're getting them in contrib anyway, and even some core docblocks have them. We may as well allow them explicitly and ensure that nothing breaks.
For another, while you can in PHP 5 specify a class or interface type explicitly in the function signature to indicate that the value MUST be of that type (and it's a fatal error if not), that doesn't do anything for return types. However, when dealing with OO code (which Drupal is starting to do a fair bit of between Views, DBTNG, the new update system, and various others) it much much nicer to be able to specify a return value type. Many IDEs can detect that information and then code-complete a returned object dynamically. That is, if I call db_query(), and it's documented to return an object of type DatabaseStatementInterface, then I can type db_query()-> and get all of my possible fetch methods listed for me. That's extremely useful.
Even for parameters there is a benefit. While PHP can specify a type in the method signature, when inheriting methods you sometimes don't want to enforce that because the child method will need a different interface than the parent; specifically an interface that is extended from the interface specified in the parent. Doing that breaks (It's E_STRICT invalid at the very least), so you can't do so in the actual method signature. You can, however, specify the exact type you want for that subclass in the docblock for a method without breaking the parser.
Even if this information is not displayed in the API docs, we should make sure that api.module doesn't choke on type information when it's provided and handles it gracefully.
Comments
#1
subscribing
#2
Hell yes!
#3
+1
#4
Better quality documentation at the cost of maybe a parser bug or two... seems like an easy win.
The only really special case I can think of is specific Object types (Interfaces, etc.), which could be turned into links.
#5
Yes, automatically linking types would be neat. I'm happy for that to be a phase 2 though, as long as we can start entering them into code. I'd love to go through DBTNG and just add a whole bunch of documentation.
#6
I already do this, and kinda look down on code that doesn't - at least, if they do doc parameters, but don't indicate type. And it's even more extra-super useful, as Crell pointed out in the OP, if you specify object types.
Massive +1.
#7
Our coding standards now suggest that if your param or return isn't a simple type, you should put the type there, and that if it is a simple type, you may put the type there. See http://drupal.org/node/1354#functions
And we have a few functions in Drupal 7 core that have types for params or returns. The API module displays them without any trouble.
The feature request would then be that if the type is a class/interface, the type would automatically generate a link to the class/interface definition.
#8
@jhodgdon: This move was a bit quick. There was neither an agreement nor a conclusion on this issue. Only a few people chimed in thus far.
I, for one, am against documenting "array" and (generic) "object" as data types, just like there's not really a reason for why we should document "int" or "string".
Interfaces, classes, and all that obscure stuff, yes, that makes sense.
#9
This was discussed on another issue, and it is standard on php.net.
#10
I would also note that I was against the whole idea of putting these types in at all, but the consensus (and standards elsewhere) were to use array and object when the param/return is an array/object.
#11
The really nice thing about things like:
<?php/**
* @return PDOStatement
**/
function execute_query() {
?>
Is that your development IDE can explain what the return type is and even give you a list of functions to use for the correct object that is returned.
Such as in the following:
<?php$row = execute_query()->fetch();
?>
#12
What's the status of this? Another issue was mentioned by jhodgdon (needs link), so I'm not sure if this still needs to be open or not.
#13
The other mentioned issue was about defining standards for how to put types on params/returns:
#711918: Documentation standard for @param and @return data types
This issue is about turning these types into links, in the API module.
#14
Changing issue title to be more concise. The issue scope split seems good to me.
Reading the other issue brought up a good point: we should expect trash in a lot of doc blocks because of the old standard. The type isn't clearly delineated (just the first word in the sentence really) so we'll need to be careful how we treat unknown types.
My recommendation is that we don't style the return type differently, just wrap it in an anchor. That way "no return type specified" and "return type specified, but not known to the API module" collapse to the same case. If we tried to separate it into a separate column, we would frequently end up with nonsense like in the linked comment.
#15
OK. Let's have a little summary:
The idea here is that if you have
* @param typename $foo*
* @return typename
*/
If typename corresponds to a class/interface name that we know about, turn it into a link.
Here's an example page:
http://api.drupal.org/api/drupal/includes--database--database.inc/functi...
The @return should link to
http://api.drupal.org/api/drupal/includes--database--database.inc/interf...
#16
Upping priority (marking "major" things that I'd like to see addressed sooner rather than later).
#17
This is a ridiculously simple fix -- the function that formats param/return just has to be told "be aggressive on looking for class names" (it is already doing that in the @throws section).
See attached patch and screen shot. I'll write some tests and commit it.
#18
I added a sample function to the tests/samples/sample.php file, with both param and return classes -- looks like this:
/*** Function that has classes for parameter and return value.
*
* @param SubSample $parameter
* This parameter should link to the class.
*
* @return SampleInterface
* This return value should link to the interface.
*/
function sample_class_function($parameter) {
}
Attached screen shot shows how it renders. I'll use this for the tests...
#19
So... we already have tests for the function that is making the links to the classes that is being used here, and we don't have any tests currently for the final formatted output. So I think we're OK on tests for now. I did have to update two test assertions (in api_update_branch_php() test -- where it counted the number of objects). Committed and pushed!
http://drupalcode.org/project/api.git/commit/9200d71
See also #83126: Better formatting for parameters section where I would like to further improve the formatting...
#20
Automatically closed -- issue fixed for 2 weeks with no activity.