Skip to content
This repository was archived by the owner on Dec 29, 2020. It is now read-only.

Conversation

mfn
Copy link
Contributor

@mfn mfn commented Apr 5, 2016

Only in effect on all-lowercase types. This is to filter out stuff like
mixed, void, $this, etc.

Currently return type hints like mixed, void or $this, which are quite common, get converted literally into PHP.

mixed and void are expected to be classes, $this isn't an allowed syntax.

The idea of this PR is that, in case the whole type hint is in lowercase, it must be one of the supported type declarations as listed at http://php.net/manual/en/functions.arguments.php#functions.arguments.type-declaration ; otherwise the type hint gets just ignroed.

This automatically filters out a lot of unsupported stuff.

An edge case is when referring to all-lowercase classes. This PR wouldn't support them.

If this is acceptable, I can provide tests too.

@dunglas
Copy link
Owner

dunglas commented Apr 18, 2016

👍! Can you provide a test?

Btw, can you add support for common aliases like integer for int? Here is a list of common aliases: https://github.com/symfony/symfony/blob/master/src/Symfony/Component/PropertyInfo/Extractor/PhpDocExtractor.php#L324-L346

Only in effect on all-lowercase types. This is to filter out stuff like
`mixed`, `void`, `$this`, etc.
@mfn
Copy link
Contributor Author

mfn commented Apr 18, 2016

  • rebased against master
  • added normalization
  • added a test

return 'callable';

case 'void':
return 'null';
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one can be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thx, force-pushed an update!

@dunglas dunglas merged commit ccf37b7 into dunglas:master Apr 19, 2016
@dunglas
Copy link
Owner

dunglas commented Apr 19, 2016

Thank you!

@mfn mfn deleted the type-whitelist branch April 27, 2016 12:24
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants