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.

Sign up for GitHub

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

Jump to bottom

Adding new managed WordPress hosts to be identified in class-functions.php. #14213

Merged
merged 9 commits into from
Jan 15, 2020
Merged

Adding new managed WordPress hosts to be identified in class-functions.php. #14213

merged 9 commits into from
Jan 15, 2020

Conversation

oikeme
Copy link
Contributor

@oikeme oikeme commented Dec 12, 2019 •
edited
Loading

Adding more hosting providers to get_hosting_provider() function to be able to identify managed WordPress hosts.

Fixes N/A

Changes proposed in this Pull Request:

  • Adding options to identify new hosts specifically Managed WordPress hosts.

Is this a new feature or does it add/remove features to an existing part of Jetpack?

  • New Feature pbtFPC-1M-p2

Testing instructions:

  • Unit Tests: phpunit --filter=WP_Test_Jetpack_Sync_Functions

Proposed changelog entry for your changes:

  • Add more managed WordPress hosts to hosting provider detection.

@oikeme
Update class-functions
1b6daa2 
Adding more hosting providers to get_hosting_provider() function to be able to identify managed WordPress hosts.
@oikeme oikeme requested a review from a team December 12, 2019 02:13
@oikeme oikeme self-assigned this Dec 12, 2019
@oikeme oikeme requested a review from ebinnion December 12, 2019 02:14
@jetpackbot
Copy link

jetpackbot commented Dec 12, 2019 •
edited
Loading

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Scheduled Jetpack release: January 14, 2020.
Scheduled code freeze: January 7, 2020

Generated by 🚫 dangerJS against 0a16fbf

@jeherve jeherve added [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Package] Sync labels Dec 12, 2019
@oikeme
Update class-functions.php
505170d 
Update get_hosting_provider() function to use an array of hosting provider constants to check the hosting provider
@oikeme
Update class-functions.php
0b82986 
Adding in secondary check for class_exists for gd-managed-wp
@oikeme oikeme changed the title Update class-functions Adding new managed WordPress hosts to be identified in class-functions.php. Dec 13, 2019
@oikeme
Update class-functions.php
a47b413 
Update variable name when returning the constant value to $hosting_provider
@oikeme
Update hosting provider check for managed WP and adding tests
cce2bae 
Update checking for managed WP hosts and adding tests for get_hosting_provider() function check.
@oikeme oikeme requested a review from a team as a code owner December 17, 2019 16:19
@ebinnion
Copy link
Contributor

This is looking pretty good to me. I agree with @tyxla's feedback on the the tests.

Besides that, I wouldn't worry much about the Code Climate issue. It seems like it doesn't like the length of the method, but that may be difficult to fix and frankly, the method will continue to grow as we we add hosts in the future. I would suggest updating/removing the functional testing instructions since you've added test coverage.

@oikeme
Update class-functions.php
80f1e5b 
Splitting out hosting provider detection in to detection by specific types in separate functions which are now called by the main get_hosting_provider() function
@oikeme
Update Tests for get_hosting_provider
317edad 
Updating class functions to be non static and adding test for get_hosting_provider() for each method of getting a hosting provider
@oikeme
Update unit tests and fix get_hosting_provider() return
eea525c 
Updating the unit tests for get_hosting_provider() to be separate tests for each method that function calls and a test for unknown hosting providers from the get_hosting_provider() callable
ebinnion
ebinnion previously approved these changes Jan 8, 2020
View reviewed changes
Copy link
Contributor

@ebinnion ebinnion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM. But, I'd like to get one other set of eyes on it as well.

@oikeme
Copy link
Contributor Author

oikeme commented Jan 8, 2020

@tyxla I've made some changes to this patch since your suggestion which was great by the way. Do you mind giving it a look over again? Thanks!

tyxla
tyxla previously approved these changes Jan 10, 2020
View reviewed changes
Copy link
Member

@tyxla tyxla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good from my perspective 👍

I'd only suggest against the way that switch(true) is used, but I don't have super strong feelings if y'all prefer it to be that way.

packages/sync/src/class-functions.php Outdated Show resolved Hide resolved
packages/sync/src/class-functions.php Show resolved Hide resolved
packages/sync/src/class-functions.php Show resolved Hide resolved
@oikeme
Update class-functions.php
0a16fbf 
Update function get_hosting_provider_by_known_constant() to loop through known hosting provider constants in place of a switch
@oikeme oikeme dismissed stale reviews from tyxla and ebinnion via 0a16fbf January 10, 2020 22:52
ebinnion
ebinnion approved these changes Jan 10, 2020
View reviewed changes
Copy link
Contributor

@ebinnion ebinnion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left a couple of comments, but they're not blockers. I like the change to use an array instead of a case for checking constants. It seems to have simplified the code quite a bit.

I think it's fine to not make the same change to the other methods for now at least. Instead, we can make that change if/when necessary, as we add more functions and classes to check.

packages/sync/src/class-functions.php Show resolved Hide resolved
packages/sync/src/class-functions.php Show resolved Hide resolved
@oikeme oikeme requested a review from ebinnion January 13, 2020 17:48
@oikeme
Copy link
Contributor Author

oikeme commented Jan 13, 2020

@tyxla I had a comment from you in an email earlier today about an issue I think may have been resolved. Can you confirm?

@oikeme oikeme added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] In Progress labels Jan 13, 2020
@jeherve jeherve added this to the 8.2 milestone Jan 14, 2020
@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Jan 14, 2020
jeherve
jeherve approved these changes Jan 14, 2020
View reviewed changes
Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me! 👍

@tyxla
Copy link
Member

tyxla commented Jan 14, 2020

I can see some of the comments were marked as resolved but not addressed, but they were also not blockers. So feel free to 🚢 if it's all intended 👍

@oikeme oikeme merged commit b0aa938 into master Jan 15, 2020
@oikeme oikeme deleted the update/add-managed-wp-hosts branch January 15, 2020 13:11
@matticbot matticbot added [Status] Needs Changelog [Status] Needs Package Release This PR made changes to a package. Let's update that package now. and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Jan 15, 2020
@jeherve jeherve added [Status] Has Changelog and removed [Status] Needs Package Release This PR made changes to a package. Let's update that package now. [Status] Needs Changelog labels Jan 17, 2020
jeherve added a commit that referenced this pull request Jan 17, 2020
@jeherve
Changelog: add #14213
c9e6ff3
jeherve added a commit that referenced this pull request Jan 28, 2020
@jeherve
Changelog: 8.2 release (#14332)
0589a49 
* [not verified] Remove empty readme section

* Initial changelog for 8.2

* Changelog: add #14220

* Changelog: add #14252

* Changelog: add #14291

* Changelog: add #14309

* Changelog: add #14304

* Changelog: add general connection log.

* Changelog: add #14275

* Changelog: add #14313

* Changelog: add #14213

* Changelog: add #14357

* Add sync testing instructions

* Add 8.1.1 changelog back

See eeaafab and 61757eb

* Changelog: add #14371

* Changelog: add #14386

* Changelog: add #14471

* Changelog: add #14325

* Changelog: add #14194

* Changelog: add #14340

* Changelog: add #14418

* Changelog: add #14417

* Changelog: add #14075

* Changelog: add #14467

* Changelog: add #14307

* Changelog: add #14326
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tyxla tyxla tyxla left review comments

@ebinnion ebinnion ebinnion approved these changes

@jeherve jeherve jeherve approved these changes

Assignees

@oikeme oikeme

Labels
[Package] Sync [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it
Projects
None yet
Milestone
8.2
Development

Successfully merging this pull request may close these issues.
6 participants
@oikeme @jetpackbot @ebinnion @tyxla @jeherve @matticbot