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

Adding new 'cmf_media_file' form type #137

Closed eiannone closed 9 years ago

eiannone commented 9 years ago
Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #99, #136
License MIT
Doc PR https://github.com/symfony-cmf/symfony-cmf-docs/pull/689

To do:

As discussed in https://github.com/symfony-cmf/MediaBundle/issues/99, the MediaBundle seems to miss a 'cmf_media_file' form type, like the already defined 'cmf_media_image'. This form type would be useful when a file must be embedded/attached to a container document, and maybe useful also for sonata admin.

eiannone commented 9 years ago

One build is failing due to a Twig compiler problem (Twig v1.12.0-RC1). It generates a compiled template with a syntax error. This is not related to my commit. Also master branch is affected. It's a bug in Twig v1.12.0-RC1, which was fixed in v1.12.0 (missing 'array' string in https://github.com/twigphp/Twig/blob/v1.12.0-RC1/lib/Twig/Node/Expression/Call.php#L24). Don't know why travis is using that RC version instead of the stable one. Maybe because the '--prefer-lowest' flag in composer?

dbu commented 9 years ago

hey, i like this one very much! the refactoring is great, and you even added tests for this!

the test failure is most likely unrelated. we do one build with --prefer-lowest exactly for the reason to see if things work or if we need to fix the minimum requirements in composer.json. i created #138 about that - we won't block this pull request because of it.

eiannone commented 9 years ago

@dbu thank you for your review. I've addressed your points, and hope now the code is better documented.

dbu commented 9 years ago

great job, thanks a lot! i created the doc issue, please do a pull request for the documentation. it can be short, but at least the user should find a hint that this exists.

lsmith77 commented 9 years ago

please rebase on master to get the tests fixed.

eiannone commented 9 years ago

Rebased (and squashed). All test are ok now. I've also updated documentation, see https://github.com/symfony-cmf/symfony-cmf-docs/pull/689

dbu commented 9 years ago

good job, thanks!