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

Fix/goodreads-timeout #13595

Merged
merged 1 commit into from
Oct 1, 2019
Merged

Fix/goodreads-timeout #13595

merged 1 commit into from
Oct 1, 2019

Conversation

wigglemuff
Copy link
Contributor

Changes proposed in this Pull Request:

Testing instructions:

  • Go to 'widgets' page, add goodreads widget
  • Test with this test ID: 101233110. It works fine with the previous code containing timeout => 3
  • Testing with this user ID: 3846340 previously failed but it works now with timeout => 10 change in the code.

Proposed changelog entry for your changes:

  • Fixes timeout issues with Goodreads widget

@wigglemuff wigglemuff requested a review from a team as a code owner September 29, 2019 04:16
@wigglemuff wigglemuff self-assigned this Sep 29, 2019
@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Hello codestor4! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer, review, and approve D33318-code before merging this PR. Thank you!

@wigglemuff
Copy link
Contributor Author

wigglemuff commented Sep 29, 2019 •
edited
Loading

Umm.. I'm not sure what happened there. Github is showing me all my previous commits in this PR. I only wish to add increase timeout to 10 commit in this PR :/

Edit: fixing this now such that it only includes my latest commit ..
Edit2: YAY I fixed it! with -->

git rebase master
git rebase --skip
git cherry -v master
git push <remote> <branch> -f

@wigglemuff wigglemuff changed the title Fix/goodreads timeout Fix/goodreads-timeout Sep 29, 2019
@jetpackbot
Copy link

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: October 1, 2019.
Scheduled code freeze: September 24, 2019

Generated by 🚫 dangerJS against 4601823

@wigglemuff
Copy link
Contributor Author

Fixed the issues by removing unwanted commits and now this is ready for review!

@wigglemuff wigglemuff added the [Status] Needs Review To request a review from Crew. Label will be renamed soon. label Sep 29, 2019
@jeherve jeherve added [Type] Bug When a feature is broken and / or not performing as intended [Feature] Extra Sidebar Widgets and removed [Status] In Progress labels Sep 30, 2019
jeherve
jeherve reviewed Sep 30, 2019
View reviewed changes
modules/widgets/goodreads.php
@@ -95,7 +95,7 @@ function goodreads_user_id_exists( $user_id ) {
$response = wp_remote_head(
$url, array(
'httpversion' => '1.1',
'timeout' => 3,
'timeout' => 10,
Copy link
Member

Choose a reason for hiding this comment

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

Any specific reason why you picked 10, and not a higher or lower number?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IIRC it worked with something like 6 or 7 but I decided to go with 10 (which was also suggested in the jpop-issue) because I think it allows room for other cases which need a higher timeout.

In my head, I'm trying to apply the same logic as dynamic arrays. When the array is full, it's better to double the size to avoid frequent changes.

jeherve
jeherve reviewed Sep 30, 2019
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.

Quick question.

@jeherve jeherve added this to the 7.9 milestone Sep 30, 2019
@jeherve jeherve added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Sep 30, 2019
@kraftbj kraftbj merged commit a1b6528 into Automattic:master Oct 1, 2019
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Oct 1, 2019
@kraftbj
Copy link
Contributor

kraftbj commented Oct 1, 2019

r197275-wpcom

@wigglemuff wigglemuff deleted the fix/goodreads-timeout branch October 2, 2019 11:30
jeherve added a commit that referenced this pull request Oct 23, 2019
@jeherve
Changelog: add #13595
bdfb7aa
jeherve added a commit that referenced this pull request Oct 29, 2019
@jeherve
7.9: Changelog (#13608)
6cd1621 
* 7.9: Changelog

* Update version number

* Update stable tag and tested up to

* Changelog: add #13530

* changelog: add #13578

* Changelog: add #13598

* Changelog: add entry for numerous block preview changes

* Changelog: add #13599

* changelog: add #13541

* Changelog: add #13542

* Changelog: add #13331

* Changelog: add #13558

* Changelog: add #13409

* Changelog: add #13582

* Changelog: add #13600

* Changelog: add #13601

* Changelog: add #13595

* Changelog: add #12695

* Changelog: add #13009

* Changelog: add #13649

* Changelog: add #13450

* Changelog: add #13507

* Changelog: add #13658

* Changelog: add #13687

* changelog: add #13683

* Changelog: add #9323

* Changelog: add #13681

* Fix typos in readme

* Add link to WordPress Beta Tester plugin

* Changelog: add #13630

* Changelog: add #13695

* Changelog: add #13659

* Changelog: add #13716

* Changelog: add #13664

* Changelog: add #13682

* Changelog: add #13362

* Changelog: add #13563

* Add testing list for #13563

* Changelog: add #13735

* Changelog: add #13752

* Changelog: add #13624

* Changelog: add #13756

* Changelog: add #13745

* Changelog: add #13728

* Changelog: add #13779

* Changelog: add #13699

* Changelog: add #13804

* Changelog: add #13761

* Changelog: add #13637

* Changelog: add #13517

* Changelog: add #13521

* Changelog: add #13729

* Testing list: add testing instructions for #13729

* Changelog: add sync changes

* Changelog: add #13807

* Changelog: add #13654

* Changelog: add #13795

* Changelog: add #13801

* Changelog: add #13818

* Changelog: add #13725

* Changelog: add #13831

* Changelog: add #13516

* Testing list: add Twenty Twenty instructions

* Changelog: add #13799

* Changelog: add #13805

* Changelog: add #13688

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

@kraftbj kraftbj kraftbj approved these changes

@jeherve jeherve jeherve approved these changes

Assignees

@wigglemuff wigglemuff

Labels
[Feature] Extra Sidebar Widgets Touches WP.com Files [Type] Bug When a feature is broken and / or not performing as intended
Projects
None yet
Milestone
7.9
Development

Successfully merging this pull request may close these issues.
5 participants
@wigglemuff @matticbot @jetpackbot @kraftbj @jeherve