symfony-cmf / media-bundle

UNMAINTAINED - Minimalistic interfaces to handle media in the context of the CMF
http://cmf.symfony.com/
30 stars 40 forks source link

[POC] Added validation #55

Closed dantleech closed 11 years ago

dantleech commented 11 years ago
Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? N/A (yet)
Fixed tickets N/A
License MIT
Doc PR N/A

I was scratching my head for a while when I uploaded files using the test application - I was getting a File '' not found message. Turns out the file exceeded my max_filesize.

This PR implements the validate method in UploadFileHelper. I also prepared a symfony PR as I think it would be a better place to have this in some way (currently the various UPLOADERR* constants are checked only in the Validation component).

But I wanted to validate the idea here first.

lsmith77 commented 11 years ago

/cc @rmsint (not sure if you get a notification when i assign a ticket to you)

rmsint commented 11 years ago

Having this validation is a lot better for the user yes. I think we should decide whether to include this untill Symfony has this change. I am +1.

And then what needs to be done to merge, I propose:

lsmith77 commented 11 years ago

@dantleech can you wrap up the open issues?

dantleech commented 11 years ago

@rmsint I don't think we need to worry about translations, these are Exception messages of last resort, the developer should add validation if they want the user to see nice error messages.

dantleech commented 11 years ago

Added test case. Note that there are some unrelated failures todo with jackalope doctrine_dbal which are preventing me checking this in the Test App, or checking the WebTestCase.

lsmith77 commented 11 years ago

fyi .. tests finally ran through on travis https://travis-ci.org/symfony-cmf/MediaBundle/jobs/11835565#L191

dantleech commented 11 years ago

ok, the tests should be fine again now. I have added a config option to disable the "is_uploaded_file" check, enabling us to disable this check when in the test environment.

lsmith77 commented 11 years ago

needs a rebase

lsmith77 commented 11 years ago

closing in favor of #72