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

Fix file_get_contents warning in ContextFactory #16

Merged
merged 2 commits into from
Mar 19, 2016

Conversation

dunglas
Copy link
Contributor

@dunglas dunglas commented Mar 1, 2016

Currently, a warning like warning: file_get_contents(): Filename cannot be empty in [...]/prophecy/vendor/phpdocumentor/type-resolver/src/Types/ContextFactory.php line 48 is generated by the library if there is no filename associated with the reflector passed to ContextFactory.

This PR fix this.

@stof
Copy link
Contributor

stof commented Mar 1, 2016

There is another case breaking: when the filename is eval'd content (or something like that, I don't remember how it looks exactly)

@stof
Copy link
Contributor

stof commented Mar 1, 2016

Your "fix" makes it impossible to read the docblock of such classes entirely though, even if they don't use aliases. So I'm not sure it is the appropriate fix

@dunglas
Copy link
Contributor Author

dunglas commented Mar 1, 2016

@stof what do you think of this alternative fix?

@stof
Copy link
Contributor

stof commented Mar 1, 2016

This is much better for the second issue. But I still think that the bug is still there for evaled code. you should add a test for it

@dunglas
Copy link
Contributor Author

dunglas commented Mar 1, 2016

@stof done

@dunglas dunglas force-pushed the fix_context_factory branch from 2a53fb4 to 73319e8 Compare March 7, 2016 20:20
@dunglas
Copy link
Contributor Author

dunglas commented Mar 7, 2016

I've pushed a better fix (use file_exists() instead of the previous hackish solution) and squashed commits.

@dunglas dunglas force-pushed the fix_context_factory branch from 73319e8 to 17156bc Compare March 7, 2016 20:22
@dunglas
Copy link
Contributor Author

dunglas commented Mar 17, 2016

ping @mvriel

It's a blocking bug for Prophecy integration (see phpspec/prophecy#264).

@theofidry
Copy link

👍

@mvriel
Copy link
Member

mvriel commented Mar 19, 2016

Hiya @dunglas,

Looks good, and thanks for including tests to verify this behaviour. I will merge it.

mvriel added a commit that referenced this pull request Mar 19, 2016
Fix file_get_contents warning in ContextFactory
@mvriel mvriel merged commit c3a7547 into phpDocumentor:master Mar 19, 2016
@dunglas dunglas deleted the fix_context_factory branch March 19, 2016 18:18
@dunglas
Copy link
Contributor Author

dunglas commented Mar 19, 2016

Thank you very much @mvriel

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.

4 participants