Skip to content

Conversation

@salathe
Copy link
Contributor

@salathe salathe commented Nov 15, 2017

This change moves the return type name to the right-hand side of the
function/method prototypes, along the same lines as PHP 7 code.

Before:

int time ( void )

After:

time ( void ) : int

While this is a step towards PHP 7 style, there are still some
departures. Notably:

  • No conversion to valid scalar type names is done (e.g. converting
    <type>boolean</type> to : bool)
  • Pseudo-types and other invalid type names are used (e.g. resource,
    mixed)

This change moves the return type name to the right-hand side of the
function/method prototypes, along the same lines as PHP 7 code.

Before:
    int time ( void )

After:
    time ( void ) : int

While this is a step towards PHP 7 style, there are still some
departures. Notably:
- No conversion to valid scalar type names is done (e.g. converting
"<type>boolean</type>" to " : bool")
- Pseudo-types and other invalid type names are used (e.g. resource,
mixed)
@salathe
Copy link
Contributor Author

salathe commented Nov 15, 2017

See also https://bugs.php.net/bug.php?id=75527

@cmb69
Copy link
Member

cmb69 commented Nov 15, 2017

We must not forget to update http://www.php.net/manual/en/about.prototypes.php if this PR will be merged.

@salathe
Copy link
Contributor Author

salathe commented Nov 15, 2017

@cmb69 I made a wiki page yesterday with some notes on this change at Move return types in function/method prototypes. Feel free to edit / improve if you think of other places that will be affected.

@cmb69
Copy link
Member

cmb69 commented Nov 15, 2017

if you think of other places that will be affected.

I can't think of other affected places, but I'll update the Wiki page, if something comes into mind.

@KalleZ
Copy link
Member

KalleZ commented Nov 15, 2017

I think from a reading perspective (which also happens to be my personal preference), the current Java/C#/C/C++ style code is more eye catching, perhaps because we had this in the documentation for close to 20 years now.

I do however understand where the reporter in FR#75527 is coming from, and while it does make sense, I'm still skeptical of changing this for keeping our well structured documentation in line with a "recently" added language feature.

I'm a -1 on implementing the FR. Tho I thought about alternatives for supporting both preferences, but I don't think a toggle option or anything like that should be implemented, its either/or imho.

@salathe
Copy link
Contributor Author

salathe commented Nov 15, 2017

I'm actually of the same mind, @KalleZ. I prefer the old (current) placement of the return type, but using the PHP 7 style is something that has come up in discussions several times previously and generally seemed like a thing that was well received.

Let's let this PR stew for a while and see what comes of it. If nothing else, it made me go and have a good look at the innards of PhD again. 😄

@rquadling
Copy link
Contributor

Day 1 developers, looking to use PHP 7+ will be utterly confounded by the documentation using a structure that every single piece of code out there that is PHP 7+ compliant with regard to return types disagrees with. From day 1, the developer will be thinking that the documentation is wrong and so downgrades the actual quality of the documentation.

I've been pushing for this change to also take place in PSR-5 DocBlocks as this is another area where the signatures are incompatible and ambiguous. (And that cascades to PHPStan and PHPCS and other tools that can report and/or coerce the code to be standards compliant).

/* @method static IsThisAMagicStaticMethodOrAMagicMethodThatReturnsAnInstanceOfThisClass()

Getting PHP's own documentation to agree with the actual syntax the developer has to use is pretty much a given surely.

I would hope that enough developers are aware that PHP7 now supports return type hinting and the position of the typehint. And so having the documentation agree with it is a no-brainer.

Also, with PHP5.6 approaching its final year also, the new syntax will be the only syntax that modern, supported version of PHP will recognise.

How many more reasons???

@rquadling
Copy link
Contributor

Some links to related issues on type hinting and return types.

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

Basically, once PHP's own documentation is compatible with the current version of PHP, we have a better chance of getting this into PSR-5 and then into PHPStan/PHP-CS/etc.

@itskevinsam
Copy link

still skeptical of changing this for keeping our well structured documentation in line with a "recently" added language feature.

right, as the purpose of php documentation is to confuse its stupid users with obscure syntax , we should stick on to it.

@morrisonlevi
Copy link
Contributor

@salathe Any concerns on merging this now? I have tested it out locally and spot checked a few files; looks good. You did a much better job of implementing it than I did (I'm not even going to share my solution publicly).

@rquadling
Copy link
Contributor

I thought I had write access to this project? I used to have access to the SVN repo (I may still do!).

@cmb69
Copy link
Member

cmb69 commented Apr 30, 2018

@rquadling You are supposed to have pull permissions for https://git.php.net/?p=phd.git.

@salathe
Copy link
Contributor Author

salathe commented Apr 30, 2018

@salathe Any concerns on merging this now? I have tested it out locally and spot checked a few files; looks good

No new concerns beyond what is already mentioned in the discussion here. I would encourage folks to re-read the discussion, particularly keeping in mind that the end result is that the function prototypes will look similar to PHP 7 return type declarations, but definitely won't always be valid PHP 7 return type declarations. (Much like function/class prototypes in general look similar to function/class declarations but aren't valid PHP code.)

You did a much better job of implementing it than I did (I'm not even going to share my solution publicly).

You're making me blush.

@ashnazg
Copy link

ashnazg commented May 15, 2018

PSR-5 is watching this PR with interest... the decision here will greatly sway the decision there.

@KalleZ
Copy link
Member

KalleZ commented May 15, 2018

I had an extra thought about this and I still stand by my original statement above, at least for as long PHP5 is still being actively supported.

@cmb69
Copy link
Member

cmb69 commented May 15, 2018

Before PHP 7.0.0 there was no support for return type declarations, so the documentation had to choose some style; obviously, a C style was chosen. Fine. However, PHP supports return type declarations since quite some time, so I can't see a single good reason to stick with the somewhat arbitrarily chosen style – even developers who still write PHP 5.x compatible code and who are accustomed to the current prototype style of the PHP manual (such as myself) will eventually have to catch up (likely sooner than later).

@rquadling
Copy link
Contributor

rquadling commented May 16, 2018

As of end of December 2018, PHP 5 will be unsupported, according to http://php.net/supported-versions.php.

That will be over 2 years since I started this task of aligning the signatures in documentation to those used in code. Sure, due to a lack of support for union types in PHP, we won't have 100% parity, but that will probably come eventually when someone works out how to deal with it efficiently (https://wiki.php.net/rfc/union_types was declined).

@philip
Copy link
Member

philip commented May 22, 2018

Is there a PHP docs php-web build online with this change so we can see what this look's like?

@salathe
Copy link
Contributor Author

salathe commented May 22, 2018

@philip I've uploaded a static HTML render (from phd --package PHP --format xhtml ...) at https://cowburn.info/phd/phd-17/.

Five randomly chosen example pages:

@philip
Copy link
Member

philip commented May 23, 2018

Thanks @salathe, it looks nice and I didn't find undesired side effects but was curious how namespaced methods like TokyoTyrantTable::getQuery would look.

Sorry it's been awhile since I looked at phd, but this PR doesn't appear to alter man pages and PDF, right? Or PEAR? Am only curious, especially about man pages as those are referenced on php.net.

Kudos for the patch and not requiring DocBook changes.

Also FWIW, ask me every day and 5 days out of 7 I'd vote yes to implement this change now :)

@salathe
Copy link
Contributor Author

salathe commented May 26, 2018

this PR doesn't appear to alter man pages and PDF, right? Or PEAR?

That is right, the intention was to get it changed in the manual pages published on the website. Changing the downloads (e.g. XHTML, CHM) was a happy side-effect. Other formats (e.g. man pages, PDF) in the PHP package and other pages (i.e. IDE, PEAR) haven't been given much, if any, consideration.

We can look at changing the other packages/formats, but the focus was very much on updating the website with this change.

Also FWIW, ask me every day and 5 days out of 7 I'd vote yes to implement this change now :)

Ditto.

@salathe
Copy link
Contributor Author

salathe commented Sep 19, 2018

Bump. Any objections to merging this?

@KalleZ
Copy link
Member

KalleZ commented Sep 19, 2018

Same statement as before, -1 but then again I'm not really an active documentor anymore so take it for what its worth

@nikic
Copy link
Member

nikic commented Sep 29, 2018

@salathe Please go ahead.

@ashnazg
Copy link

ashnazg commented Nov 8, 2018

Is this one still being considered/discussed?

@rquadling
Copy link
Contributor

I think this should still be accepted. Just needs someone with the authority to accept it!

@cmb69
Copy link
Member

cmb69 commented Nov 12, 2018

Just needs someone with the authority to accept it!

IMHO that should be the manual's editor. :)

@php-pulls php-pulls merged commit 126b9be into php:master Nov 12, 2018
@salathe salathe deleted the feature/php7-style-return-types branch November 12, 2018 19:09
@salathe
Copy link
Contributor Author

salathe commented Nov 12, 2018

Thanks for the reminders. I think this one has been brewing for long enough!

@cmb69
Copy link
Member

cmb69 commented Nov 27, 2018

Hm, just noticed that at least several mirrors (maybe all) still show the old syntax. http://docs.php.net/ is fine, though.

@salathe
Copy link
Contributor Author

salathe commented Nov 27, 2018

Hm, just noticed that at least several mirrors (maybe all) still show the old syntax.

The machine that the docs are built on hasn't had PhD upgraded yet.

@salathe
Copy link
Contributor Author

salathe commented Jan 11, 2019

@cmb69 et al., the docs are now being built with the newest version of PhD, and the website mirrors are now showing this change.

@rquadling
Copy link
Contributor

Thank you everyone!!!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants