Refactor file models

[nlemoine]

Issue

First intended to only remove the Filesize class and refactor the size()/size_raw() methods for many reasons:

• Attachment is a model, it's not in its scope to return/format a raw file size into a human readable one. You might want to format a size in other places, this is filters role.
• The size method included an HTML breaking space ( ), which is fine on the Twig side but what if I need the text only version of it? Although Timber is mostly about Twig, models can also be used PHP side and in various contexts (JSON, etc.). The Attachment model should only by responsible for returning a raw file size you can manage yourself afterwards.
• The size was grabbed directly from filesystem, which is too bad because WP 6.0 introduced a filesize key which is set and stored on attachment first upload. The Attachment::size() method now try first to get that value before getting to filesystem which will lead to potentially better performances.

While working on the above issue, I noticed that Attachment was setting every possible properties when building the object (get_info()), getting values from database that might not be needed already and maybe won't at all.

I decided to refactor this class to only grab metadata values when they are called which might lead into more efficient code.

Solution

• Remove Filesize class
• Refactor ExternalImage, Attachment, Image to add types and getting metadata in a more lazy way
• Add format_bytes filter to format raw file size into a human readable file size
• Fix/add tests
• Fix city-museum.jpg by removing exif data which was causing issues in tests

Impact

• Image/Attachment/ExternalImage properties are now protected, return types have changed
• FileSize class has been removed (but it is a BC since it was introduced in 2.0?)

Usage Changes

If you want to format a file size, you will now have to use {{ image.size|format_bytes(2) }} to display a human readable file size.

Considerations

Since metadata also contains image dimensions, ImageDimensions should also be refactored to grab those values from stored metadata rather than reading them from the filesystem.

Testing

Yes!

[nlemoine]

Note that I held myself back from removing more stuff 🙂

Things to consider:

• Remove path() methods in all models. Same reason as n°1 mentioned above: I don't think models need this method. If you want a relative path for something (src, link, etc.): use the relative filter or use a WP filter/plugin. Plus I don't think we need a helper to get the relative URL, there's already one in WP.
• Do you know what's the abs_url for?

  timber/src/Attachment.php

  Line 65 in 99f7a1a

  public $abs_url;

  This feels redundant with $image->src()
• I think we should remove the maybe_secure_url helper. If the URL is http, then it's a somewhere else problem that will be hidden (and not fixed and brought to the developer attention) by this fix that's not Timber in Timber's scope IMHO.
• Same goes for remove_double_slashes: we shouldn't aim at fixing every issues whose causes are elsewhere in the stack.

I could go further but I'd like your views on this first :)

[gchtr]

I’ll look at this more closely soon, but I want to say that I totally agree with the changes. There are some details that we need to consider and I think I have some insight into some of your raised issues. This is definitely the direction we should take here. And we still can, because the Attachment class and the filesize features were all introduced in 2.x.

[nlemoine]

And we still can, because the Attachment class and the filesize features were all introduced in 2.x.

That's good news! 😁

[gchtr]

I have only a couple of small comments of the nitpicking type ;)

In general, this feels like the right way to go for me.

• I like that we can remove the differentiation between size() and size_raw() and only handle bytes, while leaving the formatting to developers.
• It’s nice that you could add so many more types.
• Nice thing about accessing the metadata for attachments.

To me, the only thing missing are the relevant notes in the Upgrade Guide. Do you want me to take care of these, or do you want to add them yourself?

[coveralls]

coverage: 88.54% (-0.1%) from 88.676% when pulling a6c12ee on 2.x-refactor-file-models into f587cef on 2.x.

[nlemoine]

I removed the $file property which was useless and redundant with $file_loc, they both stored the absolute path.

There are still things I'm not fully ok with:

• image dimensions are still read from file, which could be avoided by getting image metadata for registered sizes and improve performance. But would require quite some refactoring (ImageDimensions doesn't takes some $metadata to look for dimensions)
• I noticed that ImageHelper::sideload_image() is very optimistic 😅 There are some places in that method that can go wrong and are not handled:
  • the extension parsing is not covering all possible cases
  • the download_url result should be handled as soon as it is is returned
  • there is no handling of the possible error index of wp_upload_bits()

This is something we can probably fix later.

[gchtr]

Very nice, thanks a lot @nlemoine.

I added your notes as a separate issue in #2785. It would be great if we could resolve these issues as well.