Skip to content

[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

Open
wants to merge 16 commits into
base: 7.4
Choose a base branch
from

Conversation

symfonyaml
Copy link
Contributor

Q A
Branch? 7.3
Bug fix? no
New feature? yes
Deprecations? no
License MIT

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

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;
}

throw new UnexpectedTypeException($constraint, Video::class);
}

if (!class_exists(\FFMpeg\FFProbe::class)) {
Copy link
Member

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

Copy link
Contributor Author

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.

Copy link
Member

@GromNaN GromNaN Feb 27, 2025

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.

Copy link
Contributor Author

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.

@OskarStark OskarStark changed the title [Validator] Add the Video constraint for validating video files [Validator] Add Video constraint for validating video files Jan 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
Copy link
Contributor Author

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 the PHP-FFMpeg/PHP-FFMpeg repo PHP-FFMpeg/PHP-FFMpeg#940

@mindaugasvcs
Copy link
Contributor

mindaugasvcs commented Feb 27, 2025

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

@mindaugasvcs
Copy link
Contributor

Q A
Branch? 7.3
Bug fix? no
New feature? yes
Deprecations? no
License MIT

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

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 commented Feb 27, 2025

@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
Copy link
Contributor Author

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

@mindaugasvcs
Copy link
Contributor

mindaugasvcs commented Feb 27, 2025

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

Copy link
Contributor

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

}
}

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.

===

Copy link
Contributor Author

@symfonyaml symfonyaml Mar 4, 2025

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
Copy link
Contributor

@Spomky Spomky Mar 4, 2025

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

@mindaugasvcs
Copy link
Contributor

?

@fabpot fabpot modified the milestones: 7.3, 7.4 May 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants