Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ReflectionMethod accepts string|object, not mixed #147

Closed
wants to merge 1 commit into from

Conversation

BenMorel
Copy link
Contributor

ReflectionMethod's constructor currently uses mixed for its $class parameter, when it actually accepts either a string or an object.

There is precedence in the docs to use union types notation (see this for example), so is it OK if I convert more mixed to union types in the future?

@cmb69
Copy link
Contributor

cmb69 commented Sep 27, 2020

We need some clean way to represent union types for docbook, and to have PhD properly render these, before we should add union types to the manual. See php/phd#25 (comment).

@BenMorel
Copy link
Contributor Author

Thanks for the pointer. In the meantime, can't we follow the footsteps of the doc pages that have already adopted union types in the following form?

<type>bool|array</type>

That would allow us to start working on better type documentation right now, and should be trivial to replace programmatically once a suitable syntax has been made available.

There are quite a few doc pages already using this syntax: https://pastebin.com/q3Yu5GPd

@Girgias
Copy link
Member

Girgias commented Sep 27, 2020

Thanks for the pointer. In the meantime, can't we follow the footsteps of the doc pages that have already adopted union types in the following form?

<type>bool|array</type>

That would allow us to start working on better type documentation right now, and should be trivial to replace programmatically once a suitable syntax has been made available.

There are quite a few doc pages already using this syntax: https://pastebin.com/q3Yu5GPd

This just makes more work for translations, because we need to do 2 passes instead of just 1. Moreover, a lot of arguments in PHP 7.x do actually accept "mixed" due to legacy behaviour which only got removed with PHP 8.0.

As we need to update the prototypes anyway due to named parameters it would be better to combine all of this into one single change/commit by basing ourselves on the stubs when they are finalized.

@BenMorel
Copy link
Contributor Author

Just stumbled upon another one here: https://www.php.net/manual/en/function.getmyuid.php

The doc says getmyuid ( void ) : int; that's a mistake, it should be : int|false. What should we do for this one?

  • leave as is: it's a documentation bug, probably not a good idea, even if the description does mention false
  • change for : mixed: I hate the idea
  • or again, use <type>int|false</type>?

@cmb69
Copy link
Contributor

cmb69 commented Oct 15, 2020

leave as is: it's a documentation bug, probably not a good idea, even if the description does mention false

IMO, leave it as is for PHP 7 docs, but do it properly for PHP 8 docs. It seems to me that it might be benefical to have separate signatures for PHP 7/8, at least for some functions. The PHP 8 signatures should be generated (semi-)automatically.

change for : mixed: I hate the idea

That would even be counter productive, since false signals failure, i.e. the rather exceptional situation.

or again, use int|false?

Clear no from me. That should be <type class="union"><type>int</type><type>false</type></type> instead (what is still not supported by PhD), "even though that triples the size of thy methodsynopses and produce aches in thy typing fingers, for if thou thinkest 'this is too verbose', the gods shall surely punish thee for thy arrogance." I mean, we use DocBook for a reason. :) And, anyway, that type could be shortened to <type class="union">&integer;&false;</type> if desired.

@BenMorel
Copy link
Contributor Author

IMO, leave it as is for PHP 7 docs, but do it properly for PHP 8 docs.

Will PHP 7 & PHP 8 have different doc pages?

@cmb69
Copy link
Contributor

cmb69 commented Oct 16, 2020

Will PHP 7 & PHP 8 have different doc pages?

That's not planned.

@BenMorel
Copy link
Contributor Author

So I didn't get your comment, sorry?

IMO, leave it as is for PHP 7 docs, but do it properly for PHP 8 docs.

@cmb69
Copy link
Contributor

cmb69 commented Oct 16, 2020

Sorry, pretty unclear explanation from side, indeed. What I meant is that we should focus on (semi-)automatic generation of the PHP 8 function signatures. If these are identical to those for PHP 7, just replace them (that would be the case for getmyuid()). If the signatures are different (e.g. because some error returns have been replaced by exceptions in PHP 8), we could have two different signatures (one for PHP 8, and one for PHP 7).

However, thinking more about it, there's nothing wrong with fixing the PHP 7 signatures as soon as we have support for union types.

@cmb69
Copy link
Contributor

cmb69 commented Oct 29, 2020

I have applied this (with proper union type markup) as http://svn.php.net/viewvc?view=revision&revision=351060.

Wrt. getmyuid() returning int|false: I don't see much point in fixing individual functions. There are likely very many which may return false, but that is not yet reflected in the signature. It seems to me that a script which checks for &return.falseforfailure; in the "return values" section, could insert that automatically to the return type.

@cmb69 cmb69 closed this Oct 29, 2020
@cmb69 cmb69 removed the union-types label Oct 29, 2020
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.

None yet

3 participants