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

Jump to bottom

Improved PHP-FPM diagnostic check config option compatibility #19031

Merged
merged 6 commits into from Apr 4, 2022
Merged

Improved PHP-FPM diagnostic check config option compatibility #19031

merged 6 commits into from Apr 4, 2022

Conversation

bx80
Copy link
Contributor

@bx80 bx80 commented Mar 31, 2022

Description:

The additional access rule checks for PHP-FPM added by #18398 do not respect the enable_required_directories diagnostic config.ini.php setting and are therefore causing unwanted http requests for config files when the setting is set to disabled.

This PR adjusts this diagnostic check to only attempt file access if the config setting allows it.

Fixes #18967

Review

@bx80 bx80 added the Regression Indicates a feature used to work in a certain way but it no longer does even though it should. label Mar 31, 2022
@bx80 bx80 added this to the 4.9.0 milestone Mar 31, 2022
@bx80 bx80 self-assigned this Mar 31, 2022
@justinvelluppillai justinvelluppillai changed the base branch from 4.x-dev to next_release March 31, 2022 02:48
@bx80 bx80 added not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. Needs Review PRs that need a code review labels Mar 31, 2022
sgiehl
sgiehl requested changes Mar 31, 2022
View reviewed changes
Copy link
Member

@sgiehl sgiehl left a comment

Choose a reason for hiding this comment

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

Actually I was wondering if we really need to make it that complex. If some private directories are accessible, I guess the other diagnostic check will print a warning nevertheless, so there isn't much value in adding another warning here.
I guess I would simply skip the directory check here, if disabled in config and print the sapi name

plugins/Diagnostics/Diagnostic/PhpInformational.php Outdated Show resolved Hide resolved
@bx80
Copy link
Contributor Author

bx80 commented Mar 31, 2022

I've simplified it so that the file access warning message will only be shown if they are using PHP-FPM and the enable_required_directories_diagnostic is not disabled and the file is accessible, for all other scenarios it will just show the sapi name. Also switched to using the GeneralConfig class.

@justinvelluppillai justinvelluppillai merged commit 816fcd2 into next_release Apr 4, 2022
@justinvelluppillai justinvelluppillai deleted the m-18967-diag-fpm-respect-config branch April 4, 2022 05:23
@justinvelluppillai justinvelluppillai removed the Needs Review PRs that need a code review label Apr 4, 2022
@HCl-not-HCi HCl-not-HCi mentioned this pull request Apr 23, 2022
Merged
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sgiehl sgiehl sgiehl requested changes

@justinvelluppillai justinvelluppillai justinvelluppillai approved these changes

Assignees

@bx80 bx80

Labels
not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. Regression Indicates a feature used to work in a certain way but it no longer does even though it should.
Projects
None yet
Milestone
4.9.0
Development

Successfully merging this pull request may close these issues.

System check - don't request private directories when disabled
3 participants
@bx80 @justinvelluppillai @sgiehl