-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Validator] Add Video
constraint for validating video files
#59042
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
base: 7.4
Are you sure you want to change the base?
Conversation
throw new UnexpectedTypeException($constraint, Video::class); | ||
} | ||
|
||
if (!class_exists(\FFMpeg\FFProbe::class)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need for this check: if a Video instance is given, it's been already verified
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nicolas-grekas I actually took the Email constraint as an example :
It also checks twice if library egulias/email-validator
is installed in the Email and also in the EmailValidator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nicolas-grekas the Video
class could be extended and the constructor skipped, which would skip the dependency check.
5c003e6
to
4e1502a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nicolas-grekas Thank you for your first review.
I will need some help for CI, to define ffmpeg
as a system dependency, so the tests can pass.
Video
constraint for validating video files
src/Symfony/Component/Validator/Tests/Constraints/VideoValidatorTest.php
Show resolved
Hide resolved
Looks like a PR should be contributed upstream to fix this:
Can you submit it? |
dbca1fd
to
19518bc
Compare
@nicolas-grekas PR created in the |
src/Symfony/Component/Validator/Tests/Constraints/VideoValidatorTest.php
Show resolved
Hide resolved
ce966c9
to
c96f852
Compare
…led in constraint class
Co-authored-by: Nicolas Grekas <nicolas.grekas@gmail.com>
df0ce8c
to
cf56118
Compare
I have done this a long time ago. Just why use classes instead of ffprobe directly? $json = shell_exec('ffprobe -hide_banner -loglevel quiet -show_error -show_format -show_streams -print_format json '.$value);
if (!$json) {
throw new LogicException('This validator requires ffprobe installed and enabled in the PATH system variable.');
} |
Tell me you are not promoter of this useless ffmpeg wrapper. |
@mindaugasvcs please express yourself in a more friendly manner. You might be right that the wrapper is not needed, but calling the wrapper "useless" or suggesting bad intentions as a hidden "promoter" is detrimental to having a constructive discussion. Open source requires respectful discussions. |
@mindaugasvcs I agree that using ffmpeg is not fantastic, I am totally open for better suggestions and good intentions ;) |
First of all, you should not have accepted this PR in the first place as we now both agree the wrapper is not needed. I believe this PR should not be a part of the framework. I asked you long time before about such validator, you gave me no answer back then. And now? WTF?! May I ask to explain yourself? |
@mindaugasvcs Let's keep the discussion constructive. If you raised this idea earlier, I understand your frustration. That said, decisions in open source evolve based on contributions. Discussions are only a first step. Here, we have a patch, and that's how things happen. If you have strong technical reasons why this validator should not be part of the framework, let's focus on those. Do you see specific downsides beyond just the existence of the wrapper? |
Reasons why including such a basic video validator is bad idea:
Reasons why this wrapper is bad as implementation of such idea:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few minor remarks. Overall it looks good.
} | ||
} | ||
|
||
if (!$constraint->allowSquare && $width == $height) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
===
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which means there is the same error on ImageValidator
@@ -6,6 +6,7 @@ CHANGELOG | |||
|
|||
* Add support for ratio checks for SVG files to the `Image` constraint | |||
* Add the `Slug` constraint | |||
* Add the `Video` constraint for validating video files |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about Add the Video
constraint to validate video files and their properties (e.g., dimensions and pixel density).
? |
Purpose
Inspired by the Image constraint, I've added a new feature to the Validator component for validating Video files using the PHP-FFMpeg library.
This constraint use exactly the same concept as the Image one, with the same options except for the corrupted options (detectcorrupted, corruptedmessage)
I think we can definitely apply more options for video files, but first I just want to check if everyone is okay with this constraint.
Exemple