Align PSR-5 @method signatures with PHP7 with regard to return types #899

rquadling · 2017-03-14T10:32:31Z

Currently, the placement of the return type does not match that of PHP7.

For a new developer, this would seem 'wrong' as there are now 2 different signatures for methods.

In addition, allows for the documentation to support static methods that are processed by __callStatic().

And finally, this change removes the ambiguity where a method may be one of two;

<?php

class Parent
{
    public function __call()
    {

    }

    public static function __callStatic()
    {

    }
}

/**
 * @method static getString()
 */
class Child extends Parent
{
    <...>
}

Is getString() a static method returning void, or a regular method returning a static instance?

By having the scope and return type separated, the ambiguity is removed.

Also, incorporate static method support.

schnittstabil · 2017-03-14T13:04:20Z

proposed/phpdoc.md

@@ -1220,18 +1220,19 @@ The @method allows a class to know which 'magic' methods are callable.

 #### Syntax

-    @method [return type] [name]([type] [parameter], [...]) [description]
+    @method <static|> [name]([type] [parameter], [...])[: return type] [description]


<static|> seems to be incorrect – according to ABNF. Dunno ABNF in all details, but [static] looks more appropriate to me.

Ah. OK. But shouldnt' [name] be name as it is not optional.

IMO, at least in this case <name> would be even better (ABNF 2.1. Rule Naming):

Unlike original BNF, angle brackets ("<", ">") are not required.
However, angle brackets may be used around a rule name whenever their
presence facilitates in discerning the use of a rule name.

But honestly, I cannot really figure out the rules how PSR-5 mixes ABNF and PHP 😕

I'm just learning this too. I don't know what is more 'correct'.

This section is not in ABNF. 😄

I'm hoping someone who knows how to express what I need will be able to add the syntax.

@teohhanhui Dunno, if I understand you correctly, but this section uses ABNF.

Thus, either name or <name> should be used IMO; and to avoid ambiguity, I'm in favor of <name>.

How about : @method ["static"] <name>([<type>] [<parameter>] [, ...])[: <returnType>] [description]

Optional literal static'.
Required name.
Optional type and parameter and repeatable.
Optional : return type.
Optional description.

If it's ABNF then it'd be "@method" ["static"] name "(" ... where name is a non-terminal.

Which brings us back to the issue:

But honestly, I cannot really figure out the rules how PSR-5 mixes ABNF and PHP 😕

Obviously the square brackets in @method [name](…) [description] have different meaning – or is name really optional – or is description mandatory?

IMO, an appropriate solution would look something like:

@method [static] <name>([<parameter-list>])[: <type>] [<description>]

Where square brackets ([ and ]) enclose optional elements, and angle brackets (< and >) denote ABNF non-terminals.

abacaphiliac · 2017-03-14T15:50:45Z

+1

localheinz

👍

rquadling · 2017-03-22T13:32:13Z

Hello @michaelcullum. Wondering if you have any suggestions on this as you are the 'required approver'.

michaelcullum · 2017-03-22T13:33:04Z

It needs the PSR-5 team to approve, it just defaults to me when nobody is in that team. We need to setup a PSR-5 WG with @mvriel.

samdark · 2017-03-25T12:05:31Z

The change itself makes perfect sense so I think it has a big chance to be merged.

rquadling · 2017-03-28T12:24:17Z

I've sent an email to @mvriel. Not sure if there's anything else I can do at this stage.

GeeH · 2017-07-11T14:25:33Z

I am putting together a WG to get PSR-5 moving again - would be happy if anyone here would like to join.

SpacePossum · 2017-07-12T07:58:31Z

@GeeH if someone is interested in joining the WG, how would one proceed?
(I know someone who might be a good addition to the group)

GeeH · 2017-07-12T08:57:18Z

Ideally reply to the thread on the FIG mailing list - I'll post more info here soon.

…

On Wed, 12 Jul 2017 at 08:58 SpacePossum ***@***.***> wrote: @GeeH <https://github.com/geeh> if someone is interested in joining the WG, how would one proceed? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#899 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAlcAOdVwjNqFlqFvfzxNymg2vdnZPKUks5sNHy6gaJpZM4McYyT> .

GeeH · 2017-07-12T09:58:59Z

If you would like to be involved in the working group, please post here https://groups.google.com/forum/#!topic/php-fig/bcV4KXIW6dQ

tractorcow · 2017-08-06T22:34:09Z

Thanks @GeeH I've put my hand up. :)

rquadling · 2017-11-07T13:29:14Z

My proposal for supporting multiple types in DocBook has been accepted (with amendments) into the DocBook standard. [1], [2], [3].

I'm now looking at getting the PHP Documentation rendering tools to output the PHP 7 syntax for function and method signatures.

This may provide a review of the rfc for union types in PHP [4].

In the meantime, is there anyone ready, willing, able to actually move PSR-5 forward a little in this regard. I know it is a tiny change, but even having it accepted into the standard would allow for additional tooling to start the task of getting things uptodate.

[1] docbook/docbook#91
[2] docbook/docbook#96
[3] http://docbook.org/xml/5.2b03/
[4] https://wiki.php.net/rfc/union_types

ashnazg · 2018-05-11T15:44:22Z

I'm discussing with the phpDocumentor team about the feasibility of deprecating the old syntax in favor of this new syntax, as I think the parser should be able to distinguish between the two. I've also broached the subject with the WG.

Any folks from the IDEs: feedback here would be most useful.

neuro159 · 2018-05-14T19:55:14Z

Note
@method static returnType name()
where static is a static qualifier was introduced by PhpStorm in 2014 due to user requests https://youtrack.jetbrains.com/v2/issue/WI-21935

We don't have metrics on how widely it is now used in codebases out there.

neuro159 · 2018-05-14T19:58:29Z

While automatic migration is relatively easy...

I personally fail to see the real benefit of changing @method syntax for EVERYBODY, instead of only those wanting '@method static'. Much easier change would be '@method-static', as '@property-read'.

Lets be realistic - the old syntax will be out there for .. AGES. Even if we only generate new syntax and all major frameworks update.. we stuck with all implementations still supporting old syntax.

Please think twice about existing users. Perhaps working on commercial software and dealing with paying customers complains biases me too much... But such a change "just for sake of consistency" reminds me of lot of rollbacks I personally had to do. Also, being too disruptive may reduce chances of this PSR to be approved. Lets try to find the middle way.

ashnazg · 2018-05-14T20:01:04Z

I'm mainly swayed by this argument based on the matching return type syntax of PHP7.

ashnazg · 2018-05-14T20:21:17Z

Maybe a middle ground here is to allow an optional static to appear between @method and the optional return type, e.g. @method static int foo().

EDIT: Eclipse PDT (Oxygen) already allows for this:

RE-EDIT: I see now this matches the 2014 addition that @neuro159 mentioned earlier about PhpStorm.

rquadling · 2018-05-14T23:45:45Z

With modern tooling, converting the inconsistent style to match PHP 7's syntax should make this a non-issue. There are many tools that can hook into GitHub and other repos that will push the changes quite happily as a pull request.

I suppose another way to look at this is what SHOULD the standard be?

rquadling · 2018-05-14T23:48:24Z

@ashnazg, the ambiguity is with @method static foo(). Is static scope or return type?

@method static foo(): static is clear and unambiguous.

@method static static foo() looks like a cut and paste typo, but is correct.

JanTvrdik · 2018-05-15T07:50:00Z

We don't have metrics on how widely it is now used in codebases out there.

I actually have the metrics (computed based on top 1000 packages on packagist).

method tag
  static returnType name + (args)                 549      4.245 %
  static returnType name + ()                     242      1.871 %
  static name + (args)                             30      0.232 %
  static name + ()                                  0      0.000 %
  returnType name + (args)                      11148     86.205 %
  returnType name + ()                            767      5.931 %
  name + (args): returnType                         0      0.000 %
  name + (): returnType                             0      0.000 %
  name + (args)                                    78      0.603 %
  name + ()                                        63      0.487 %
  name                                             23      0.178 %
  other                                            32      0.247 %

So the @method static <type> <name>(<args>) syntax is used by over 6 % of @method tags.

rquadling · 2018-05-15T09:52:57Z

So, with that analysis, having tools like PHPCS to fix it to PSR-5 would be relatively simple.

@neuro159 With any change, there's resistance. That's fair. Every point has to be argued and an agreement reached. But the "that's how we've always done it" position is problematic in that it can be used to resist all change. For all those new developers coming to PHP in the next decade, why are we forcing 3 different and incompatible syntaxes on them for what should/could be an easy replacement.

In a decade's time, if unchanged, we'll have more developers and more code not matching. The benefit's aren't just about "consistency". If a new developer comes to the PHP documentation and sees one syntax for the functions and methods and tries to write that in their code, it'll fail. If they try to use the docblock syntax, their code will fail. How is this of any benefit? Tooling can fix this.

Certainly reaching out to the IDEs would be extremely useful. I'm only a user of PHPStorm, but I can certainly put a request for some comments out there. https://youtrack.jetbrains.com/issue/WI-42107

neuro159 · 2018-05-15T13:27:23Z

I just was saying that
to remove ambiguity for 6 % of users you make a new convention that affects 100% users
including
returnType name + (args) 11148 86.205 %
returnType name + () 767 5.931 %
by moving return type to the end.

It will take me a day to support this, with inspection and auto converter in PhpStorm, its NOT A PROBLEM for IDE. (thanks for request :)

IMO you adding NO value (except aesthetics) to any of those people (those 6% who use static know how it works anyway) and this is the problem.
Do you understand that If they convert to new syntax they loose support in all existing tool chains including old versions of all IDEs and type checkers they have set up? So they would not.

JanTvrdik · 2018-05-15T13:43:38Z

@rquadling New syntax cannot resolve ambiguity. It can only create new one. Ambiguity can only be resolved by introducing a resolution rule or by breaking compatibility. I'm not actually sure at all, what you're proposing. For example https://youtrack.jetbrains.com/issue/WI-42107 mentions that the return type declaration is optional. Is that an oversight? Or is that something you are actually proposing? Can you explicitly specify what conflict resolution rules and what breaking changes are you proposing?

ashnazg · 2018-05-15T16:25:16Z

If php/phd#17 does get accepted, I think it gives this PR a bit more strength.

To reiterate, I like this change not so much for resolving the static static use case, but to more fully embrace the PHP7 return type hinting syntax and growing its usage.

JanTvrdik · 2018-05-15T16:34:10Z

I like this change

What change? Can you explain to me what is actually being proposed? You cannot just propose new stuff without proposing how to handle the old stuff.

ashnazg · 2018-05-15T18:18:42Z

@JanTvrdik : what's being proposed is that the token positions in the @method tag be changed, from its PHP4 beginnings in phpDocumentor, to fit the new PHP7 return typing format.

Old way: @method [optional return type] functionName()
New way: @method [optional scope modifier] functionName() [: optional return type]

If a return type is given, it now comes after the function name, delimited by colon... just like PHP7 return types are written on the function signature.

If a scope modifier exists, it is now the only thing to expect between the tag and the function name.

That is the change being proposed. One provided rationale for the change has been the static (modifier) static (return type) potential ambiguity, but what sways me on the overall worth of the change is instead the matching of the return type hinting in PHP7.

In my discussions within the phpDocumentor team, we're thinking we can successfully deal with recognizing both the old and new formats, and treating the old format as "deprecated". I get the feeling from @neuro159 's comments that IDE support wouldn't be complicated either.

Also, as @rquadling has mentioned repeatedly, updating old code is likely feasible via code sniffing CS fixers.

JanTvrdik · 2018-05-15T20:04:55Z

we're thinking we can successfully deal with recognizing both the old and new formats

There are grammar conflicts, i.e. it cannot be done. That's why I'm asking about the grammar conflict resolution rules and the BC break impact.

Specifically based on your explanation of what is being proposed (and me guessing what does this mean from grammar point of view), at least the following stuff will be broken due to grammar conflicts if this proposal is accepted.

all non-static @method tags which return static type, e.g. @method static fluentMethod()
all @method tags with description that starts with :
all @method tags which return callable type

ashnazg · 2018-05-15T20:22:46Z

Deprecation of the old format would be of the "best effort" type, as opposed to "handles all cases":

is it the new format?
if not, does it look like the old format?
if so, try to resolve it as the old format, and give some indication of "deprecated syntax"
otherwise, denote the syntax as invalid

I'm not arguing that all possible permutations of both syntaxes be guaranteed.

For your three examples, I would expect these responses:

this is already ambiguous when in the old syntax... use the new, problem solved
- @method fluentMethod() : static
this is true, but do we reasonably expect descriptions to begin with a colon?
- if we're determined to handle this edge case, we could parse for the first word after the colon to see if it's a known FQSEN
I don't see why callable alone as the return type shouldn't work in either syntax...
- @method callable thisMethod()
- @method thisMethod() : callable
- now, if you mean a syntax that also includes the callable's return type, ... then that's a question beyond the current discussion anyway

ashnazg · 2018-06-04T17:40:31Z

Rereading this with fresh eyes... if there's no consensus on changing the @method syntax, then maybe @neuro159 's suggestion of @method-static is the only other possibility for resolving the aforementioned @method [static function modifier] [static return type] foo() use case.

JanTvrdik · 2018-06-05T08:27:09Z

@ashnazg Can't we just require explicit return type for static methods? i.e. the syntax would be

MethodTagValue
  = StaticKeyword ReturnType MethodName '(' [MethodArgs] ')' [Description]
  / [ReturnType] MethodName '(' [MethodArgs] ')' [Description]

(That's what PHPStan currently does)

ashnazg · 2018-09-26T17:40:51Z

@JanTvrdik yes, that would be a viable option to solve the static ambiguity.

ashnazg · 2018-09-26T17:45:16Z

It seems like php/phd#17 might be close to being merged. If it does, then I might look at doing a Google survey to see how much interest there is in changing to @rquadling 's original request for PHP7 return syntax. If there's mostly pushback to keep the existing @method syntax, then that will be that. However, given the improvements in tooling the last few years, I can imagine a potential majority of community willing to make the change... had tooling been this good way back, I could believe PSR-2 might have been more acceptable to have been more uniform in its rules.

ashnazg · 2018-09-26T18:21:33Z

Note to self: this PR would have to be updated due to the split into PSR-5/PSR-19.

ashnazg · 2018-11-16T19:10:55Z

As much as I'd like this change, I need to decline it for PSR-19 due to level of BC. Note that I won't forget it, though, when working on a superceding PSR that adds new concepts (generics et al) later on.

rquadling · 2018-11-22T18:09:34Z

As of 15th November, the PHP Documentation is now rendered closer to the standard format : http://docs.php.net/manual/en/function.curl-init.php as an example.

WinterSilence · 2022-03-21T11:34:43Z

@JanTvrdik

So the @method static () syntax is used by over 6 % of @method tags.

your stats base on PhpDoc for static methods or at all?

Align PSR-5 @method signatures with PHP7 with regard to return types

fa5a326

Also, incorporate static method support.

This was referenced Mar 14, 2017

objectOrClass, propertyExists, and propertiesExist beberlei/assert#218

Merged

Docblocked static methods with return types. PHP-CS-Fixer/PHP-CS-Fixer#2592

Closed

schnittstabil reviewed Mar 14, 2017

View reviewed changes

Use a more appropriate structure for declaring the 'static' element

f988de3

localheinz approved these changes Mar 14, 2017

View reviewed changes

samdark approved these changes Mar 25, 2017

View reviewed changes

samdark added the PSR-5 + PSR-19: PHPDoc label Mar 25, 2017

Fuco1 mentioned this pull request May 2, 2017

Allow @method annotations for methods with no arguments to omit () phpstan/phpstan#193

Closed

samdark approved these changes Jul 11, 2017

View reviewed changes

samdark self-assigned this Sep 7, 2017

rquadling mentioned this pull request Nov 16, 2017

Render return type in PHP 7 style php/phd#17

Merged

azjezz approved these changes Aug 14, 2018

View reviewed changes

samdark removed their assignment Aug 16, 2018

ashnazg self-assigned this Oct 2, 2018

ashnazg mentioned this pull request Oct 13, 2018

Aligning @method syntax with PHP syntax phpDocumentor/fig-standards#93

Closed

pullapprove bot requested review from ashnazg and SniperSister November 13, 2018 12:27

ashnazg closed this Nov 16, 2018

rquadling mentioned this pull request Mar 16, 2022

Align PSR-19 @method signatures with PHP7 with regard to return types #1272

Closed

Align PSR-5 @method signatures with PHP7 with regard to return types #899

Align PSR-5 @method signatures with PHP7 with regard to return types #899

Conversation

rquadling commented Mar 14, 2017

schnittstabil Mar 14, 2017 • edited Loading

Choose a reason for hiding this comment

rquadling Mar 14, 2017

Choose a reason for hiding this comment

schnittstabil Mar 14, 2017 • edited Loading

Choose a reason for hiding this comment

rquadling Mar 14, 2017

Choose a reason for hiding this comment

teohhanhui Mar 16, 2017

Choose a reason for hiding this comment

rquadling Mar 16, 2017

Choose a reason for hiding this comment

schnittstabil Mar 16, 2017

Choose a reason for hiding this comment

rquadling Mar 16, 2017

Choose a reason for hiding this comment

teohhanhui Mar 16, 2017 • edited Loading

Choose a reason for hiding this comment

schnittstabil Mar 16, 2017

Choose a reason for hiding this comment

abacaphiliac commented Mar 14, 2017

localheinz left a comment

Choose a reason for hiding this comment

rquadling commented Mar 22, 2017

michaelcullum commented Mar 22, 2017

samdark commented Mar 25, 2017

rquadling commented Mar 28, 2017

GeeH commented Jul 11, 2017

SpacePossum commented Jul 12, 2017 • edited Loading

GeeH commented Jul 12, 2017 via email • edited Loading

GeeH commented Jul 12, 2017

tractorcow commented Aug 6, 2017

rquadling commented Nov 7, 2017 • edited Loading

ashnazg commented May 11, 2018

neuro159 commented May 14, 2018

neuro159 commented May 14, 2018 • edited Loading

ashnazg commented May 14, 2018

ashnazg commented May 14, 2018 • edited Loading

rquadling commented May 14, 2018

rquadling commented May 14, 2018

JanTvrdik commented May 15, 2018

rquadling commented May 15, 2018

neuro159 commented May 15, 2018

JanTvrdik commented May 15, 2018

ashnazg commented May 15, 2018

JanTvrdik commented May 15, 2018

ashnazg commented May 15, 2018

JanTvrdik commented May 15, 2018

ashnazg commented May 15, 2018

ashnazg commented Jun 4, 2018

JanTvrdik commented Jun 5, 2018 • edited Loading

ashnazg commented Sep 26, 2018

ashnazg commented Sep 26, 2018

ashnazg commented Sep 26, 2018

ashnazg commented Nov 16, 2018

rquadling commented Nov 22, 2018 • edited Loading

WinterSilence commented Mar 21, 2022

schnittstabil Mar 14, 2017 •

edited

Loading

schnittstabil Mar 14, 2017 •

edited

Loading

teohhanhui Mar 16, 2017 •

edited

Loading

SpacePossum commented Jul 12, 2017 •

edited

Loading

GeeH commented Jul 12, 2017 via email •

edited

Loading

rquadling commented Nov 7, 2017 •

edited

Loading

neuro159 commented May 14, 2018 •

edited

Loading

ashnazg commented May 14, 2018 •

edited

Loading

JanTvrdik commented Jun 5, 2018 •

edited

Loading

rquadling commented Nov 22, 2018 •

edited

Loading