search

craftcms / cms

Build bespoke content experiences with Craft.
https://craftcms.com
Other
3.26k stars 634 forks source link

Asset's width and height are returned as strings instead of integers #7817

Open schwarzdesign opened 3 years ago

schwarzdesign commented 3 years ago

Description

According to the API reference, an image asset's width and height should be returned as integer, float or null. However, Asset::getWidth() and Asset::getHeight consequently return numeric strings, which makes it annoying to do math with them.

Steps to reproduce

I'm not sure if this is caused by my setup, I haven't done anything special. I just created an image volume, an assets field and tried to access the image's width/height in a Twig template. Everywhere I do that the methods consequently return strings.

I looked into the craft\elements\Asset class. Asset::getWidth() and Asset::setWidth() (as well as the corresponding height methods) have docblocks indicating the type as int|float|null, but don't use typehints and don't enforce the type. I'm not sure where the width and height are actually set, but apparently it's setting them as strings and the class doesn't cast them to integer.

We're using PHP 8 with GD (since Imagick isn't supported yet), maybe it's related to that?

Additional info

brandonkelly commented 3 years ago

This happens because the values come back from MySQL as strings, even though they are stored in integer columns.

Will be fixed in Craft 4.0 as we’re adding type declarations to all properties.

schwarzdesign commented 3 years ago

@brandonkelly Thanks for the explanation! Though I'm a bit surprised by this – the database storage shouldn't ever be allowed to influence what I receive from high-level classes and interfaces. But even if this couldn't be prevented due to some technical complication – why include a docblock that's just straight up wrong?

Ok, sorry for the rant, that's unhelpful. But should this bug really have to wait until the next major release? I'd consider this just a bug, since the docblock indicates a return type of int|float|null (BTW: Can an image height ever be a float?) but the class doesn't enforce this. Or, if changing the class to return the (arguably) correct type is considered a breaking change, the next bugfix release should at least fix the docblock to @returns string. At the moment, the docblock is just lying, so users are left wondering whether they did something wrong ...

brandonkelly commented 3 years ago

the database storage shouldn't ever be allowed to influence what I receive from high-level classes and interfaces.

Agree with that, but typecasting all incoming data was deemed not worth the effort, since the problem will resolve itself in Craft 4 thanks to typed property support in PHP 7.4.

That said, we did start typecasting a couple things in ~3.5, which led to several reports of JS code breaking due to the type change, so now we consider it a breaking change that must wait until 4.0.

Or, if changing the class to return the (arguably) correct type is considered a breaking change, the next bugfix release should at least fix the docblock to @returns string.

That’s fair. Though worth mentioning that if you’re using Postgres, types will already be correct. This is a MySQL-specific bug. And the scope is pretty huge (far beyond just these two properties), so again, probably not worth the effort. You could say that the return type can be considered an int, even if technically w/ MySQL, working with it as an int will cause some type juggling.

schwarzdesign commented 3 years ago

@brandonkelly I see, I guess the scope is bigger than I imagined. Thanks for the explanation! I guess I'll just make sure to type-cast the return value to int in my templates for the time being. Looking forward to having typecasting in 4.0!