Skip to content
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

Replace boolean cast to be able to disable frame, aspect ratio, trans… #7262

Merged
merged 2 commits into from
Mar 23, 2017
Merged

Replace boolean cast to be able to disable frame, aspect ratio, trans… #7262

merged 2 commits into from
Mar 23, 2017

Conversation

joost-florijn-kega
Copy link
Contributor

@joost-florijn-kega joost-florijn-kega commented Nov 1, 2016

…parency or constrain only in view.xml

If you set frame to false in view.xml the code would cast it to true because (bool)'false' is equal to true. By using the filter_var function the cast to boolean is done correctly.

…parency or constrain only in view.xml

If you set frame to false in view.xml the code would cast it to true because (bool)'false' is equal to true. By using the filter_var function the cast to boolean is done correctly.
@joost-florijn-kega
Copy link
Contributor Author

This will fix issue #4622

@@ -278,7 +278,7 @@ public function getQuality()
*/
public function setKeepAspectRatio($keep)
{
$this->_keepAspectRatio = (bool)$keep;
$this->_keepAspectRatio = filter_var($keep, FILTER_VALIDATE_BOOLEAN);
Copy link
Contributor

Choose a reason for hiding this comment

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

Would something like

$this->_keepAspectRatio = $keep && $keep !== 'false';

do the trick and make an intention clearer for somebody who read the code? ;)

@joost-florijn-kega
Copy link
Contributor Author

@orlangur It is indeed a better way. I have changed it.

@Ctucker9233
Copy link

@orlangur Is this PR going to be merged?

@orlangur
Copy link
Contributor

@Ctucker9233 I believe yes, as it's still perfect :)

Cannot do it by myself, here you can track the progress of PR processing: https://github.com/magento/magento2/projects/3

It will take some time to process all old PRs, if you really need this fix in next release please ping somebody from Magento team. Cannot evaluate severity of this issue by myself.

@Ctucker9233
Copy link

@magento-team Would like to request that this PR be merged.

@magento-team magento-team merged commit 1bbcff4 into magento:develop Mar 23, 2017
magento-team pushed a commit that referenced this pull request Mar 23, 2017
@ishakhsuvarov
Copy link
Contributor

@Ctucker9233 @joost-florijn-kega PR have been merged into the develop branch. Thank you.

@@ -278,7 +278,7 @@ public function getQuality()
*/
public function setKeepAspectRatio($keep)
{
$this->_keepAspectRatio = (bool)$keep;
$this->_keepAspectRatio = $keep && $keep !== 'false';
Copy link

@eldonbite eldonbite Jan 8, 2018

Choose a reason for hiding this comment

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

Actually, you could probably just do a json_decode($keep) here..

Copy link
Contributor

Choose a reason for hiding this comment

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

Even if json_decode works it would be much slower and much less clear.

Choose a reason for hiding this comment

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

Yeah, point well-taken 👍

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.

7 participants