Deserialize the image size when using maxWidth #2632
Conversation
| @@ -1589,6 +1589,8 @@ public static function addImageToTemplate($template, array $rowData, $maxWidth = | |||
| } | |||
|
|
|||
| // Normalize size | |||
| $size = StringUtil::deserialize($size); | |||
There was a problem hiding this comment.
This cannot be the correct place to deserialize the string. It should be in line 1605 inside the else condition IMHO, don't you think?
There was a problem hiding this comment.
We're also deserializing any input in the ImageFactory and that's what was in place before as well.
So - in theory - someone could rely on it working with a single serialized integer for example.
There was a problem hiding this comment.
That is weird as well then. 😄 If we know that $size can be numeric or an object, why would we try to deserialize it without checking if deserializing is necessary first?
There was a problem hiding this comment.
It could for instance be a numeric value ("123") or a serialized numeric value (s:3:"123";). This is at least what the old implementation allowed and the current PictureFactory is allowing.
In fact $size can imo be one of int|string|array|PictureConfiguration|null with string including all possible serialized versions ofint|string|array|null.
There was a problem hiding this comment.
You are missing the point.
$size = StringUtil::deserialize($size);
if ($size instanceof PictureConfiguration)
{
return array($size, $margin);
}StringUtil::deserialize() does not allow serialized objects, so this condition can only be true if $size is already an object before it is passed to StringUtil::deserialize(). It therefore does not make sense to have the instanceof check after StringUtil::deserialize(). It should be:
if ($size instanceof PictureConfiguration)
{
return array($size, $margin);
}
$size = StringUtil::deserialize($size);There was a problem hiding this comment.
Yes, I know and StringUtil::deserialize() is the identity function for objects 🙂. I was referring to your initial comment suggesting to put the line in the else clause where it would miss the serialized numeric case.
Nevermind, I moved it after the instanceof check in 1b0bc6b.
|
Thank you @m-vo. |
leofeyer
changed the title
Description ----------- | Q | A | -----------------| --- | Fixed issues | - | Docs PR or issue | - We must deserialize the image size when someone is using the legacy `maxWidth`/`maxImageWidth` parameter/config option in `Controller::addImageToTemplate`. Proof of failing test is in 92ad5f3. Commits ------- 92ad5f3 test maxWidth with serialized size 72763bc always deserialize size when using maxWidth fallback 1b0bc6b deserialize after checking for object instance aaf92e5 Merge branch '4.10' into bugfix/max-width-deserialize-size
We must deserialize the image size when someone is using the legacy
maxWidth/maxImageWidthparameter/config option inController::addImageToTemplate.Proof of failing test is in 92ad5f3.