Skip to content

2.x Fix implicit conversion from float to int #2775

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.

Sign up for GitHub

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

Jump to bottom
Merged
gchtr merged 7 commits into from
Jul 28, 2023
Merged

2.x Fix implicit conversion from float to int #2775

gchtr merged 7 commits into from
Jul 28, 2023

Conversation

@gchtr
Copy link
Member

@gchtr gchtr commented Jul 20, 2023

Related:

Issue

We require dimensions to be int, but SVG might have float dimensions.

Solution

Convert to int.

Impact

No deprecation notice.

Usage Changes

None.

Considerations

Should we round the values before typecasting them to int using round()?

I think we also have to consider whether it might be important to get SVG dimensions as floats. For using the dimensions as width and height attributes in HTML, it has to be an integer value.

But maybe there’s a use case where you’ll want to access SVG dimensions as floats. In that case, we might somehow have to give access to ImageDimensions::get_dimensions_svg(), which is protected at the moment:

timber/src/ImageDimensions.php

Line 164 in a23d781

protected function get_dimensions_svg($svg)

Testing

No.

@gchtr gchtr added the 2.0 label Jul 20, 2023
@gchtr gchtr requested review from jarednova and nlemoine as code owners July 20, 2023 07:59
@Levdbas
Copy link
Member

Levdbas commented Jul 24, 2023

I think there is no need to disclose the original values via get_dimensions_svg() since having SVG widths and heights as floats are bad to begin with in my experience. I exerienced a lot of issues with for example hairlines when scaling a svg down with a large number after the decimal as width/height. And I think rounding the numbers is the way to go before typecasting them.

@Levdbas
Copy link
Member

Levdbas commented Jul 25, 2023

It seems like the tests asserts that the SVG viewbox width of 530.91 is rounded down to 530, instead of up. In my opinion the current round function is what we would like.

We could change the testing value to 531 or the actual svg viewbox width 529.91, which gets then rounded up to 530?

But correct me if I am wrong @gchtr !

Levdbas
Levdbas previously approved these changes Jul 27, 2023
View reviewed changes
@gchtr gchtr merged commit 9c344c9 into 2.x Jul 28, 2023
@gchtr gchtr deleted the 2.x-implicit-conversion-from-float-to-int branch July 28, 2023 06:24
@gchtr
Copy link
Member Author

gchtr commented Jul 28, 2023

@Levdbas Thanks for the hint! I didn’t runt the tests locally, so I missed the rounded numbers issue.

@gchtr gchtr mentioned this pull request Jul 28, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Levdbas Levdbas Levdbas left review comments

@jarednova jarednova Awaiting requested review from jarednova

@nlemoine nlemoine Awaiting requested review from nlemoine nlemoine is a code owner

Assignees

No one assigned

Labels

2.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@gchtr @Levdbas