Skip to content

Image::place() converts percentage arguments to numeric value - #122

Merged
dg merged 1 commit into
nette:masterfrom
PavelJurasek:patch-1
Oct 24, 2016
Merged

Image::place() converts percentage arguments to numeric value#122
dg merged 1 commit into
nette:masterfrom
PavelJurasek:patch-1

Conversation

@PavelJurasek

@PavelJurasek PavelJurasek commented Oct 19, 2016

Copy link
Copy Markdown
Contributor

Fixes compatibility with PHP 7.1 as it caused E_NOTICE: A non well formed numeric value encountered in Image.php:474.

@dg

dg commented Oct 19, 2016

Copy link
Copy Markdown
Member

Please fix it here too https://github.com/PavelJurasek/utils/blob/efb19fe940d5f152174688ba25d4af0714fd737d/src/Utils/Image.php#L324-L336

@PavelJurasek

PavelJurasek commented Oct 19, 2016

Copy link
Copy Markdown
Contributor Author

Done

@PavelJurasek

PavelJurasek commented Oct 20, 2016

Copy link
Copy Markdown
Contributor Author

Is anything else needed?

@dg

dg commented Oct 22, 2016

Copy link
Copy Markdown
Member

abs() is missing

@PavelJurasek

PavelJurasek commented Oct 22, 2016

Copy link
Copy Markdown
Contributor Author

Fixed

@dg

dg commented Oct 24, 2016

Copy link
Copy Markdown
Member

Thanks

@dg
dg merged commit 53e7466 into nette:master Oct 24, 2016
dg pushed a commit that referenced this pull request Oct 26, 2016
@PavelJurasek @dg
Image: fix conversion of percentage arguments to numeric values (#122)
8683a0b
dg
dg reviewed May 2, 2017
View reviewed changes
Comment thread src/Utils/Image.php Outdated
{
if (is_string($newWidth) && substr($newWidth, -1) === '%') {
$newWidth = (int) round($srcWidth / 100 * abs($newWidth));
$newWidth = (int) round($srcWidth / 100 * substr($newWidth, 0, -1));

@dg dg Oct 19, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think that abs() should stay here

@PavelJurasek PavelJurasek May 2, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

8683a0b was merged. abs is present in that commit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dg dg dg left review comments
+1 more reviewer
@JanTvrdik JanTvrdik JanTvrdik approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@PavelJurasek @dg @JanTvrdik