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 Attachment::$caption property in favor of Attachment::caption() method #2612

Merged
merged 5 commits into from Jul 27, 2022

Conversation

gchtr
Copy link
Member

@gchtr gchtr commented Jul 20, 2022

Ticket: #2610

Issue

This pull request iterates on the work by @xavivars in #2610 (thanks, @xavivars). For 2.x, we missed that the caption of an attachment is set while initializing it. See this excerpt from 1.x:

timber/lib/Image.php

Lines 215 to 217 in 8d9bff2

if ( isset($basic->post_excerpt) ) {
$this->caption = $basic->post_excerpt;
}

Solution

This pull request:

  • Adds a test to confirm the bug in Caption on thumbnails is empty (2.x) #2607.
  • Removes the Attachment::$caption property and instead adds an Attachment::caption() method. I think this is the approach we choose more and more: Using methods instead of countless properties we have to initialize.
  • Updates the Upgrade guide.
  • Adds the wp_get_attachment_caption filter, which is used in the equivalent WordPress core function wp_get_attachment_caption().

Impact

This doesn’t break backwards compatibility, even if Timber\Image::$caption is used in PHP, because Timber\Core::__get() will automatically resolve to Timber\Image::caption().

Usage Changes

None.

Considerations

Yes, caption() belongs in Timber\Attachment and not Timber\Image, because you can set captions in all kinds of attachments in WordPress, not just images.

Testing

Yes.

@gchtr gchtr added the 2.0 label Jul 20, 2022
@gchtr gchtr mentioned this pull request Jul 20, 2022
@xavivars
Copy link
Contributor

Much better approach!

@gchtr gchtr removed the request for review from jarednova July 27, 2022 14:41
@gchtr
Copy link
Member Author

gchtr commented Jul 27, 2022

@nlemoine I’d be happy about a quick review here, too 😊, thanks!

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.

I didn't know hummingbirds can’t walk! Thanks for that fun fact. Here's another one I just stumbled upon about .ch TLD: https://shkspr.mobi/blog/2022/07/dns-esoterica-why-you-cant-dig-switzerland/

Everything looks good :)

@gchtr
Copy link
Member Author

gchtr commented Jul 27, 2022

@nlemoine Yeah right? I found tests are a good place to hide fun facts, favorite movie quotes or other phrases that are stuck in your mind.

Whoa, and every day we learn something new. Didn’t know about the Chaosnet network protocol and how it’s connected to .ch a.k.a "Chaos" domains 😆.

Back on topic: Let’s merge this in, then! 🥂

@gchtr gchtr merged commit 58bc92f into 2.x Jul 27, 2022
@gchtr gchtr deleted the pr/2610 branch July 27, 2022 15:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants