[Validator] Support SVGs when validating image dimensions #45486
Conversation
|
Hey! I see that this is your first PR. That is great! Welcome! Symfony has a contribution guide which I suggest you to read. In short:
Review the GitHub status checks of your pull request 8000 and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change. When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor! I am going to sit back now and wait for the reviews. Cheers! Carsonbot |
|
Hey! I think @jschaedl has recently worked with this code. Maybe they can help review this? Cheers! Carsonbot |
| /** | ||
| * @param mixed $value | ||
| * | ||
| * @return bool | ||
| */ | ||
| private function isSvg($value) |
There was a problem hiding this comment.
Please use native type declarations. Can we be more precise about the $value type?
There was a problem hiding this comment.
$value can be an instance of File or string. that's way I put mixed
upadte validator changelog Use Mime Component, Check for corrupted XML
4a55bc5 to
a4ace64
Compare
|
@nicolas-grekas @derrabus can you re-check, please? |
| $width = intval($svgAttributes->width); | ||
| $height = intval($svgAttributes->height); | ||
|
|
||
| return [$width, $height]; |
There was a problem hiding this comment.
I don't know if it's common for others, but I usually work with SVG files and the following is not that uncommon:
- To not have any
widthorheightattribute and just rely on theviewBoxattribute dimensions - To have values like
100%in thewidthorheightattributes because those images are embedded in other parent elements with some height/width defined
There was a problem hiding this comment.
I agree with @javiereguiluz here. Does it mean that this is not that useful? The size of an SVG is not that important anyway, maybe just the ratio? cc @symfony/mergers
There was a problem hiding this comment.
maybe just the ratio?
I agree this one would be important.
There was a problem hiding this comment.
Just to be clear, I would only implement the ratio validation for SVGs.
There was a problem hiding this comment.
I would be fine to also validate width and height if we make sure to skip this validation in case the given width or height are relative values (like mentioned by Javier).
There was a problem hiding this comment.
But I fail to see how this is useful. A 100x200 SVG file looks perfectly fine at 20x40 or 2000x4000.
|
I'm going to close here for the reasons explained in the code review and because there is no more feedback here. |
…imecolin) This PR was merged into the 7.3 branch. Discussion ---------- [Validator] Validate SVG ratio in Image validator | Q | A | ------------- | --- | Branch? | 7.3 | Bug fix? | no | New feature? | yes | Deprecations? | no | Issues | Fix #45269 | License | MIT Implement ratio check for SVG images. Checking SGV size is not relevant as a SVG image can be enlarged without loss, but ratio can be important to check. Currently, the validator add a violation with `SIZE_NOT_DETECTED_ERROR` in case of SVG image. SVG size is guessed from viewbox, width and height attributes. Viewbox will provides default size, width and height can override viewbox size if they are number. Width and height as percentage are ignored as the final size will depend on the container. I use `preg_match` instead of `\DomDocument` or `simplexml` functions to extract viewBox, width and height in order to avoid new dependencies on `ext-dom` or `ext-simplexml`. In case of SVG, `minWidth`, `maxWidth`, `minHeight`, `maxHeight`, `minPixels` and `maxPixels` are ignored because not relevant. Only `maxRatio`, `minRatio`, `allowSquare`, `allowLandscape` and `allowPortrait` can generate violations, like suggested in the comments of the abandoned #45486. Commits ------- c4ad092 [Validator] Validate SVG ratio in Image validator
Add SVG support in Image Validator