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

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

Merged
merged 7 commits into from
Mar 28, 2024

Conversation

lwcorp
Copy link
Contributor

@lwcorp lwcorp commented Mar 5, 2024

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 added 4 commits March 5, 2024 21:59
Added &justexisting=true
1. Added support for &justexisting=true
1. Added support for new bounce rule action
Added support for new bounce action
Added non default title (otherwise it takes the wrong one)
@lwcorp lwcorp changed the title Bouncemgt Bouncemgt - allowing processing only existing bounces + a related new rule action Mar 5, 2024
@lwcorp
Copy link
Contributor Author

lwcorp commented Mar 5, 2024

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

public_html/lists/admin/bouncemgt.php Outdated Show resolved Hide resolved
public_html/lists/admin/processbounces.php Outdated Show resolved Hide resolved
public_html/lists/admin/processbounces.php Outdated Show resolved Hide resolved
1. Replaced goto with if-else
1. Hardcoded "-1" instead of supplying it in a sprintf value
@bramley
Copy link
Contributor

bramley commented Mar 10, 2024

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
Copy link
Member

michield commented Mar 10, 2024

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
Copy link
Contributor Author

lwcorp commented Mar 11, 2024

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
Copy link
Contributor

bramley commented Mar 11, 2024

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
Copy link
Contributor Author

lwcorp commented Mar 11, 2024

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
Copy link
Member

I think this can be merged into the next release.

@marianaballa marianaballa changed the base branch from main to release-3.6.15 March 27, 2024 15:38
@marianaballa marianaballa merged commit 3022f16 into phpList:release-3.6.15 Mar 28, 2024
4 of 6 checks passed
@lwcorp lwcorp deleted the bouncemgt branch March 28, 2024 17:56
@phpListDockerBot
Copy link
Contributor

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
Copy link
Contributor

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

marianaballa added a commit that referenced this pull request Apr 26, 2024
* Translations for 3.6.15  (#1032)

* Translated using Weblate (English)

Currently translated at 91.4% (1950 of 2132 strings)

Translation: phpList/phpList3
Translate-URL: http://translate.phplist.org/projects/phplist/phplist3/en/

* Translated using Weblate (French)

Currently translated at 99.8% (2128 of 2132 strings)

Translation: phpList/phpList3
Translate-URL: http://translate.phplist.org/projects/phplist/phplist3/fr/

---------

Co-authored-by: Duncan Cameron <[email protected]>
Co-authored-by: Alain Rihs <[email protected]>

* Support for indicating and getting feedback for e-mail test messages (#1031)

* Update sendemaillib.php

1. Appended a test subject indicator to test messages
1. Added a reply-to address to test messages that have no manual reply-to: using the logged in admin's address or at least the general admin's

* Update sendemaillib.php

Rephrased variable name

* Update sendemaillib.php

Switched to using $admin_auth

* Allowing subscribers to be filtered by confirmed and/or blacklisted (#1030)

* Update users.php

Allowed to filter by confirmed and/or non blacklisted - and not just by unconfirmed and/or blacklisted

* Changed users to subscribers

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

* Update bouncemgt.php

Added &justexisting=true

* Update processbounces.php

1. Added support for &justexisting=true
1. Added support for new bounce rule action

* Update lib.php

Added support for new bounce action

* Update bouncemgt.php

Added non default title (otherwise it takes the wrong one)

* Update processbounces.php

1. Replaced goto with if-else
1. Hardcoded "-1" instead of supplying it in a sprintf value

* Hardcoding defaults for older PHP versions

* Removed modern solution

* Update Common plugin and Segment plugin (#1024)

* Define timestamp columns explicitly (#1019)

* Define timestamp fields explicitly to avoid problem with the mysql setting explicit_defaults_for_timestamp

* Remove setting of timestamp fields that are automatically updated

* update CI to remove old PHP versions and add 8.3 (#1004)

* Escape single quote in error message (#1003)

* Allow ajax page links to have a title, defaulting to the link description (#1002)

Fixes #996

* Update CONTRIBUTING.md (#994)

Removed obsolete references

* update UUID class to the latest upstream (#990)

* update UUID class to the latest upstream

* clean up old files

* use the list order, even when grouping by category (#1025)

* restore ability to create other super users (#1014)

* restore ability to create other super users

* correctly initialise the privileges array

* Bounces' subscriber' status indicator + allowing to confirm right from bounces (#1029)

* Update listbounces.php

Added support for confirmed/blacklisted indicator

* Update bounces.php

Added confirmed/blacklisted indicator

* Update bounce.php

1. Added confirmed/blacklisted indicator
1. Added support for confirming user from a bounce

* Update bounce.php

1. Avoided ternary if because translation system doesn't support it
1. Used the newer s() function

* Update listbounces.php

Added curly brackets

* Used potential translation

* Php8fixes 202401 (#1026)

* remove deprecated ini_set call

* stop possible warning

* avoid warning

* avoid warning

* cast to int

* avoid warning on existing being null

* force template to be an integer

* suppress warnings

* check on valid var and cast to int

* give buttons an ID, so they can be targetted with testing

* avoid warning on empty array index

* add notification by email when an admin logs in from a new IP address. (#1027)

* add notification by email when an admin logs in from a new IP address.

* check IP per admin

* force columns to be not null

* prevent blocking login on an non-upgraded system and send login alert just to admin, or superuser

* keep newlines in translation as they are

* make shorter lines, so it renders a bit better

* Remove redundant upgrade steps (#1020)

* Remove steps that are unnecessary due to the 3.2.0 being the minimum upgrade version

* Keep silent when there are no subscriber UUIDs to generate

* Remove other unnecessary upgrade steps

---------

Co-authored-by: Michiel Dethmers <[email protected]>

* Use utf8mb4 for the connection etc (#1001)

* Use utf8mb4 for the connection etc

* Support utf8mb4 in campaign subject and content

---------

Co-authored-by: Michiel Dethmers <[email protected]>

* use PHP8.2 to build

* use latest phplint

* update docker build from bookworm

* set version

* avoid the admin being kicked out after upgrade (#1033)

* mark update translations as @wip

---------

Co-authored-by: Duncan Cameron <[email protected]>
Co-authored-by: Alain Rihs <[email protected]>
Co-authored-by: lwcorp <[email protected]>
Co-authored-by: Duncan Cameron <[email protected]>
Co-authored-by: Michiel Dethmers <[email protected]>
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.

5 participants