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

PHP8 fix - Ensure data retention settings are respected #19869

Merged
merged 1 commit into from Oct 17, 2022
Merged

Conversation

sgiehl
Copy link
Member

@sgiehl sgiehl commented Oct 17, 2022

Description:

On PHP 8, the settings if log and report data should be deleted might not be respected correctly.

The reason for this is, that the API converts the setting to a boolean e.g. here:

'delete_logs_enable' => !empty($enableDeleteLogs),

But as the value is then stored in an option in the database, it gets converted to a string again.
So the resulting database value is either 1 (string value of true) or an empty string (string value of false).

When the value is later checked in the task this is done this way:

if ($settings['delete_reports_enable'] == 0) {

That used to work smoothly for PHP 7. But for PHP 8 the result of that compare changed. The result of comparing an empty string against 0 changed. See https://3v4l.org/v4p2U

That means that removing log and report data is actually always enabled on PHP 8 once the settings in the UI had been saved.

Review

@sgiehl sgiehl added Critical Indicates the severity of an issue is very critical and the issue has a very high priority. Needs Review PRs that need a code review Regression Indicates a feature used to work in a certain way but it no longer does even though it should. labels Oct 17, 2022
@sgiehl sgiehl added this to the 4.12.1 milestone Oct 17, 2022
Copy link
Contributor

@bx80 bx80 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good spot! 👍 Tested by enabling and disabling the purge log setting in the UI, then break point testing the values returned by getPurgeDataSettings() before and after this fix. Running on PHP8.1.11

@bx80 bx80 merged commit 5bedd02 into 4.x-dev Oct 17, 2022
@bx80 bx80 deleted the fixsettings branch October 17, 2022 21:18
@tsteur
Copy link
Member

tsteur commented Oct 17, 2022

@sgiehl great find! As it's a critical regression can we add a test for this?

@sgiehl
Copy link
Member Author

sgiehl commented Oct 18, 2022

created a simple test in #19877

@justinvelluppillai justinvelluppillai changed the title Data retention settings might not be respected on PHP 8 PHP8 fix - Ensure data retention settings are respected Oct 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Critical Indicates the severity of an issue is very critical and the issue has a very high priority. Needs Review PRs that need a code review Regression Indicates a feature used to work in a certain way but it no longer does even though it should.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants