silverstripe / silverstripe-framework

Silverstripe Framework, the MVC framework that powers Silverstripe CMS
https://www.silverstripe.org
BSD 3-Clause "New" or "Revised" License
720 stars 820 forks source link

[ORM] API Documentation - Missing method parameters #7880

Open mooror opened 6 years ago

mooror commented 6 years ago

Affected Version

Silverstripe 3.6 API Docs Silverstripe 4.x API Docs

Description

When using the API documentation (for SS 3), I noticed that some methods are missing parameters. For example the DataList's sort() method. This method has the following DocBlocks

framework/model/DataList.php Line 283-296

/**
     * Return a new DataList instance as a copy of this data list with the sort
     * order set.
     *
     * @see SS_List::sort()
     * @see SQLQuery::orderby
     * @example $list = $list->sort('Name'); // default ASC sorting
     * @example $list = $list->sort('Name DESC'); // DESC sorting
     * @example $list = $list->sort('Name', 'ASC');
     * @example $list = $list->sort(array('Name'=>'ASC', 'Age'=>'DESC'));
     *
     * @param String|array Escaped SQL statement. If passed as array, all keys and values are assumed to be escaped.
     * @return DataList
     */

But when you look at DataList's sort method in the docs you will see that no Parameters (or examples) are listed.

Now I'm not sure if this is a DocBlock syntax issue or what, but when I brought it up on slack I was asked to create an issue for it. So uh, here it is.

Steps to Reproduce

If you travel to the API documentation for DataList's sort method you will see that the sort() method has no parameters listed even though the methods DocBlocks have one listed.

NightJar commented 6 years ago

Could be an issue with the generator, or docblock format, unsure. Advised the creation of an issue rather than a PR due to unknown cause.

robbieaverill commented 6 years ago

Thanks for reporting. I suspect it's a syntax issue, as you mentioned. The @param is invalid because it doesn't have a parameter name. Also String should be string. The API site runs PHP 7, in where String uppercased would represent a class name, which is a reserved word in PHP 7. There are plenty of occurrences in other parts of the core PHPDocs that use this though, so I don't think this would be the problem.

It looks a little iffy about how to declare this with variadic arguments across multiple PHP versions (SS3 supports PHP 5.3) so I'm not sure what the solution is.

You could test a few things out locally by installing the API site and running a build in a few different contexts to see.

robbieaverill commented 6 years ago

I've triaged this as a bug since we're incorrectly using PHPDoc. Will spend a bit of time now going through some instances of doc blocks in 3.6

dhensby commented 6 years ago

fixed by #7994? @robbieaverill

robbieaverill commented 6 years ago

I’m not sure, that PR was a whip over to fix up some of the phpdocs but I didn’t have time to do all of them

robbieaverill commented 5 years ago

Still an issue, just checked