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

413 nginx error fix #121

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

413 nginx error fix #121

wants to merge 3 commits into from

Conversation

nieka
Copy link
Contributor

@nieka nieka commented May 15, 2019

Added validation for file size in agenda and niews iten create/edit screen. The max file size is 20 mb
#42

Copy link
Contributor

@wouterbles wouterbles left a comment

Choose a reason for hiding this comment

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

Wat voor max post size willen we op de server instellen? Op dit moment is het 2MB, lijkt me opzich geen slecht idee om een enigszins kleine bestandsgrootte in te stellen totdat we eventueel hebben ingesteld dat afbeeldingen die op pagina's worden geupload via summernote ook geresized worden.

Zou ook mooi zijn om de ze filehandler toe te voegen aan de summernote afbeeldinge upload.

@@ -18,6 +18,7 @@
"date" => "Invalid birth date",
"error" => "Error",
'bannedEmailDomain' => "Email addresses of educational institutions (student.tue.nl) are not allowed to make sure you can still receive our emails after you graduate",
'fileToLarge' => 'Files is to large. The max size is 20mb',
Copy link
Contributor

Choose a reason for hiding this comment

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

File is too large. The max size is 20MB.

@@ -18,6 +18,7 @@
"date" => "Geen geldige geboortedatum",
"error" => "Error",
'bannedEmailDomain' => "Email adressen van onderwijsinstellingen (student.tue.nl) zijn niet toegestaan om te voorkomen dat je onze mails niet meer ontvangt na je afstuderen",
'fileToLarge' => 'Bestanden is te groot. Bestanden mogen maximaal 20mb zijn',
Copy link
Contributor

Choose a reason for hiding this comment

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

Bestand is te groot.....

@wouterbles
Copy link
Contributor

Laravel heeft ook de mogelijkheid om gewoon in de controller validation 'thumbnail' => 'required|size:1000' te doen. Dat lijkt me persoonlijk een iets nettere oplossing dan het filehandler script. Misschien dat dat script nog wel handig is om max file upload te checken voor de summernote images. Op dit moment faalt die upload met een error in de console, nogal onduidelijk voor gebrukers.

@nieka
Copy link
Contributor Author

nieka commented May 21, 2019

@wouterbles Je kan server side niet checken op file size omdat nginx er daarvoor al uitklapt. Daarom moet dit client side gebeuren

@wouterbles
Copy link
Contributor

Bij mij lokaal kan het iig, wel. Zou dat komen omdat ik xampp draai? Draai jij dat niet ook lokaal?

@wouterbles
Copy link
Contributor

Even een korte demo van dat het bij mij lokaal werkt:

image

Dit is met de code 'thumbnail' => 'required|size:1000'

@nieka
Copy link
Contributor Author

nieka commented May 21, 2019

Lokaal gebruik je apache die dit probleem niet heeft. Ik gebruik lokaal ook ngix waardoor ik het probleem wel heb

@wouterbles
Copy link
Contributor

Voordat dit gemerged kan worden, op de server staat nu 2 MB ingesteld, willen we dit verhogen, en zo ja naar wat? 20 MB lijkt me persoonlijk wel erg veel. Ook nog even naar mijn comments hierboven kijken ;)

@codecov
Copy link

codecov bot commented Jan 8, 2020

Codecov Report

❗ No coverage uploaded for pull request base (master@cbefbf2). Click here to learn what that means.
The diff coverage is 18.81%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #121   +/-   ##
=========================================
  Coverage          ?   24.42%           
  Complexity        ?      577           
=========================================
  Files             ?       81           
  Lines             ?     2027           
  Branches          ?        0           
=========================================
  Hits              ?      495           
  Misses            ?     1532           
  Partials          ?        0
Impacted Files Coverage Δ Complexity Δ
app/AgendaItem.php 81.81% <ø> (ø) 11 <0> (?)
app/repositories/RepositorieFactory.php 95.65% <ø> (ø) 3 <0> (?)
app/Http/Controllers/PendingUserController.php 79.16% <ø> (ø) 7 <0> (?)
app/PhotoAlbum.php 50% <ø> (ø) 2 <0> (?)
app/Http/Controllers/frontEndController.php 0% <0%> (ø) 20 <2> (?)
app/repositories/PhotoAlbumRepository.php 0% <0%> (ø) 8 <1> (?)
app/Http/Controllers/Api/UserController.php 0% <0%> (ø) 2 <2> (?)
app/Http/Controllers/PhotoController.php 0% <0%> (ø) 23 <0> (?)
app/Http/Controllers/PhotoAlbumController.php 0% <0%> (ø) 7 <6> (?)
app/MailList.php 0% <0%> (ø) 8 <0> (?)
... and 13 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cbefbf2...d0036dd. Read the comment docs.

@indiePeeters
Copy link
Contributor

Dit gaat vooral toch om de thumbnail die bij een agenda item geupload word? hiervoor lijkt mij 10mb max voldoende. Het lijkt me dat sommige nog wel foto's die gemaakt zijn op cameras willen uploaden die vaak tussen de 8 en 10 mb liggen.

@wouterbles
Copy link
Contributor

10 lijkt me ook prima, dan moet het limiet ook even op de server worden ingesteld, anders heeft dit niet heel veel zin denk ik

@indiePeeters
Copy link
Contributor

Dit gaat vooral toch om de thumbnail die bij een agenda item geupload word? hiervoor lijkt mij 10mb max voldoende. Het lijkt me dat sommige nog wel foto's die gemaakt zijn op cameras willen uploaden die vaak tussen de 8 en 10 mb liggen.

Bij nader inzien lijkt me het handig om de max_upload_size niet naar 10MB te zetten. Dit omdat ik tot nu toe bij de foto functie geen bestanden boven de 3MB heb gezien. (aangezien het nu op 2MB staat is dit de reden dat sommige foto's falen)

Daarom heb ik in commit c252fb5 de downscale en resize functie uit de photoalbum javascript getrokken zodat het ook op andere plekken gebruikt kan worden.

Lijkt jullie het dan wat om voor deze issue de thumbnail in de front end te downscalen? en vervolgens de max_upload_size naar 4/5MB zetten?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants