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 Queue.php #7771

Closed
wants to merge 1 commit into from
Closed

Update Queue.php #7771

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Dec 13, 2016

These 2 items were backwards. After spending hours troubleshooting why my newsletter was being rejected by my mail host i tracked it back to this.

These 2 items were backwards.  After spending hours troubleshooting why my newsletter was being rejected by my mail host i tracked it back to this.
@magento-cicd2
Copy link
Contributor

magento-cicd2 commented Dec 13, 2016

CLA assistant check
All committers have signed the CLA.

@ihor-sviziev
Copy link
Contributor

ihor-sviziev commented Dec 13, 2016

@cbarsness Would be great to cover this case with unit test in order to prevent such situations in future

@wert2all wert2all self-requested a review December 26, 2016 12:02
@wert2all wert2all self-assigned this Dec 26, 2016
@okorshenko
Copy link
Contributor

Closing this pull request. Source branch was removed

@okorshenko okorshenko closed this Feb 27, 2017
@okorshenko okorshenko added this to the February 2017 milestone Feb 27, 2017
@orlangur
Copy link
Contributor

@okorshenko why?

It looks like bug is still present in develop: https://github.com/magento/magento2/blob/develop/app/code/Magento/Newsletter/Model/Queue.php#L239

Changes can be obtained with git fetch mainline pull/7771/head:pr7771, there is no matter whether source branch exists or not.

Of course, probability that PR author will make any changes if it is required is low when source branch is removed, my two cents is that it's better to pick and process good PR in its current state than just close.

@hostep
Copy link
Contributor

hostep commented Mar 27, 2017

Looks like this was recently fixed by #6354

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants