phpList / phplist3

Fully functional Open Source email marketing manager for creating, sending, integrating, and analysing email campaigns and newsletters.
https://www.phplist.org
GNU Affero General Public License v3.0
734 stars 268 forks source link

Bouncemgt - allowing processing only existing bounces + a related new rule action #1028

Closed lwcorp closed 3 months ago

lwcorp commented 4 months ago

Description

  1. Added support for processing bounces without downloading new bounces (great for testing rules!)
  2. Added support for a new bounce rule action to undo automatic actions (this connects to point 1 when subscribers get mistakenly unconfirmed and you wish to test how not to do it again without the need to risk yet to be downloaded bounces)

Screenshots (if appropriate):

image image

lwcorp commented 4 months ago

It shouldn't have failed a syntax check - Named Arguments (which I've used as it "allows skipping default values arbitrarily") have been supported since at least PHP 8.0.0, while phpList requires 8.1.

See: image image

bramley commented 4 months ago

I have just seen the comment about requiring php 8.1 or later. That is not a hard-requirement and I still know of people using php 7. Using named parameters is going to make the page unavailable in these cases.

The only reference in the code to a required version seems to be php 5.3.

if (version_compare(PHP_VERSION, '5.3.3', '<') && WARN_ABOUT_PHP_SETTINGS) {
    Error(s('Your PHP version is out of date. phpList requires PHP version 5.3.3 or higher.'));
}
michield commented 4 months ago

Yes, I wonder if we need to at least require 7.4 or something, but even 7.4 is EOL.

In fact, 8.0 is EOL

lwcorp commented 4 months ago

Fine, I've made it version dependent. Users who use EOL versions will get hardcoded default values. If those values ever change, only users with non EOL versions will automatically get the new default values.

I have just seen the comment about requiring php 8.1 or later. That is not a hard-requirement

So please fix the statement in that link to mention this.

bramley commented 4 months ago

There is a syntax error in the latest change.

But the original statement is not how pageLink2() is intended to be called, although there are lots of instances of how that is done and maybe you copied one of those. The first parameter is the page name only, any further parameters for the URL should be in the $url parameter

PageLink2('processbounces', s('Reprocess Only Existing Bounces'), 'justexisting=true', false, s('Reprocess Only Existing Bounces'))

thus having only one hard-coded parameter to pageLink2(). There are many calls to PageLink2 that have to hard-code the third or fourth parameters in order to specify the $title parameter, so one more should not really be a problem. Having a version check with two alternate statements seems to be a worse solution as it has both the modern and old lines of code.

lwcorp commented 4 months ago

There is a syntax error in the latest change.

No, there wasn't, see discussion above - it's phpList's interpreter deciding to ignore v8+ commands even in v8+ mode. In any case, I don't know why would an old approach be better than a dual modern/old approach, but since it matters so much I've changed to just an old approach, thus no "syntax error" this time.

You can see function PageLink2's default values are blank for $url and false for $no_plugin. Since passing those values does no harm, I've kept them as such.

michield commented 4 months ago

I think this can be merged into the next release.

phpListDockerBot commented 3 months ago

This pull request has been mentioned on phpList Discuss. There might be relevant details there:

https://discuss.phplist.org/t/3-6-15-release-candidate-is-available-for-testing/9473/1

phpListDockerBot commented 3 months ago

This pull request has been mentioned on phpList Discuss. There might be relevant details there:

https://discuss.phplist.org/t/phplist-3-6-15-has-been-released/9495/1