Respect user settings in php.ini if they are big enough

[mickenordin]

In the admin guide:

• https://docs.nextcloud.com/server/latest/admin_manual/configuration_files/big_file_upload_configuration.html

Fix #32209

it is mentioned that you can tweek:

• max_input_time
• max_execution_time

in order to enable larger file uploads. However, the current codebase
will hard code these values to one hour, no matter what the user sets in
php.ini.

This patch will allow the user to set these settings in php.ini and they
will be respected, if and only if, they are set to something bigger than
3600 seconds.

[mickenordin]

This fixes #32209

[szaimen]

Thanks for your PR! can you please solve DCO (see https://github.com/nextcloud/server/pull/32216/checks?check_run_id=6223623697)? Thank you!

[mickenordin]

Thanks for your PR! can you please solve DCO (see https://github.com/nextcloud/server/pull/32216/checks?check_run_id=6223623697)? Thank you!

Sure, fixed.

[kesselb]

Thanks for sending a pull request 👍

The above code should work and allow an administrator to pick a higher value.

I think the actual issue here is that we configure max_execution_time and max_input_time at all. That code should go and we introduce a setup check to inform the administrator that an update for php.ini / user.ini / .htaccess is required to configure good limits. This auto configuration approach might be nice but it's always a pain if one needs a higher limit. My take here is that Nextcloud should not try to auto configure the environment but tell the administrator what's necessary. Less magic => less pain ;)

Also keep in mind that almost every request is executing this code. It's already bad to have the ini_set calls (with suppress operator) here. If we merge this patch we check on each time if there is a higher limit and if not run the ini_set call. I don't think that removing those calls will improve the performance yet it's still bad.

[CarlSchwan]

Thanks for sending a pull request +1

The above code should work and allow an administrator to pick a higher value.

I think the actual issue here is that we configure max_execution_time and max_input_time at all. That code should go and we introduce a setup check to inform the administrator that an update for php.ini / user.ini / .htaccess is required to configure good limits. This auto configuration approach might be nice but it's always a pain if one needs a higher limit. My take here is that Nextcloud should not try to auto configure the environment but tell the administrator what's necessary. Less magic => less pain ;)

Also keep in mind that almost every request is executing this code. It's already bad to have the ini_set calls (with suppress operator) here. If we merge this patch we check on each time if there is a higher limit and if not run the ini_set call. I don't think that removing those calls will improve the performance yet it's still bad.

For shared hosting providers, it's often not possible to modify the ini files, this is probably the reason why this is done like this currently.

[solracsf]

For shared hosting providers, it's often not possible to modify the ini files, this is probably the reason why this is done like this currently.

For shared hosting, most of the time, both are disabled (ini files and ini_set())

[mickenordin]

Thanks for sending a pull request 👍

The above code should work and allow an administrator to pick a higher value.

I think the actual issue here is that we configure max_execution_time and max_input_time at all. That code should go and we introduce a setup check to inform the administrator that an update for php.ini / user.ini / .htaccess is required to configure good limits. This auto configuration approach might be nice but it's always a pain if one needs a higher limit. My take here is that Nextcloud should not try to auto configure the environment but tell the administrator what's necessary. Less magic => less pain ;)

Also keep in mind that almost every request is executing this code. It's already bad to have the ini_set calls (with suppress operator) here. If we merge this patch we check on each time if there is a higher limit and if not run the ini_set call. I don't think that removing those calls will improve the performance yet it's still bad.

I am not married to either solution, I just want to be able to change the setting and not have it hard coded, which I think is the worst solution of them all.

This patch is proposed because it is backwards compatible with the old solution. I don't know if removing the hard coded value altogether will break stuff for a lot of people on shared hosting or for people who did not have to think about this untill now. I also don't think there is a lot of performance penalty with doing it this way so it shouldn't be a problem, but if you guys would rather remove the hard coded value that is fine too for me. :)

[solracsf]

Sure, this is a "problem" of all ini_set() calls at lib/base.php ;)

[welcome]

Thanks for your first pull request and welcome to the community! Feel free to keep them coming! If you are looking for issues to tackle then have a look at this selection: https://github.com/nextcloud/server/issues?q=is%3Aopen+is%3Aissue+label%3A%22good+first+issue%22

[szaimen]

/backport to stable24

[szaimen]

/backport to stable23