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

Update management props #378

Merged
merged 4 commits into from
Oct 15, 2019
Merged

Update management props #378

merged 4 commits into from
Oct 15, 2019

Conversation

joshcanhelp
Copy link
Contributor

Changes

This is a breaking change for Management API functionality.

Management class properties have been changed from public to private access. Instead of using the class property to access the endpoint functionality, you'll now use a method with the same (or very similar) name as the property.

You can see these changes in some of the tests below but, for clarity, if you were getting users like so:

$m_api = new Management( $api_token, $domain );
$results = $m_api->users->getAll();

... it would now be:

$m_api = new Management( $api_token, $domain );
$results = $m_api->users()->getAll();

Other changes:

  • Management constructor declares parameter types
  • Endpoint methods declare return types
  • Removed unused protected setApiClient() method

References

#363 - Deprecation PR for properties

Testing

  • This change modifies test coverage (removes the property type check)
  • This change has been tested on PHP 7.1

Checklist

  • All existing and new tests complete without errors.
  • The correct base branch is being used.

@joshcanhelp joshcanhelp requested a review from a team October 15, 2019 15:23
@joshcanhelp joshcanhelp added this to the 7.0.0 milestone Oct 15, 2019
@damieng
Copy link
Contributor

damieng commented Oct 15, 2019

What's the underlying rationale behind moving from properties to methods? (Our .NET management API uses properties)

@joshcanhelp
Copy link
Contributor Author

@damieng - Properties could be accidentally set to something else and all endpoints are loaded at once, even if you just use one.

@damieng
Copy link
Contributor

damieng commented Oct 15, 2019

PHP doesn't let you do read-only properties or delayed loading? I guess they're more like instance variables than properties?

@joshcanhelp
Copy link
Contributor Author

PHP doesn't let you do read-only properties?

Not really. You can define a a magic __get method and look for the property but ... methods work a little better. Would be boilerplate for either way.

... or delayed loading?

This package uses an auto-loader so, yes, but you have to instantiate individual classes. This Management class is the "router" of sorts so would have to re-architect a bit.

@joshcanhelp joshcanhelp merged commit ee4e8b1 into 7.0.0-dev Oct 15, 2019
@joshcanhelp joshcanhelp deleted the update-management-props branch October 15, 2019 16:38
@github-actions
Copy link
Contributor

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants