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

2.x Remove unneeded Timber\Image::is_image() method #2669

Merged
merged 3 commits into from
May 20, 2023

Conversation

gchtr
Copy link
Member

@gchtr gchtr commented Nov 4, 2022

Related:

Issue

While working on #2659, I found that we probably don’t need the Timber\Image::is_image() method anymore. We now have an is_image() function in the PostFactory:

protected function is_image(WP_Post $post)

The logic is that if you use Timber::get_post(), you will only get an instance of Timber\Image, if it’s an image attachment and a Timber\Attachment instance if it’s a non-image attachment.

The Timber\Image::is_image() method is used to check whether a certain function can be used, because it only makes sense for images. But now, if we’re inside Timber\Image, we can assume that we have an image.

Solution

Remove the method and where it’s used.

Impact

This is a breaking change, hence the note in the Upgrade Guide.

Usage Changes

Instead of using Timber\Image::is_image(), we would have to use:

if ( $post instanceof Timber\Image ) {

}

Considerations

Using instanceof doesn’t work so well in Twig. Should there be an is_image() function in Timber\Post?

public function is_image()
{
	return $this instanceof Timber\Image;
}

We could also just remove the is_image() checks in Timber\Image an keep the method.

Any opinions?

Testing

Covered through existing tests.

@gchtr gchtr added the 2.0 label Nov 4, 2022
@gchtr gchtr mentioned this pull request Nov 16, 2022
1 task
@gchtr gchtr added the Ready for Review Ready for a contrib to take a look at and review/merge label Dec 30, 2022
Base automatically changed from 2.x-phpstan-level-0 to 2.x-phpstan-setup January 24, 2023 17:01
Base automatically changed from 2.x-phpstan-setup to 2.x January 24, 2023 17:27
@gchtr gchtr mentioned this pull request May 17, 2023
30 tasks
nlemoine
nlemoine previously approved these changes May 17, 2023
Copy link
Member

@nlemoine nlemoine left a comment

Choose a reason for hiding this comment

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

This looks good to me!

Levdbas
Levdbas previously approved these changes May 19, 2023
@gchtr gchtr dismissed stale reviews from Levdbas and nlemoine via fe2c2bc May 20, 2023 18:49
@gchtr
Copy link
Member Author

gchtr commented May 20, 2023

Thanks for the feedback here, everyone!

@gchtr gchtr merged commit b71a6bd into 2.x May 20, 2023
50 checks passed
@gchtr gchtr deleted the 2.x-remove-is-image-method branch May 20, 2023 19:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.0 Ready for Review Ready for a contrib to take a look at and review/merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants