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.

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

Missing image constructor with file path in 2.x #2627

Closed
xavivars opened this issue Aug 7, 2022 · 10 comments
Closed

Missing image constructor with file path in 2.x #2627

xavivars opened this issue Aug 7, 2022 · 10 comments

Comments

@xavivars
Copy link
Contributor

xavivars commented Aug 7, 2022

Expected behavior

Can create an image from a URL or local path

$myImage = new Image( get_stylesheet_directory() . '/img/compass.png' );

Actual behavior

Constructor for images based on path no longer exists

Steps to reproduce behavior

What version of WordPress, PHP and Timber are you using?

PHP 7.4, WordPress 6.0, Timber 2.x beta 1

More information

Images used to have an init method that was called from the image constructor.

https://github.com/timber/timber/blob/master/lib/Image.php#L265

All the underlying code is still there (now in Attachment class), but it's protected and can't be directly used:

https://github.com/timber/timber/blob/2.x/src/Attachment.php#L129

@gchtr gchtr added the 2.0 label Aug 7, 2022
@aj-adl
Copy link
Contributor

aj-adl commented Sep 2, 2022

Direct instantiation of Timber\Post and it's child classes is deprecated in 2.0, so new Image(); would not be expected to work (at all) for this reason.

The functionality you're after has been preserved though, after reviewing the docs I think this could be made clearer, but you're after Timber::get_attachment_by( $pathToImage ) - see https://github.com/timber/timber/blob/2.x/src/Timber.php#L600

This is also available in twig templates, so where you used to do {% set img = Image( 'path/to/image.jpg' ) %} you would now do {% set img = get_attachment_by( 'path/to/image.jpg' ) %}.

Highly recommend reviewing the upgrade guide a couple times - https://timber.github.io/docs/v2/upgrade-guides/2.0/#timber\image-and-timber\attachment - It's taken a few read-throughs for the implications to really sink in for me personally!

Hope this helps.

@xavivars
Copy link
Contributor Author

xavivars commented Sep 2, 2022

The behavior is not the same. That only works for attachments: images that have been uploaded to WordPress via the media manager.

But what I'm expecting is to be able to use Timber Image class for arbitrary images, maybe part of the theme.
When they are not attachments, this line returns null

https://github.com/timber/timber/blob/2.x/src/Timber.php#L615

But in timber 1, you could create Image objects based on arbitrary paths, and then use image manipulation functions with them.

@aj-adl
Copy link
Contributor

aj-adl commented Sep 2, 2022

Right, I can see this, and appreciate the nuance there - but I believe that the intention behind classes like Attachment is exactly that, to mirror a WP_Post backed object. Looking at the implementation in 1.x, Timber\Image created via URL or File Paths are very much second class citizens, and almost none of the methods on the actual Timber\Image class work or return anything useful.

If your main concern is resizing or other operations like this, are you aware they do not need to receive a Timber\Image class?

If you have the abspath OR an external URL, you can pass this into the resize filter in twig just fine, and if you're trying to do it in PHP then you'd be looking at calling Timber\ImageHelper::resize( $path_or_url, $w, $h ); instead, with the same result.

Other operations in the Image Helper class can be called directly or accessed via the twig filters or their PHP static functions without a Timber\Image being provided.

While I understand that it means some changes to existing code, 2.0 is definitely going to need that across the board - the whole API is being tightened so it can be better maintained, and this involves the team rolling back a lot of "grey areas" where the use over time has made it really hard to make meaningful changes to Timber.

Using a class like Timber\Attachment which is 99% of the time a WP_Post backed object doesn't make the most sense to me, as 90% of the methods don't work when instantiated with a file or url.

Don't take my word as gospel - definitely up to the core team to decide what's right here, but based on what I've laid out above, I think it makes sense for this not to be supported anymore.

@xavivars
Copy link
Contributor Author

xavivars commented Sep 2, 2022

I completely agree with you on a few things there.

But a few points

  • I was not using Timber\Attachment before. That's a new class introduced in 2.0. In 1.x, that class didn't even exist.
  • It feels weird that you can provide a URL using TimberImage constructor class, and leaves you with an "improperly formatted" object. Shouldn't it throw an exception (or have the constructor private so it can only be accessed from the factory, if that's the desire)?
  • Code is still here https://github.com/timber/timber/blob/2.x/src/Attachment.php#L129, but it's inaccessible. I would assume if the removal of this feature is a conscious one, then I wouldn't be there anymore, and it would be documented as part of the deprecated functionalities on the migration docs.

That's why I think this is a clear gap in 2.0. Not sure if the gap is functionality is missing (that was my guess), or deprecation documentation is missing (that's another option), but "everything is as expected" is clearly not what I see here 😅

@aj-adl
Copy link
Contributor

aj-adl commented Sep 2, 2022

Absolutely - totally agree with you that if this is how it's going to be then the docs and deprecation notices need to be improved.

In 1.x Timber\Image still extended Timber\Post, so in a similar way to the new Attachment class, the 'intent' for lack of a better word was for it to be backed by a Post in the majority of cases.

When dealing with Image manipulation, the main benefit I can see is that you don't need to treat external / theme provided images differently in your templates, and they can be agnostic to the "source" of the original image, even though in these cases really all the Image is doing is providing it's src method.

There's a few potential ways forward here, might need some input from @jarednova / @gchtr - allow this in Attachment, ban it and do better docs / deprecations, or some middle ground where we make a new helper object to represent non-wp post backed images that is compatible with uses of Timber\Image in templates.

@gchtr
Copy link
Member

gchtr commented Sep 12, 2022

Thanks @xavivars and @aj-adl for flagging this and thinking it through. This is really some shortcoming of the current image functionality. This particular functionality is still missing in 2.x. That’s why it also isn’t documented yet.

We thought about the concept of images from external URLs when the introduced the Attachment class. But we postponed it as a later task. This was even before we started working on the factory classes. It might have slipped through as a feature that we forgot we still should implement.

The relevant discussion can be found here:

We thought of a new ExternalImage or ImageAsset class (how should be best name this?) that doesn’t extend the Attachment class, because as you both have correctly assessed, images from URLs are not really attachments. It should be a class that provides the functionality you can actually use on these images, but no useless methods or properties. Using the object factories, it should be easy to instantiate such an object.

Basically what @aj-adl proposes:

or some middle ground where we make a new helper object to represent non-wp post backed images that is compatible with uses of Timber\Image in templates.

I didn’t look into what exactly is needed to make this work. If any of you feels like they could implement this, we would appreciate the help.

@xavivars
Copy link
Contributor Author

I'll take this. However, I'm wondering if I should add another method (like get_external_image) in the Timber class, to act as a factory method (like the existing ones, get_post, get_attachment, or get_image), or if it would be better to overload get_image...

I'll take the first approach for now, but happy to change it later.

@xavivars
Copy link
Contributor Author

A possible solution for this has been implemented in #2639

@gchtr
Copy link
Member

gchtr commented Feb 3, 2023

This was fixed in #2639.

Thanks for leading the way and taking the time to help with this, @xavivars 🥳.

@gchtr gchtr closed this as completed Feb 3, 2023
@xavivars
Copy link
Contributor Author

xavivars commented Feb 3, 2023

Thank you! You did most of the work ☺️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants