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

Mantis 18706 - Repeat message processing tries to copy an index #174

Merged
merged 1 commit into from May 7, 2017

Conversation

Projects
None yet
3 participants
@bramley
Contributor

bramley commented May 5, 2017

Avoid trying to copy indices when copying a message.
Explicitly set the finishsending date when copying a message, otherwise eventually a repeated campaign will not be sent because finishsending is in the past.

Avoid trying to copy indices when copying a message.
Explicitly set the finishsending date when copying a message.
@@ -1907,6 +1908,16 @@ function repeatMessage($msgid)
unset($e['second']);
setMessageData($newid, 'embargo', $e);
$finishSending = time() + DEFAULT_MESSAGEAGE;
$finish = array(

This comment has been minimized.

@oliverklee

oliverklee May 5, 2017

Contributor

I find both variable names very similar, and the different names don't make the differences clear. Proposal: finishTimestamp and finishDateParts.

What do you think?

This comment has been minimized.

@bramley

bramley May 5, 2017

Contributor

I think it is clear that $finish is an array of parts of dates. To understand why it is like that you need to look into setMessageData(). I'm happy with the way it is.

This comment has been minimized.

@oliverklee

oliverklee May 5, 2017

Contributor

I'm just trying to help your PR get into the code base in its best, clearest, most readable form. If you'd prefer to not get these suggestions, please let me know.

$columnsToCopy = array_diff(
array_keys($DBstruct['message']),
array(
'id', 'entered', 'modified', 'embargo', 'status', 'sent', 'processed', 'astext', 'ashtml',
'astextandhtml', 'aspdf', 'astextandpdf', 'viewed', 'bouncecount', 'sendstart', 'uuid',
)
);
$columnsToCopy = preg_grep('/^index_/', $columnsToCopy, PREG_GREP_INVERT);

This comment has been minimized.

@oliverklee

oliverklee May 5, 2017

Contributor

I propose changing the old variable name in line 1874 to something different as it now does not represent the columns to copy anymore, but more columns.

This comment has been minimized.

@bramley

bramley May 5, 2017

Contributor

The variable has a "lifetime" of 10 lines, so it is not difficult to work out how it is being used. I am happy the way it is.

@michield michield merged commit 534c70a into phpList:master May 7, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@bramley bramley deleted the bramley:Mantis_18706 branch May 12, 2017

samtuke added a commit to samtuke/phplist3 that referenced this pull request Nov 21, 2017

Avoid trying to copy indices when copying a message. (phpList#174)
Explicitly set the finishsending date when copying a message.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment