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

[Feature] Use type hint for methods #282

Closed
sylfabre opened this issue Aug 9, 2019 · 4 comments · Fixed by #284
Closed

[Feature] Use type hint for methods #282

sylfabre opened this issue Aug 9, 2019 · 4 comments · Fixed by #284

Comments

@sylfabre
Copy link
Contributor

sylfabre commented Aug 9, 2019

Symfony4's auto-wiring feature requires type hint on constructors to work.

For instance \Intercom\IntercomUsers::__construct($client) should become \Intercom\IntercomUsers::__construct(IntercomClient $client).

@GabrielAnca
Copy link
Contributor

Hi @sylfabre 👋 thanks for opening this issue!

I am wondering why you would need to use the IntercomUsers class directly rather than $intercomClient->users.

The main reason for my question is because as far as I know, you wouldn't be able to autowire IntercomClient anyway because it needs your token, so even if we make IntercomUsers autowirable, you would still need to define the service for IntercomClient manually in your services.yaml file. Please correct me if I'm wrong, I didn't use Symfony a lot since version 4 was launched.

Those are just my questions about your use case. I think adding the types to the constructors would definitely be a good idea, especially now that all the current PHP versions support it. Feel free to create a PR for that if you have time for it 😃

@sylfabre
Copy link
Contributor Author

@GabrielAnca I use IntercomUsers class directly because it's easier to mock only this than both IntercomClient and IntercomUsers when I use phpunit.

Yes you're right, I still need to define the service because of the token. But with SF4 autowiring, I can only define scalar arguments of the constructor. So using type hinting makes me write less code

@sylfabre sylfabre mentioned this issue Aug 25, 2019
@sylfabre
Copy link
Contributor Author

@GabrielAnca here is my PR: #284

@GabrielAnca
Copy link
Contributor

Makes sense, thank you for sharing that! It helps us understand how the SDK is used and how it could be improved in the future 😃 And also thanks for the PR! I'll release a new version so you can leverage it immediately

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 a pull request may close this issue.

2 participants