Skip to content
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

Refactor file models #2753

Merged
merged 8 commits into from
Jul 31, 2023
Merged

Refactor file models #2753

merged 8 commits into from
Jul 31, 2023

Conversation

nlemoine
Copy link
Member

@nlemoine nlemoine commented May 26, 2023 •
edited
Loading

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
Refactor file models
558b4ce 
- 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
@nlemoine nlemoine requested review from gchtr and jarednova as code owners May 26, 2023 15:29
@nlemoine nlemoine added the 2.0 label May 26, 2023
@nlemoine nlemoine self-assigned this May 26, 2023
@nlemoine nlemoine requested review from Levdbas and removed request for jarednova May 26, 2023 15:31
gchtr
gchtr reviewed May 26, 2023
View reviewed changes
@nlemoine
Copy link
Member Author

nlemoine commented May 26, 2023 •
edited
Loading

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
Copy link
Member

gchtr commented May 26, 2023

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
Copy link
Member Author

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

That's good news! 😁

@nlemoine nlemoine mentioned this pull request May 26, 2023
Closed
30 tasks
gchtr
gchtr reviewed May 30, 2023
View reviewed changes
gchtr
gchtr requested changes May 30, 2023
View reviewed changes
Copy link
Member

@gchtr gchtr left a comment

Choose a reason for hiding this comment

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

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?

gchtr and others added 3 commits July 25, 2023 08:57
@gchtr
Merge branch '2.x' into 2.x-refactor-file-models
9569c08
@nlemoine
Refactor file models
b398110
@nlemoine
Merge branch '2.x' of github.com:timber/timber into 2.x-refactor-file…
cbb9ae0 
…-models

# Conflicts:
#	src/Attachment.php
#	src/ExternalImage.php
#	src/FileSize.php
#	src/URLHelper.php
@coveralls
Copy link

coveralls commented Jul 28, 2023 •
edited
Loading

Coverage Status

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

@nlemoine
Copy link
Member Author

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:

This is something we can probably fix later.

@gchtr gchtr mentioned this pull request Jul 31, 2023
Open
@gchtr
Copy link
Member

gchtr commented Jul 31, 2023

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.

@gchtr gchtr merged commit 7b1dd06 into 2.x Jul 31, 2023
@gchtr gchtr deleted the 2.x-refactor-file-models branch July 31, 2023 12:16
@nlemoine nlemoine mentioned this pull request Oct 16, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gchtr gchtr gchtr approved these changes

@Levdbas Levdbas Awaiting requested review from Levdbas

Assignees

@nlemoine nlemoine

Labels
2.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@nlemoine @gchtr @coveralls