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

Related Posts: Define array before foreach loop #8147

Merged
merged 1 commit into from
Nov 13, 2017
Merged

Related Posts: Define array before foreach loop #8147

merged 1 commit into from
Nov 13, 2017

Conversation

Umangvaghela
Copy link
Contributor

@Umangvaghela Umangvaghela commented Nov 11, 2017 •
edited
Loading

fix #8030

The excluded_post_ids array is getting reset each time in the foreach loop, so it is good to define excluded_post_ids array before foreach loop then we overcome the issue.

Thanks

@Umangvaghela Umangvaghela requested a review from a team as a code owner November 11, 2017 06:19
@Umangvaghela
Copy link
Contributor Author

@zinigor

Sir , When you get a time so please review it and give your feedback if i miss something.

Thanks

@jeherve jeherve changed the title Define array before foreach loop Related Posts: Define array before foreach loop Nov 13, 2017
@jeherve
Copy link
Member

jeherve commented Nov 13, 2017

Thanks for the PR. Quick remark: you do not need to ping specific people to get a review on your PRs. They will all be reviewed!

@jeherve jeherve added [Feature] Related Posts [Pri] Normal [Status] Needs Review This PR is ready for review. [Type] Bug When a feature is broken and / or not performing as intended labels Nov 13, 2017
zinigor
zinigor approved these changes Nov 13, 2017
View reviewed changes
Copy link
Member

@zinigor zinigor left a comment

Choose a reason for hiding this comment

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

Definitely makes sense to define the array outside of the foreach loop :) Thank you!

@zinigor zinigor added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review This PR is ready for review. labels Nov 13, 2017
oskosk
oskosk approved these changes Nov 13, 2017
View reviewed changes
Copy link
Contributor

@oskosk oskosk left a comment

Choose a reason for hiding this comment

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

Thanks for the fix @Umangvaghela . LGTM!

@oskosk oskosk merged commit 38d2498 into Automattic:master Nov 13, 2017
jeherve added a commit that referenced this pull request Nov 24, 2017
@jeherve
Changelog: add #8147
1102173
oskosk pushed a commit that referenced this pull request Nov 28, 2017
@jeherve @oskosk
Changelog 5.6: running changelog. (#8192)
7844644 
* Changelog 5.6: create base for changelog.

* Update changelog with 5.5.1 info.

* Changelog: add #7930 and #8238

* Changelog: add #8076

* Changelog: add #8100

* Changelog: add #8117

* Changelog: add #8141

* Changelog: add #8143

* Changelog: add #8147

* Changelog: add #8149

* Changelog: add #8153

* Changelog: add #8173

* Changelog: add #8184

* Changelog: add #8196

* Changelog: add #8199

* Changelog: add #8093

* Changelog: add #8171

* Changelog: add #8182

* Changelog: add #8202, #8222

* Changelog: add #8228

* Changelog: add #8240

* Changelog: add #8251

* remove AL card change
@kraftbj kraftbj removed the [Status] Ready to Merge Go ahead, you can push that green button! label Oct 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zinigor zinigor zinigor approved these changes

@oskosk oskosk oskosk approved these changes

Assignees
No one assigned
Labels
[Feature] Related Posts [Pri] Normal Touches WP.com Files [Type] Bug When a feature is broken and / or not performing as intended
Projects
None yet
Milestone
No milestone
6 participants
@Umangvaghela @jeherve @zinigor @oskosk @kraftbj @matticbot