Skip to content

feat(settings): add big file upload setup checks - #49372

Merged
skjnldsv merged 1 commit into
masterfrom
feat/php-setup-file-upload
Nov 19, 2024
Merged

feat(settings): add big file upload setup checks#49372
skjnldsv merged 1 commit into
masterfrom
feat/php-setup-file-upload

Conversation

@skjnldsv

@skjnldsvskjnldsv commented Nov 19, 2024•
edited
Loading

Copy link
Copy Markdown
Member

@skjnldsvskjnldsv added this to the Nextcloud 31 milestone Nov 19, 2024
@skjnldsv
skjnldsv requested review from a team, come-nc and nickvergessenNovember 19, 2024 11:43
@skjnldsvskjnldsv self-assigned this Nov 19, 2024
@skjnldsv
skjnldsv requested review from icewind1991 and nfebe and removed request for a teamNovember 19, 2024 11:43
@skjnldsv
skjnldsvforce-pushed the feat/php-setup-file-upload branch from 106e651 to 3e8149eCompareNovember 19, 2024 11:43
@skjnldsv

skjnldsv commented Nov 19, 2024

Copy link
Copy Markdown
MemberAuthor

/backport to stable30

@skjnldsv

skjnldsv commented Nov 19, 2024

Copy link
Copy Markdown
MemberAuthor

/backport to stable29

@skjnldsv

skjnldsv commented Nov 19, 2024

Copy link
Copy Markdown
MemberAuthor

/backport to stable28

@skjnldsv
skjnldsvforce-pushed the feat/php-setup-file-upload branch from 3e8149e to 3d988aeCompareNovember 19, 2024 11:51
szaimen
szaimen reviewed Nov 19, 2024
View reviewed changes
Comment threadapps/settings/lib/SetupChecks/PhpMaxFileSize.php
szaimen
szaimen approved these changes Nov 19, 2024
View reviewed changes

@szaimenszaimen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice idea!! 🤩

LGTM but didnt test

@szaimen

szaimen commented Nov 19, 2024•
edited
Loading

Copy link
Copy Markdown
Contributor

@skjnldsv Just an idea: can we also somehow check web server body limits and timeouts? (These are also important to test afaik)

@szaimenszaimen mentioned this pull request Nov 19, 2024
Closed
@skjnldsv

skjnldsv commented Nov 19, 2024

Copy link
Copy Markdown
MemberAuthor

@skjnldsv Just an idea: can we also somehow check web server body limits and timeouts? (These are also important to test afaik)

Those variables are not accessible from php

come-nc

This comment was marked as resolved.

Sign in to view

come-nc
come-nc reviewed Nov 19, 2024
View reviewed changes
Comment threadapps/settings/lib/SetupChecks/PhpMaxFileSize.php Outdated
Comment threadapps/settings/lib/SetupChecks/PhpMaxFileSize.php Outdated
@skjnldsv

skjnldsv commented Nov 19, 2024

Copy link
Copy Markdown
MemberAuthor

Is that not crashing when the ini settings are missing?

No, there is always a fallback by php defaults if the value is not defined

@skjnldsv
skjnldsvforce-pushed the feat/php-setup-file-upload branch from 3d988ae to 96daddbCompareNovember 19, 2024 14:08
@skjnldsv
skjnldsvforce-pushed the feat/php-setup-file-upload branch from 96daddb to fb62fc2CompareNovember 19, 2024 14:11
come-nc
come-nc reviewed Nov 19, 2024
View reviewed changes
Comment threadapps/settings/lib/SetupChecks/PhpMaxFileSize.php Outdated
@skjnldsv
feat(settings): add big file upload setup checks
4a88848
Signed-off-by: skjnldsv <skjnldsv@protonmail.com>
@skjnldsv
skjnldsvforce-pushed the feat/php-setup-file-upload branch from fb62fc2 to 4a88848CompareNovember 19, 2024 15:43
@skjnldsv
skjnldsv merged commit 4d85f44 into masterNov 19, 2024
@skjnldsv
skjnldsv deleted the feat/php-setup-file-upload branch November 19, 2024 16:21
This was referenced Nov 19, 2024
Merged
Merged
Merged
@skjnldsvskjnldsv mentioned this pull request Nov 19, 2024
Merged
@nickvergessennickvergessen mentioned this pull request Dec 3, 2024
Merged
4 tasks
@nextcloudnextcloud deleted a comment from nickvergessenDec 3, 2024
@ThibautPlg

ThibautPlg commented Dec 18, 2024•
edited
Loading

Copy link
Copy Markdown
Contributor

Hi,
I've just talked to the NC support about this subject last month and we came to the conclusion that chunking was used across Nextcloud (excluding public shares in old NC versions) so big values were not required anymore.
Do I miss something here?
Best regards,

EDIT: just saw this was disabled now (#49633)

@skjnldsv

skjnldsv commented Dec 19, 2024

Copy link
Copy Markdown
MemberAuthor

We're discussing this internally :)

@skjnldsvskjnldsv mentioned this pull request Jan 7, 2025
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@szaimenszaimenszaimen approved these changes
@come-nccome-nccome-nc approved these changes
@nickvergessennickvergessenAwaiting requested review from nickvergessen
@icewind1991icewind1991Awaiting requested review from icewind1991icewind1991 is a code owner automatically assigned from nextcloud/server-backend
@nfebenfebeAwaiting requested review from nfebenfebe is a code owner automatically assigned from nextcloud/server-backend

Assignees

@skjnldsvskjnldsv

Labels

3. to reviewWaiting for reviewsenhancementfeature: settings

Projects

None yet

Milestone

Nextcloud 31

Development

Successfully merging this pull request may close these issues.

4 participants

@skjnldsv@szaimen@ThibautPlg@come-nc