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

[PropertyInfo] added handling of nullable types in PhpDoc #27618

Merged
merged 1 commit into from Jul 2, 2018
Merged

[PropertyInfo] added handling of nullable types in PhpDoc #27618

merged 1 commit into from Jul 2, 2018

Conversation

oxan
Copy link
Contributor

@oxan oxan commented Jun 17, 2018

While not specified in PSR-5, PhpDocumentor does support parsing nullable types in the PHP 7.1 syntax (i.e. ?string), and returns those in a Nullable wrapper type. We currently don't handle this and neither throw an error, which results in all kind of weird breakage when this syntax is used (e.g. "class string|int not found").

Correctly parse this syntax into a nullable type.

Q A
Branch? 3.4
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets n/a
License MIT
Doc PR n/a

@oxan
Copy link
Contributor Author

oxan commented Jun 17, 2018

Okay, so the tests fail because phpdocumentor/typeresolver only parses nullable types from v0.3.0 onwards, and we require v0.2.1+. Should I bump the dependency or make the tests conditional?

@nicolas-grekas
Copy link
Member

Given 0.3 supports 5.5, which is the minimum version of Symfony 3.4, I think it's fine bumping.

@nicolas-grekas nicolas-grekas added this to the 3.4 milestone Jun 17, 2018
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

bump pending :)

While not specified in PSR-5, PhpDocumentor does support parsing
nullable types in the PHP 7.1 syntax (i.e. ?string), and returns those
in a Nullable wrapper. We currently don't handle this and neither throw
an error, which results in all kind of weird breakage when this syntax
is used (e.g. "class string|int not found").

Correctly parse this syntax into a nullable type.
@oxan
Copy link
Contributor Author

oxan commented Jul 1, 2018

@nicolas-grekas I fixed the dependency (& tests are working now) :)

Copy link
Member

@dunglas dunglas left a comment

Choose a reason for hiding this comment

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

Nice one, thanks!

@fabpot
Copy link
Member

fabpot commented Jul 2, 2018

Thank you @oxan.

@fabpot fabpot merged commit 38b369b into symfony:3.4 Jul 2, 2018
fabpot added a commit that referenced this pull request Jul 2, 2018
…(oxan)

This PR was merged into the 3.4 branch.

Discussion
----------

[PropertyInfo] added handling of nullable types in PhpDoc

While not specified in PSR-5, PhpDocumentor does support parsing nullable types in the PHP 7.1 syntax (i.e. `?string`), and returns those in a `Nullable` wrapper type. We currently don't handle this and neither throw an error, which results in all kind of weird breakage when this syntax is used (e.g. "class string|int not found").

Correctly parse this syntax into a nullable type.

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | n/a
| License       | MIT
| Doc PR        | n/a

Commits
-------

38b369b [PropertyInfo] added handling of nullable types in PhpDoc
@oxan oxan deleted the propertyinfo-fix-nullable-phpdoc branch July 2, 2018 16:52
This was referenced Jul 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants