spicywebau / craft-embedded-assets

Manage YouTube videos, Instagram photos and more as first class assets in Craft CMS
MIT License
171 stars 35 forks source link

EmbeddedAssetImage's size attribute has an unexpected value #248

Closed tremby closed 9 months ago

tremby commented 9 months ago

Bug Description

Assets in Craft have a size field which is the file size in bytes of the asset.

The EmbeddedAssetImage objects coming back as images attached to video embeds also have a size field, whose docs say "The size of the image". I think I'm probably not alone in expecting that to be equivalent to Craft's own assets' size field. In fact, it seems to be the asset's area, i.e. width * height.

(I guess the idea is that this can be sorted by, so the developer can pick the largest version? But it's no bother to grab width and height and multiply them if that's what the developer wants.)

Steps to reproduce

  1. Query (I'm using GQL) for a video asset, including its images and their size fields.

Expected behaviour

File size or null if not known, or call it area instead, or at least change the field's docs to make its purpose clearer.

Embedded Assets version

3.2.0

Craft CMS version

4.5.11.1

ttempleton commented 9 months ago

Much like #247, this data is being provided by the Embed library, so you'd need to take up the issue with them if you believe it's incorrect. That said, a search I did for "image size" meaning suggests that it is not uncommon for the term "image size" to refer to the size in terms of the number of pixels.

tremby commented 9 months ago

Sure, camera people talk about image/sensor size in megapixels which is area. But we are in the context of Craft; when Craft already has a field called size which means something different, surely you can see how it could be misleading.

I can't tell where in the upstream code it's coming from. Maybe it comes from the providers.

Consider at least changing the docstring?

ttempleton commented 9 months ago

In Embed v3, which Embedded Assets currently uses, image sizes are calculated here.

I don't believe any change to our documentation is required, since the PHP function getimagesize() returns an array containing (among other data not including the file size) the size of the image in terms of its dimensions. It could be argued that Craft is the inconsistent one in using the term size to refer to the asset's size in bytes, and that it should be renamed to filesize to avoid confusion when the asset is an image.

tremby commented 9 months ago

In Embed v3, which Embedded Assets currently uses, image sizes are calculated here.

OK, well that seems to be gone, so I've nothing to report upstream.

I don't believe any change to our documentation is required, since the PHP function getimagesize() returns an array containing (among other data not including the file size) the size of the image in terms of its dimensions.

I don't see how that's really relevant. The target audience for the docs is people consuming the data, which I would have thought would be largely Craft template authors (many of whom do not often touch the bare PHP API), and people like me consuming the GQL API, who may be using any language at all and who may have never even touched PHP.

If something called "getimagesize" promises me an array, sure, I'll expect it to be dimensions. But never would I expect an integer coming back for "size" to be "area", especially when it's also giving me width and height from which I can trivially calculate area.

It's caused confusion with at least one user, me. If you really don't think it's worth making a tiny doc change to eliminate that confusion, fair enough. Though I don't see what the downside would be.