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

Setup on attachment #2610

Closed
wants to merge 1 commit into from
Closed

Conversation

xavivars
Copy link
Contributor

@xavivars xavivars commented Jul 19, 2022

Ticket:
Fixes: #2607

Issue

Caption on images is empty, and not intitialized.

Solution

Implements Attachment::build method that will be used in PostFactory so caption is defined.

Impact

It's really a bugfix.

Considerations

Not 100% sure this is the way that it's meant to be implemented. An alternative (that would break backwards compatibility) would be to just remove the field, and document it so post_excerpt is used instead.

Testing

I didn't add any test.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 87.997% when pulling 034f247 on xavivars:2.x-initialize-caption into 1500e48 on timber:2.x.

@gchtr
Copy link
Member

gchtr commented Jul 20, 2022

Hey @xavivars, thanks for reporting the issue and making this pull request. This is really something we missed when working on the factories as well as splitting Timber\Image into the two separate Timber\Image and Timber\Attachment classes. Nice catch!

Not 100% sure this is the way that it's meant to be implemented. An alternative (that would break backwards compatibility) would be to just remove the field, and document it so post_excerpt is used instead.

I would keep {{ image.caption }} and not document that it’s saved in the post excerpt. I even think we should use a caption() method instead of the property and added a pull request that iterates on your pull request in #2612.

@xavivars xavivars closed this Jul 20, 2022
@xavivars xavivars deleted the 2.x-initialize-caption branch July 20, 2022 20:42
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