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

Add docblock properties #543

Closed
wants to merge 2 commits into from
Closed

Add docblock properties #543

wants to merge 2 commits into from

Conversation

spaze
Copy link
Contributor

@spaze spaze commented Oct 31, 2018

Hi everyone,

this adds two properties that are only used when updating objects:

Thanks!

@remi-stripe
Copy link
Contributor

@spaze Thanks for opening a PR! I don't think we should do this as the docblocks are here to surface the properties on the resource itself and not the possible parameters. Since there's no way to flag which one is a property and which one is a parameter, I think it would end up being confusing, even if you can set $customer->coupon it's really only used for the save() method.

Let me know what you think!

@ob-stripe
Copy link
Contributor

I agree with @remi-stripe here -- those two aren't request parameters, not resource properties.

I don't think we even have a good way of documenting request parameters since in most cases the methods are simply inherited from the common traits.

@spaze
Copy link
Contributor Author

spaze commented Oct 31, 2018

I understand your point of view and I'm not sure this would be the right way either.

But I'm running a static analysis (using PHPStan) and it flags property access like $subscription->coupon for obvious reasons. I'm aware these are used only for save() but technically they're set on the resource object, and as I couldn't figure out any nicer way of resolving the static analysis issues so I've submitted the PR.

As a compromise, I could add a comment like

 * @property string $source Used for save() only

which might help distinguish these from the others.

@ob-stripe
Copy link
Contributor

Ah, that's a good point. I think that's a reasonable compromise. To be super clear, can you change it like this?

 * @property string $source Used for save() only, not returned on resources.

@spaze
Copy link
Contributor Author

spaze commented Oct 31, 2018

Of course! :-) Changed.

@remi-stripe
Copy link
Contributor

I'm still wary of doing this change. We would have to go and do this for every single resource since most have something similar and then maintain it over time manually since none of this is automated today. This has been maintained mostly thanks to @nickdnk doing all that work.

@nickdnk
Copy link
Contributor

nickdnk commented Oct 31, 2018

Thanks for the creds, @remi-stripe.

I think this is a bad idea for a few reasons: One is that we have to do it everywhere to be consistent, as remi says, another is that this indicates to people who use the SDK that they can read source or coupon from these objects when returned from the API. Also nowhere else do we have comments on the phpdoc blocks, so that's also a thing to consider.

All the current phpdocs match 1:1 to "The {object} object"-sections in the API. In this case the The customer object or The subscription object sections.

Edit: @spaze Maybe you can silence the errors if you use $object['coupon'] notation instead of $object->coupon? I don't know if that works though, but it should.

@spaze
Copy link
Contributor Author

spaze commented Oct 31, 2018

I've created a simple extension for PHPStan that will emulate those undefined properties so they won't be flagged anymore. I'll add more properties to the extension once I'll find them.

This can be closed then. Thanks everyone!

@spaze spaze closed this Oct 31, 2018
@remi-stripe
Copy link
Contributor

@spaze Awesome, thanks for sharing that related project too!

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.

None yet

4 participants