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

Remove useless ini_set calls #24934

Merged
merged 1 commit into from
Jan 7, 2021
Merged

Remove useless ini_set calls #24934

merged 1 commit into from
Jan 7, 2021

Conversation

solracsf
Copy link
Member

@solracsf solracsf commented Jan 2, 2021

They're all PHP_INI_SYSTEM.
See comments below 👍

@solracsf solracsf added the 3. to review Waiting for reviews label Jan 2, 2021
@kesselb
Copy link
Contributor

kesselb commented Jan 2, 2021

Did you run into an issue because the value is wrong?

  • The default for max_file_uploads is 20 and that seems enough. It's not relevant anyway because max_file_uploads limits the number of uploaded files per request. Each file is uploaded in a own request usually.
  • Afaik a positive integer is considered as true. I think we should remove the ini_set call for file_uploads. That's something for htaccess or user.ini.

@solracsf
Copy link
Member Author

solracsf commented Jan 2, 2021

I was simply checking what ini_set calls Nextcloud was making in a ini_set disabled host to set those values manually.

It should be either fixed with this PR or (re)moved as you've said.

@kesselb
Copy link
Contributor

kesselb commented Jan 2, 2021

file_uploads max_file_uploads
Master On 20
This PR On 50

I don't see why we need to increase max_file_uploads.

@solracsf
Copy link
Member Author

solracsf commented Jan 2, 2021

As far as i understand, this can't even be changed trough .htaccess our ini_set...
https://bugs.php.net/bug.php?id=50684

So the best option is to remove it.

@kesselb
Copy link
Contributor

kesselb commented Jan 2, 2021

As far as i understand, this can't even be changed trough .htaccess our ini_set...

That's correct. file_uploads is also PHP_INI_SYSTEM and not changeable via ini_set. Please remove it.

@kesselb
Copy link
Contributor

kesselb commented Jan 2, 2021

And if your are on it please remove upload_max_filesize and post_max_size because they are PHP_INI_SYSTEM too ;)

3 method calls less 🤣 🥳

@solracsf solracsf changed the title Fix @ini_set 'max_file_uploads' Remove useless ini_set functions Jan 2, 2021
@kesselb
Copy link
Contributor

kesselb commented Jan 2, 2021

I found another one 🙈

max_input_time is PHP_INI_PERDIR (https://www.php.net/manual/en/info.configuration.php)

Would be nice to squash all commits together (or drop the old ones) and sign off (https://github.com/nextcloud/server/pull/24934/checks?check_run_id=1637306644 for some hints)

Thanks for cleaning this up 👍

@solracsf solracsf changed the title Remove useless ini_set functions Remove useless ini_set calls Jan 2, 2021
@rullzer
Copy link
Member

rullzer commented Jan 4, 2021

Could you squash this to 1 commit?
Then lets get it in.

@rullzer rullzer added this to the Nextcloud 21 milestone Jan 4, 2021
@rullzer rullzer mentioned this pull request Jan 6, 2021
5 tasks
@rullzer rullzer merged commit 4f52e83 into master Jan 7, 2021
@rullzer rullzer deleted the max_file_uploads branch January 7, 2021 19:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants