Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[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

Open
symfonyaml wants to merge16 commits intosymfony:7.4
base:7.4
Choose a base branch
Loading
fromsymfonyaml:video-validator

Conversation

symfonyaml
Copy link
Contributor

QA
Branch?7.3
Bug fix?no
New feature?yes
Deprecations?no
LicenseMIT

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

namespaceApp\Entity;useSymfony\Component\HttpFoundation\File\File;useSymfony\Component\Validator\ConstraintsasAssert;class Tutorial{    #[Video(maxWidth:1920, maxHeight:1080)]privateFile$video;}

skigun, GromNaN, and Spomky reacted with thumbs up emoji
throw new UnexpectedTypeException($constraint, Video::class);
}

if (!class_exists(\FFMpeg\FFProbe::class)) {

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

symfonyaml and GromNaN reacted with thumbs up emoji
Copy link
ContributorAuthor

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.

Copy link
Member

@GromNaNGromNaNFeb 27, 2025
edited
Loading

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.

Copy link
ContributorAuthor

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.

@OskarStarkOskarStark changed the title[Validator] Add the Video constraint for validating video files[Validator] AddVideo constraint for validating video filesJan 9, 2025
@nicolas-grekas
Copy link
Member

Looks like a PR should be contributed upstream to fix this:

  1x: Method "Alchemy\BinaryDriver\AbstractBinary::getName()" might add "string" as a native return type declaration in the future. Do the same in child class "FFMpeg\Driver\FFProbeDriver" now to avoid errors or add an explicit @return annotation to suppress this message.

Can you submit it?

symfonyaml reacted with thumbs up emoji

@symfonyaml
Copy link
ContributorAuthor

Looks like a PR should be contributed upstream to fix this:

  1x: Method "Alchemy\BinaryDriver\AbstractBinary::getName()" might add "string" as a native return type declaration in the future. Do the same in child class "FFMpeg\Driver\FFProbeDriver" now to avoid errors or add an explicit @return annotation to suppress this message.

Can you submit it?

@nicolas-grekas PR created in thePHP-FFMpeg/PHP-FFMpeg repoPHP-FFMpeg/PHP-FFMpeg#940

symfonyamland others added14 commitsJanuary 12, 2025 18:53
Co-authored-by: Nicolas Grekas <nicolas.grekas@gmail.com>
@mindaugasvcs
Copy link
Contributor

mindaugasvcs commentedFeb 27, 2025
edited by OskarStark
Loading

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.');}

@mindaugasvcs
Copy link
Contributor

QA
Branch?7.3
Bug fix?no
New feature?yes
Deprecations?no
LicenseMIT

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

namespace App\Entity;use Symfony\Component\HttpFoundation\File\File;use Symfony\Component\Validator\Constraints as Assert;class Tutorial{    #[Video(maxWidth: 1920, maxHeight: 1080)]    private File $video;}

Tell me you are not promoter of this useless ffmpeg wrapper.

@nicolas-grekas
Copy link
Member

nicolas-grekas commentedFeb 27, 2025
edited
Loading

@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.

symfonyaml, chapterjason, and Spomky reacted with heart emoji

@symfonyaml
Copy link
ContributorAuthor

@mindaugasvcs I agree that using ffmpeg is not fantastic, I am totally open for better suggestions and good intentions ;)

@mindaugasvcs
Copy link
Contributor

@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.

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?

@nicolas-grekas
Copy link
Member

@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?

chapterjason reacted with thumbs up emoji

@mindaugasvcs
Copy link
Contributor

mindaugasvcs commentedFeb 27, 2025
edited
Loading

@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:

  1. Working with videos isn't so straightforward as working with images: videos can have multiple video streams, multiple audio streams, multiple text streams, videos can even be 3D aka stereo ones. Not every Symfony web site / web app needs a validator of videos, even if they needed one, they would most probably find this implementation too basic.
  2. Validation of videos should be left for a custom implementation at application level because it's very dependent of business needs. Other reasons why: SOLID and ease of configuration: possibility to pass constraint values via DI, system admins sometimes aren't programmers.
  3. No other major web framework provides support for video files out of box (most probably because of reasons I just outlined).

Reasons why this wrapper is bad as implementation of such idea:

  1. One more layer of complexity and maintenance: when this project dies the implementation will automatically turn into technical debt.
  2. It's not supported by PHP nor PHP community.
  3. Enormous amount of time spent just to learn how to use it, even for basic stuff.
chapterjason reacted with eyes emoji

Copy link
Contributor

@SpomkySpomky left a 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.

symfonyaml reacted with thumbs up emoji
}
}

if (!$constraint->allowSquare && $width == $height) {
Copy link
Contributor

Choose a reason for hiding this comment

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

===

symfonyaml reacted with eyes emoji
Copy link
ContributorAuthor

@symfonyamlsymfonyamlMar 4, 2025
edited
Loading

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

OskarStark reacted with thumbs up emoji
@@ -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
Copy link
Contributor

@SpomkySpomkyMar 4, 2025
edited
Loading

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).

OskarStark reacted with thumbs up emoji
@mindaugasvcs
Copy link
Contributor

?

@fabpotfabpot modified the milestones:7.3,7.4May 26, 2025
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@nicolas-grekasnicolas-grekasnicolas-grekas left review comments

@GromNaNGromNaNGromNaN left review comments

@stofstofstof left review comments

@SpomkySpomkySpomky left review comments

Assignees
No one assigned
Projects
None yet
Milestone
7.4
Development

Successfully merging this pull request may close these issues.

8 participants
@symfonyaml@nicolas-grekas@mindaugasvcs@GromNaN@stof@Spomky@fabpot@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp