-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Add check for problematic modules reqtimeout, deflate and pagespeed #14699
Conversation
Thanks a lot for your contribution! Contributions to the core repo require a signed contributors agreement http://owncloud.org/contribute/agreement/ Alternatively you can add a comment here stating that this contribution is MIT licensed. Some more details about out pull request workflow can be found here: http://owncloud.org/code-reviews-on-github/ |
@@ -647,6 +654,11 @@ public static function checkServer(\OCP\IConfig $config) { | |||
} | |||
} | |||
} | |||
foreach ($dependencies['modules'] as $checkModule => $module) { | |||
if (apacheModuleEnabled($checkModule)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you mean if (self::apacheModuleEnabled($checkModule))
?
@nickvergessen |
The inspection completed: 2 new issues, 1 updated code elements |
@@ -602,8 +603,14 @@ public static function checkServer(\OCP\IConfig $config) { | |||
'output_buffering' => false, | |||
'default_charset' => 'UTF-8', | |||
], | |||
'modules' => array( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New array annotation please while at it :)
['foo']
Do we know if that lead to issues with sharing hosting environments where such modules are enabled but not necessary enabled on every virtual host? If this is the case then please consider performing some checks using a client-side check: https://github.com/owncloud/core/blob/7e0fd8fb501459f3ff1cfa5ddb40e1c97d50d617/core/js/setupchecks.js – I think the Deflate should be signalized in the response header? |
@LukasReschke The client-side check is probably the best way which could also catch "gzip on" (owncloud-archive/documentation#922) and ngx_pagespeed (#14604) enabled NginX servers. So to sum up:
I really don't have the knowledge in JS so if this way is preferred some one else have to do this. |
Okay. So I'll take care of those three in another PR and you update this PR accordingly? :) |
If only mod_reqtimeout is left we could also just close the PR as this module is documented with owncloud-archive/documentation#897 and probably not that wide-spread as mod_deflate and pagespeed? |
foreach($problematicModules as $problematicModule) { | ||
$errors[] = array( | ||
'error' => $l->t('The Apache module %s is enabled.', array($problematicModule)), | ||
'hint' => $l->t('This module can cause various issues when enabled.' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no string concat please - translation extraction will fail - thx
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi,
then you might want to fix the whole util.php as this is used on several places. But i will just close the PR as the module check is not very reliable.
Partly fixes #4783 and #14603.
Currently untested and should be just a discussion starting point how to check for such modules.
The checks also works only with mod-php5 and not with php-cgi/php-fpm
MIT licensed