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

Update .user.ini when setting upload size limit #17182

Merged
merged 2 commits into from
Aug 17, 2015

Conversation

RobinMcCorkell
Copy link
Member

We try to change as many files as possible, so if we can write to .user.ini but not to .htaccess then we will (but the file contents will become inconsistent). Of course, whichever one actually affects the installation will be shown in the upload limit box in the admin settings, so there is no ambiguity in what happened, and the logs clearly state if any files could not be written to.

Not sure if this can be unit tested?

Fixes #14748

cc @PVince81 @LukasReschke @DeepDiver1975 @RealRancor @MorrisJobke

@mmattel
Copy link
Contributor

mmattel commented Jun 25, 2015

👍

@karlitschek
Copy link
Contributor

@DeepDiver1975 What do you think?

$htaccess .= "\n" . $setting;

$content = '';
while (!feof($handle)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@DeepDiver1975
Copy link
Member

Not sure if this can be unit tested?

I guess it is possible

  • by passing in the root folder where files are to be read/written this can be tweaked for unit testing.

@RobinMcCorkell
Copy link
Member Author

@MorrisJobke I used fopen() and related functions so we only need to open the file and check for access once - before we do any reading or replacing of the contents. The original code used file_get_contents() and file_put_contents().

Robin McCorkell added 2 commits July 20, 2015 12:52
There was also a bug with checking the upper limit on the passed upload
size. PHP does funny things with integer vs float comparisons, so our
check didn't work. Now the check is much simpler, and ensures the value
is sane.
@scrutinizer-notifier
Copy link

A new inspection was created.

@RobinMcCorkell
Copy link
Member Author

Unit tests! There was also a bug with checking the upper limit on the passed upload size. PHP does funny things with integer vs float comparisons, so our check didn't work. Now the check is much simpler, and ensures the value is sane.

Oh, there's also a new message in the admin panel that warns the admin that with PHP-FPM, the changes may take up to 5 minutes to take effect: http://php.net/manual/en/configuration.file.per-user.php#101705

@ghost
Copy link

ghost commented Jul 20, 2015

🚀 Test PASSed.🚀
chuck

@RobinMcCorkell
Copy link
Member Author

@DeepDiver1975 @MorrisJobke Re-review please?

@RobinMcCorkell
Copy link
Member Author

@MorrisJobke Do you still think this should be using file_get_contents() and file_put_contents() instead of fopen()?

@MorrisJobke
Copy link
Contributor

@MorrisJobke Do you still think this should be using file_get_contents() and file_put_contents() instead of fopen()?

I'm fine with the way it is now.

@RobinMcCorkell
Copy link
Member Author

@DeepDiver1975 @MorrisJobke please review

@MorrisJobke
Copy link
Contributor

Tested and works 👍 (also with changed permissions)

RobinMcCorkell pushed a commit that referenced this pull request Aug 17, 2015
Update .user.ini when setting upload size limit
@RobinMcCorkell RobinMcCorkell merged commit 675d852 into master Aug 17, 2015
@RobinMcCorkell RobinMcCorkell deleted the user_ini_upload_size branch August 17, 2015 12:27
@lock lock bot locked as resolved and limited conversation to collaborators Aug 10, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

File handling in admin: Also write to .user.ini
6 participants