Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
[Validator] AddVideo
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 ourterms of service andprivacy 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
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
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 libraryegulias/email-validator
is installed in theEmail and also in theEmailValidator.
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 theVideo
class could be extended and the constructor skipped, which would skip the dependency check.
Uh oh!
There was an error while loading.Please reload this page.
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 defineffmpeg
as a system dependency, so the tests can pass.
Video
constraint for validating video filesUh oh!
There was an error while loading.Please reload this page.
Looks like a PR should be contributed upstream to fix this:
Can you submit it? |
@nicolas-grekas PR created in the |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
…led in constraint class
Co-authored-by: Nicolas Grekas <nicolas.grekas@gmail.com>
mindaugasvcs commentedFeb 27, 2025 • edited by OskarStark
Loading Uh oh!
There was an error while loading.Please reload this page.
edited by OskarStark
Uh oh!
There was an error while loading.Please reload this page.
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) {thrownewLogicException('This validator requires ffprobe installed and enabled in the PATH system variable.');} |
Tell me you are not promoter of this useless ffmpeg wrapper. |
nicolas-grekas commentedFeb 27, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@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? |
mindaugasvcs commentedFeb 27, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
===
symfonyamlMar 4, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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 onImageValidator
@@ -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 aboutAdd theVideo
constraint to validate video files and their properties (e.g., dimensions and pixel density).
? |
Purpose
Inspired by theImage constraint, I've added a new feature to the Validator component for validating Video files using thePHP-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