Remove memory and upload PHP settings

[rcdailey]

Because memory & file size limitation settings are specified in the local configs, they override any
values in the global php.ini on the system. Because of this, it's impossible to, for example,
increase the memory limit of Nextcloud without touching its own configuration/source files.

These settings are removed in favor of the php.ini settings. The justification for not having
these settings specified strictly by Nextcloud is that they are variant. In other words, their
values are environment-dependent, and especially dependent on the content hosted by Nextcloud
itself. For example, hosting lots of images and thumbnails frequently causes memory issues.

[rcdailey]

@J0WI: Would like you to take a peek as well, since this relates to nextcloud/docker#568

[J0WI]

I just noticed that the issue does not exist when changing the upload limit. After some research I found this:

server/lib/private/IntegrityCheck/Checker.php

Lines 183 to 227 in 72e745b

	// The .user.ini and the .htaccess file of ownCloud can contain some
	// custom modifications such as for example the maximum upload size
	// to ensure that this will not lead to false positives this will
	// copy the file to a temporary folder and reset it to the default
	// values.
	if($filename === $this->environmentHelper->getServerRoot() . '/.htaccess'
	|| $filename === $this->environmentHelper->getServerRoot() . '/.user.ini') {
	if(!$copiedWebserverSettingFiles) {
	$tmpFolder = rtrim($this->tempManager->getTemporaryFolder(), '/');
	copy($this->environmentHelper->getServerRoot() . '/.htaccess', $tmpFolder . '/.htaccess');
	copy($this->environmentHelper->getServerRoot() . '/.user.ini', $tmpFolder . '/.user.ini');
	\OC_Files::setUploadLimit(
	\OCP\Util::computerFileSize('511MB'),
	[
	'.htaccess' => $tmpFolder . '/.htaccess',
	'.user.ini' => $tmpFolder . '/.user.ini',
	]
	);
	}
	}
	// The .user.ini file can contain custom modifications to the file size
	// as well.
	if($filename === $this->environmentHelper->getServerRoot() . '/.user.ini') {
	$fileContent = file_get_contents($tmpFolder . '/.user.ini');
	$hashes[$relativeFileName] = hash('sha512', $fileContent);
	continue;
	}
	// The .htaccess file in the root folder of ownCloud can contain
	// custom content after the installation due to the fact that dynamic
	// content is written into it at installation time as well. This
	// includes for example the 404 and 403 instructions.
	// Thus we ignore everything below the first occurrence of
	// "#### DO NOT CHANGE ANYTHING ABOVE THIS LINE ####" and have the
	// hash generated based on this.
	if($filename === $this->environmentHelper->getServerRoot() . '/.htaccess') {
	$fileContent = file_get_contents($tmpFolder . '/.htaccess');
	$explodedArray = explode('#### DO NOT CHANGE ANYTHING ABOVE THIS LINE ####', $fileContent);
	if(\count($explodedArray) === 2) {
	$hashes[$relativeFileName] = hash('sha512', $explodedArray[0]);
	continue;
	}
	}

I think it's worth to add an exclusion for memory_limit too.

[rcdailey]

Seems like a lot of coding/work when those values could just be eliminated from the configuration files to begin with. Would be interested in knowing why it is so important for them to be there, versus expecting a properly configured PHP before running Nextcloud?

[J0WI]

@rullzer I wonder if the integrity check for this file is still useful if any value can be overwritten on the very end of the .htaccess. There is no such line in the .user.ini.

[aignerat]

I think the best way to cope with this is a config-setting. the priority seems rather low, but big instances might want to use more than 512 mb ram and want to be bleeding-edge, so you always have to rewrite this after some minor update every month and there is always a chance that you forget about it.

something like this: nextcloudStandardServerParameters -> true; = leave the settings as nextcloud says
nextcloudStandardServerParameters -> false; = use php.ini

Thinking again about this issue, it's 512 mb per session and not for the whole server, the only thing I think off a user would need more than this is writing a .pdf that has 100 pages or more. I guess onlyoffice uses other config-settings so this part of the issue seems to be easily tolerable.

[rcdailey]

I apologize for repeating myself, but for my own educational benefit could someone address my last question? This seemingly simple issue seems to be more complex than it appears, and I'm assuming there's a good reason for that.

[J0WI]

Would be interested in knowing why it is so important for them to be there, versus expecting a properly configured PHP before running Nextcloud?

I stuck with the same question in #8207 (comment).

[aignerat]

I apologize for repeating myself, but for my own educational benefit could someone address my last question? This seemingly simple issue seems to be more complex than it appears, and I'm assuming there's a good reason for that.

It's just another approach to solve the problem, I personally don't think it's a good approach and far from best practice, but not everything can have a valueable design from the start on, and it's easy to judge now. I guess it seemed a good idea to some people when it was implemented. In this case I'd like to reference to: "Computers will never need to have more than 640kb or ram.

[rcdailey]

It's just another approach to solve the problem, I personally don't think it's a good approach and far from best practice, but not everything can have a valueable design from the start on, and it's easy to judge now. I guess it seemed a good idea to some people when it was implemented. In this case I'd like to reference to: "Computers will never need to have more than 640kb or ram.

Thanks for explaining. I'm honestly not too concerned with the history of it, I realize software changes and code rots. The main thing I wanted to make sure of is that it didn't have any importance in today's code. Sounds like it's not critical.

This is important to understand, because so far it feels like a lot of effort is being put into keeping the settings and working around them. To me, this is just contributing to the original issue (code rot). I prefer the straightforward approach: Remove them.

The biggest harm I can think of that this causes is that users that do not currently have a php.ini configured in their instance will fall back to the default of (I believe) 128MB of RAM. This could cause their instances to stop working. Solving this might be as simple as a big bold disclaimer in the release notes. I'm not sure what the process is in this case, but I'm confident it's ultimately a minor issue and very solvable.

Given the discussion up to this point, and the simplicity of the issue, I still feel that my current pull request represents the desired changes from my point of view. Unless there are serious concerns, I'd like us to merge the changes I have without changing (specifically: over-complicating) the solution/strategy.

Making this change would enable the Nextcloud Docker project to proceed with changes enabling us to more directly control the PHP configuration settings of the instance without modifying files in the working directory. There's just no other reasonable workaround IMHO.

Since I'm still pretty unfamiliar with the project, could someone let me know which individuals are key for approval to get this merged? I'd like to get discussion going so we can come to a consensus. Really hoping we keep this simple, though.

Thanks!

[kesselb]

I apologize for repeating myself, but for my own educational benefit could someone address my last question? This seemingly simple issue seems to be more complex than it appears, and I'm assuming there's a good reason for that.

Best guess ;-) owncloud/core#10992

I stuck with the same question in #8207 (comment).

Shipping a sane default is not a bad in general but i agree with you at this point. There should be a setup check (or better a check before the setup) like "you are running nextcloud with insufficient php/webserver settings. things will not work".

server/lib/base.php

Lines 627 to 637 in 0bb55f4

	//try to set the maximum execution time to 60min
	if (strpos(@ini_get('disable_functions'), 'set_time_limit') === false) {
	@set_time_limit(3600);
	}
	@ini_set('max_execution_time', '3600');
	@ini_set('max_input_time', '3600');
	//try to set the maximum filesize to 10G
	@ini_set('upload_max_filesize', '10G');
	@ini_set('post_max_size', '10G');
	@ini_set('file_uploads', '50');

Here is another issue with a similar topic #10082.

[rcdailey]

@danielkesselberg Looks like we replied within seconds of each other :-)

Bearing in mind I prefer an iterative approach (address the issue in small chunks of work), what would you recommend in terms of getting this merged? Basically, I don't want to dive too much into coding PHP for this. I agree it is a good idea to have the start up checks, but they don't seem mandatory to me.

This might be due to my inexperience with the project and PHP in general, but to me the PHP settings are just part of the environment. And when I think of installing a piece of server software, I always assume some manual configuration of the environment is necessary. Things like setting proxy settings, memory limits, max upload sizes, etc.

Making Nextcloud set these up for you or warn about them seems more supplementary: It's a defensive mechanism. Again, I'm most likely naive here, but that's my thought.

Regardless, if you are OK with it, I'd like to merge this first and then open another issue to discuss potential changes to start up checks to either proactively set defaults or warn about configuration issues, assuming you are suggesting those additions are desired. I honestly feel like getting rid of these settings is an easy decision, while making start up more robust in their absence something that will require a more complex strategy and more discussion. On top of that, there are LOT of open issues and pull requests, so you guys are obviously spread thin. Keeping PRs small and manageable will greatly help here.

Let me know if I'm off track. Thanks again.

[kesselb]

This is important to understand, because so far it feels like a lot of effort is being put into keeping the settings and working around them. To me, this is just contributing to the original issue (code rot). I prefer the straightforward approach: Remove them.

👍

Given the discussion up to this point, and the simplicity of the issue, I still feel that my current pull request represents the desired changes from my point of view. Unless there are serious concerns, I'd like us to merge the changes I have without changing (specifically: over-complicating) the solution/strategy.

I would vote for a setup check (look at this as a start #12824).

Since I'm still pretty unfamiliar with the project, could someone let me know which individuals are key for approval to get this merged? I'd like to get discussion going so we can come to a consensus. Really hoping we keep this simple, though.

@MorrisJobke

[aignerat]

i got another suggestion that the code could nearly stay the same, is it possible to use config-settings in user.ini and .htaccess? That way you could keep the standard settings and just deactivate them with the config. Another possible way is to write a bash- or occ-script that you use after each update. Still the config-setting-way would be the smartest to respect all opinions if it's possible.

[MorrisJobke]

I haven't read fully through this ticket, but want to leave a not first, before coming back to this ticket later:

The limits are in here to make them available on as many systems as possible out of the box. IMO they still make sense for many instances, but as you described they could also unintentionally overwrite system settings that are there for a specific reason. One way would be to check for the current values and only set them in the .htaccess if they are not on a useful value. This then would be added here:

server/lib/private/Setup.php

Lines 491 to 502 in b9f2ce2

	$content = "#### DO NOT CHANGE ANYTHING ABOVE THIS LINE ####\n";
	$htaccessContent = explode($content, $htaccessContent, 2)[0];
	//custom 403 error page
	$content .= "\nErrorDocument 403 " . $webRoot . '/';
	//custom 404 error page
	$content .= "\nErrorDocument 404 " . $webRoot . '/';
	// Add rewrite rules if the RewriteBase is configured
	$rewriteBase = $config->getValue('htaccess.RewriteBase', '');
	if($rewriteBase !== '') {

Maybe also this dynamic setting can be turned off in config.php. But this would only be a last resort if the automatic way is completely impossible.

[kesselb]

i got another suggestion that the code could nearly stay the same, is it possible to use config-settings in user.ini and .htaccess? That way you could keep the standard settings and just deactivate them with the config. Another possible way is to write a bash- or occ-script that you use after each update. Still the config-setting-way would be the smartest to respect all opinions if it's possible.

Yeah add even more complexity!

[rcdailey]

@MorrisJobke:

Maybe also this dynamic setting can be turned off in config.php. But this would only be a last resort if the automatic way is completely impossible.

What is the strategy for trying to make environment configuration as automatic as possible? This, as seen so far, has mixed results. The whole idea of things being environment-specific is that it's very unlikely for any two environments to be 100% identical. Different network specifications, different hardware, different geographical locations. There's so many parts that interleave and play together to make this kind of automation self-defeating.

I think it's worth balancing turn-key features with the costs and benefits. In this case, requiring users to configure PHP (which AFAIK most PHP applications already require) with a few basic settings is less of a burden than complex defensive mechanisms written in software that only work for a few select range of environment types.

I realize I'm the newbie here so I apologize for being so opinionated. I by no measure feel like I have enough experience with Nextcloud to say all of this with complete confidence. But my driving philosophy as an engineer is to keep it simple, and in this case I just don't see the benefits for the cost in this case.

Of course, even having said all of that, I'm willing to do whatever (within reason) to get us past the issue. If you guys want to over-engineer this, I might have to hand off the task to someone with more experience. My original intent was to minimize change and complexity.

If we're all in agreement, I'll go ahead and amend my PR with the start up checks suggested by @danielkesselberg. Is this acceptable @MorrisJobke? I'd like your buy off before I do work that ends up getting rejected. Let me know.

Thanks for the quick turnaround in the discussion guys, I really appreciate it.

[MorrisJobke]

What is the strategy for trying to make environment configuration as automatic as possible? This, as seen so far, has mixed results. The whole idea of things being environment-specific is that it's very unlikely for any two environments to be 100% identical. Different network specifications, different hardware, different geographical locations. There's so many parts that interleave and play together to make this kind of automation self-defeating.

I fully get this. The idea is to have as few as possible need for manual configuration as possible. But as you said: the PHP memory limit and the upload size is tricky due to the many moving parts. Then also I had another idea: we should make it a setup check and realized (due to not reading all of this before) that @danielkesselberg already proposed it:

Shipping a sane default is not a bad in general but i agree with you at this point. There should be a setup check (or better a check before the setup) like "you are running nextcloud with insufficient php/webserver settings. things will not work".

I'm fine with dropping those values and at the same time add a setup check that gives a warning in the admin settings that the memory limit or upload size is potential low together with a hint how to fix it.

[kesselb]

If we advice people to adjust the limits in .user.ini and/or .htaccess we need a solution for integrity check.

[nickvergessen]

we need a solution for integrity check.

in .htaccess you just need to put it below the marker

server/lib/private/Setup.php

Line 491 in b9f2ce2

$content = "#### DO NOT CHANGE ANYTHING ABOVE THIS LINE ####\n";

The reason why the values are in there, is that you can modify them via this method:

server/lib/private/legacy/files.php

Lines 381 to 461 in 79d9841

	/**
	* set the maximum upload size limit for apache hosts using .htaccess
	*
	* @param int $size file size in bytes
	* @param array $files override '.htaccess' and '.user.ini' locations
	* @return bool|int false on failure, size on success
	*/
	public static function setUploadLimit($size, $files = []) {
	//don't allow user to break his config
	$size = (int)$size;
	if ($size < self::UPLOAD_MIN_LIMIT_BYTES) {
	return false;
	}
	$size = OC_Helper::phpFileSize($size);
	$phpValueKeys = array(
	'upload_max_filesize',
	'post_max_size'
	);
	// default locations if not overridden by $files
	$files = array_merge([
	'.htaccess' => OC::$SERVERROOT . '/.htaccess',
	'.user.ini' => OC::$SERVERROOT . '/.user.ini'
	], $files);
	$updateFiles = [
	$files['.htaccess'] => [
	'pattern' => '/php_value %1$s (\S)*/',
	'setting' => 'php_value %1$s %2$s'
	],
	$files['.user.ini'] => [
	'pattern' => '/%1$s=(\S)*/',
	'setting' => '%1$s=%2$s'
	]
	];
	$success = true;
	foreach ($updateFiles as $filename => $patternMap) {
	// suppress warnings from fopen()
	$handle = @fopen($filename, 'r+');
	if (!$handle) {
	\OCP\Util::writeLog('files',
	'Can\'t write upload limit to ' . $filename . '. Please check the file permissions',
	ILogger::WARN);
	$success = false;
	continue; // try to update as many files as possible
	}
	$content = '';
	while (!feof($handle)) {
	$content .= fread($handle, 1000);
	}
	foreach ($phpValueKeys as $key) {
	$pattern = vsprintf($patternMap['pattern'], [$key]);
	$setting = vsprintf($patternMap['setting'], [$key, $size]);
	$hasReplaced = 0;
	$newContent = preg_replace($pattern, $setting, $content, 2, $hasReplaced);
	if ($newContent !== null) {
	$content = $newContent;
	}
	if ($hasReplaced === 0) {
	$content .= "\n" . $setting;
	}
	}
	// write file back
	ftruncate($handle, 0);
	rewind($handle);
	fwrite($handle, $content);
	fclose($handle);
	}
	if ($success) {
	return OC_Helper::computerFileSize($size);
	}
	return false;
	}

The UI is still there in /index.php/settings/admin. But yeah, the only reason I would see is people running it on a share hoster where you do not have access to php.ini. But then again, why would you set it to something lower than the maximum that is possible? (or is it possible to set it to something higher than php.ini?) It also does not prevent uploading larger files, because junking solves this for you. 🤷

All things taken into account, I would propose:

1. Remove the method to set the value:
   

   server/lib/private/legacy/files.php

   Lines 381 to 461 in 79d9841

   	/**
   	* set the maximum upload size limit for apache hosts using .htaccess
   	*
   	* @param int $size file size in bytes
   	* @param array $files override '.htaccess' and '.user.ini' locations
   	* @return bool|int false on failure, size on success
   	*/
   	public static function setUploadLimit($size, $files = []) {
   	//don't allow user to break his config
   	$size = (int)$size;
   	if ($size < self::UPLOAD_MIN_LIMIT_BYTES) {
   	return false;
   	}
   	$size = OC_Helper::phpFileSize($size);
   	$phpValueKeys = array(
   	'upload_max_filesize',
   	'post_max_size'
   	);
   	// default locations if not overridden by $files
   	$files = array_merge([
   	'.htaccess' => OC::$SERVERROOT . '/.htaccess',
   	'.user.ini' => OC::$SERVERROOT . '/.user.ini'
   	], $files);
   	$updateFiles = [
   	$files['.htaccess'] => [
   	'pattern' => '/php_value %1$s (\S)*/',
   	'setting' => 'php_value %1$s %2$s'
   	],
   	$files['.user.ini'] => [
   	'pattern' => '/%1$s=(\S)*/',
   	'setting' => '%1$s=%2$s'
   	]
   	];
   	$success = true;
   	foreach ($updateFiles as $filename => $patternMap) {
   	// suppress warnings from fopen()
   	$handle = @fopen($filename, 'r+');
   	if (!$handle) {
   	\OCP\Util::writeLog('files',
   	'Can\'t write upload limit to ' . $filename . '. Please check the file permissions',
   	ILogger::WARN);
   	$success = false;
   	continue; // try to update as many files as possible
   	}
   	$content = '';
   	while (!feof($handle)) {
   	$content .= fread($handle, 1000);
   	}
   	foreach ($phpValueKeys as $key) {
   	$pattern = vsprintf($patternMap['pattern'], [$key]);
   	$setting = vsprintf($patternMap['setting'], [$key, $size]);
   	$hasReplaced = 0;
   	$newContent = preg_replace($pattern, $setting, $content, 2, $hasReplaced);
   	if ($newContent !== null) {
   	$content = $newContent;
   	}
   	if ($hasReplaced === 0) {
   	$content .= "\n" . $setting;
   	}
   	}
   	// write file back
   	ftruncate($handle, 0);
   	rewind($handle);
   	fwrite($handle, $content);
   	fclose($handle);
   	}
   	if ($success) {
   	return OC_Helper::computerFileSize($size);
   	}
   	return false;
   	}

2. Remove the UI that shows / allows to set this value:
   https://github.com/nextcloud/server/blob/cea4c9d00091e765e36849d4ff2edaf7134f8963/apps/files/lib/Settings/Admin.php
   https://github.com/nextcloud/server/blob/b79997dd3378871c9e2cd603e529030f1581e35f/apps/files/templates/admin.php
3. Remove the value from the default files (not on upgrade) (your PR)
4. Show a setup warning/error if the limits are below X/Y MB

[MorrisJobke]

The reason why the values are in there, is that you can modify them via this method:

Which is also not needed anymore because we support chunking on all platforms (except native WebDAV coming from Windows/macOS, but there is very little we can do here anyways)..

[MorrisJobke]

And yes - the 4 steps that @nickvergessen described are the way to go and should come in one PR IMO.

[rcdailey]

Thanks for all the guidance. I'm temporarily rebasing to stable15 so I can test on my personal server. Once I'm done, I will rebase back to master for merging. Please excuse any superfluous force pushes and notifications as I test my work.

[rcdailey]

Can I get some advice on how to test and run the server on Windows for development? All I really do is use VS Code to edit the source, but I really don't want to have to muck with my home Nextcloud server for testing.

Can I easily run the PHP server on my Windows 10 machine somehow for debugging and testing? Do you have a developer guide somewhere? I'm not a PHP developer, so I'm sure there's some basics I'm not aware of. Greatly appreciate any time you guys can take to help point me in the right direction.

[aignerat]

I'd use docker, its an easy way to get started quick, depending on what you want to do choose a nginx (needs less performance) or apache 2.4 (can be configured better and can easy use gssapi). I'd look for an image with seperated postgres or mariadb. The splitted php-fpm isnt necessary for local testing. I wonder what you want to do without knowledge of php, php is no rocket science, still you should know a bit about webcoding before you start. Maybe lookup onlineblogs of ppl customizing joomla or nextcloud modules to get started.

[rcdailey]

Thanks for the tips! By the way I never said I didn't know about web development, I just haven't used PHP tech before. I have done web servers in Java & C++ before though. So I have a general idea of the topography/structure. Just needed some PHP-tech-specific advice.

[nickvergessen]

Sorry for the troubles @rcdailey
Windows is not a supported enviroment for the server code. If this all generates too much work on your side, we can also take it over and fix the comments that have been made.

[rcdailey]

Only reason I'm really offering to do the work instead of your team is because it seemed apparent to me that either 1) this isn't a high priority in your list or 2) you don't have the resources to work on it

Obviously, if your team did the work, it would be done most optimally since you know the code base better than I do. However, I'd rather it get done than sit in open status for a long time, so I'm working through the learning curve.

If you have the bandwidth to work on this, obviously I'd be very grateful to have your help. However, if not, I'll do my best to contribute what I can. I know I'm already taking up a lot of your time for the guidance. I'll try docker containers on Windows to see if I can test this. It will take a while to set up, but I think I can get it to work.

Let me know if you want to take this over in the next week or two. If so, I'll let you do the work. Otherwise, if either of the 2 points above apply, I'm happy to pitch in. Thanks!

[nickvergessen]

The main reason I'm asking is that 22nd february (upcoming friday) marks the feature freeze for Nextcloud 16, so if it is not in by then, it will have to wait until Nextcloud 17:
https://github.com/nextcloud/server/wiki/Maintenance-and-Release-Schedule

[rcdailey]

Oh, sadly since I'm working on this with whatever little free time I have, I won't be done by then :-(

If you can go ahead and take this over and get it done before then, that'd be excellent. Is that possible for you?

So far I started down the rabbit trail of removing setUploadLimit(), but that has usage across the code base, and I'm trying to be careful about removing references to calling that function. So far, things aren't as straightforward for me as they sounded initially, even though everyone has done a really good job outlining the steps required to make the necessary UI changes and such.

[MorrisJobke]

Maybe I look into this until then.

[rcdailey]

@MorrisJobke How are things going? Did we miss the release train?

[MorrisJobke]

@MorrisJobke How are things going? Did we miss the release train?

Just noticed that this is against 15 - we first need it in master and then decide on if this is back ported. Do you mind to do a PR against master?

For 15 the freeze was already over 1 week ago, so it would be anyways.

[rcdailey]

@MorrisJobke 15 only because that's what I'm running. Given the scope of change currently proposed, I wouldn't suspect this is appropriate as a "hotfix" to an otherwise stable release. I'm OK with this being in version 16, I can upgrade to that when it comes out. At the end of the day it's up to you how you want to manage the release of this. I'll follow whatever you decide.

I only rebased this to stable15 so I could implement the feature and test on my live server at home. I didn't want to upgrade it to 16 just to test this change. But since you're doing the work, by all means rebase it back to master.

[MorrisJobke]

Superseeded by #14430