Skip to content

Fix/goodreads-timeout#13595

Merged
kraftbj merged 1 commit intoAutomattic:masterfrom
rakmob42:fix/goodreads-timeout
Oct 1, 2019
Merged

Fix/goodreads-timeout#13595
kraftbj merged 1 commit intoAutomattic:masterfrom
rakmob42:fix/goodreads-timeout

Conversation

@rakmob42
Copy link
Copy Markdown

@rakmob42 rakmob42 commented Sep 29, 2019

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

@rakmob42 rakmob42 requested review from a team and jeherve September 29, 2019 04:16
@rakmob42 rakmob42 self-assigned this Sep 29, 2019
@matticbot
Copy link
Copy Markdown
Contributor

matticbot commented Sep 29, 2019

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!

@rakmob42
Copy link
Copy Markdown
Author

rakmob42 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

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

jetpackbot commented Sep 29, 2019

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

@rakmob42
Copy link
Copy Markdown
Author

rakmob42 commented Sep 29, 2019

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

@rakmob42 rakmob42 added the [Status] Needs Review This PR is ready for review. label Sep 29, 2019
@jeherve jeherve added 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
$url, array(
'httpversion' => '1.1',
'timeout' => 3,
'timeout' => 10,
Copy link
Copy Markdown
Member

@jeherve jeherve Sep 30, 2019

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
Copy Markdown
Author

@rakmob42 rakmob42 Sep 30, 2019

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
Copy Markdown
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 need more details from you. This label will be auto-added until the PR meets all requirements. [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review This PR is ready for review. [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. 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
Copy Markdown
Contributor

kraftbj commented Oct 1, 2019

r197275-wpcom

@rakmob42 rakmob42 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

@rakmob42 rakmob42

Labels

Bug When a feature is broken and / or not performing as intended [Feature] Extra Sidebar Widgets Touches WP.com Files

Projects

None yet

Milestone

7.9

Development

Successfully merging this pull request may close these issues.

5 participants

@rakmob42 @matticbot @jetpackbot @kraftbj @jeherve