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

PHP suggestions #2440

Closed
troublete opened this issue Dec 4, 2016 · 5 comments
Closed

PHP suggestions #2440

troublete opened this issue Dec 4, 2016 · 5 comments
Assignees
Labels

Comments

@troublete
Copy link

Good afternoon,

I now have explored the PHP library of protobuf a good bit and even though I realize it is still in Beta state I would like to make two suggestions that could improve the library.

  • The code generator for PHP IS ALREADY created composer conform. So it doesn't really make sense that the classes generated defined with imports create require_once statements, and they even break the autoloading completely when the generated classes get included in the autoloading via composer. My suggestions here would be to use use statements instead which will then just include the full qualified namespace of the imported proto file.

  • Secondly: even though the internal library has a nearly completlely covering error handling, it would be more appropriate to change the current behaviour of throwing errors to throwing exceptions (especially in the check functions). Since they can be handled way more proper in PHP applications.

Else it is pretty neat! Some inconssistencies with the API but overall nice to use. :)

@troublete
Copy link
Author

One more thing packagist has the wrong (too old) commit registered for dev-master causing issues like #2359 when one just requires google/protobuf in composer.json.

@TeBoring
Copy link
Contributor

TeBoring commented Dec 7, 2016

#2435 change the generated code to conform PSR-4. You can use psr-4 autoload in composer to load generated files. Does that fix your first suggestion?

@TeBoring
Copy link
Contributor

TeBoring commented Dec 7, 2016

For the second suggestion, we will consider that in future release. But in the coming release, which is very soon, we will still keep the existing behavior.

@troublete
Copy link
Author

1: By description of #2267 that it is supposed to fix, I assume yes it'll fix my problem. Will check when I get home though to confirm. 2: Great thank you so much! :)

@TeBoring TeBoring self-assigned this Mar 7, 2017
@troublete
Copy link
Author

I'll close this since I assume the bug is fixed, didn't have time to try yet. If the issue still occurs, I'll just reopen.

Thanks for the feedback. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants