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
737 stars 268 forks source link

Changed timer functions in connect.php so that they return numbers to avoid errors when sending rate is computed #953

Closed MarcelvC closed 1 year ago

MarcelvC commented 1 year ago

Changed elapsed() and interval() in connect.php so that they return numbers instead of strings

I'm managing a phpList installation and noticed that the sending rate had slowed down extremely in recent months (over 12 hours for ~800 messages). I discovered that this was related to switching the server from php 7.4 to php 8.0.

Processing the queue via SSH with error reporting turned on revealed that lines 262 and 1224 of admin/processqueue throw errors (division by zero / non-numeric value). This is due to the fact that in the German locale, something like "sprintf('%0.10f', $elapsed / 1000000)" returns a string with a comma.

To resolve the issue, I have removed the sprintf() wrappers from the elapsed() and interval() functions in the timer class, so that they return numbers instead of strings.

phpListDockerBot commented 1 year ago

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

https://discuss.phplist.org/t/is-php-8-x-x-supported-by-phplist/8371/18

bramley commented 1 year ago

This problem has already been reported and fixed see #931 That fix will be in the next release of phplist, 3.6.13.

MarcelvC commented 1 year ago

Thanks for letting me know!

Since the return values of the timer functions are used in calculations, wouldn't it make more sense though to return a number instead of a string and only format values right before they are presented to the user or written into the database? processQueueOutput() for example uses sprintf() anyway to format $msgperhour.

bramley commented 1 year ago

The change that I made was the simplest to avoid being locale specific and keep the current behaviour of the code. I wasn't trying to "improve" the code at the same time.

michield commented 1 year ago

Closing this, as already addressed in latest release. Feel free to open a new PR for additional improvements.