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

don't guard against non-"attachment" post-type #82

Closed
wants to merge 2 commits into from

Conversation

drzraf
Copy link
Contributor

@drzraf drzraf commented Aug 1, 2024

This code-path can't be override (contrary to checks done by the Timmy\Image constructor. Be it for usage with custom-post-type or subclassing attachment, or any other plugin edge-case, this check would be better left to a place it can be skipped/made optional.

drzraf and others added 2 commits August 1, 2024 17:26
This code-path can't be override (contrary to checks done by the Timmy\Image constructor.
Be it for usage with custom-post-type or subclassing attachment, or any other plugin edge-case, this check would be better left to a place it can be skipped/made optional.
@gchtr
Copy link
Member

gchtr commented Aug 27, 2024

@drzraf I think I see what your concern is.

But I think we still need some kind of check whether it’s a post. And I didn’t want to introduce a full factory pattern yet. So my question is. Would a check for a post work, as we do it in Timber?

$wp_post = \get_post($attachment);

if (!$wp_post) {
    return null;
}

@drzraf
Copy link
Contributor Author

drzraf commented Aug 28, 2024

For my use-case, yes, it would be fine: Ensuring the object is present in the database is one thing.

My problem is about assuming that the object to be used is going to have post_type == "attachment" (instead of, eg, a duplicated-attachment, attachment-revision, remote-attachment-mirrored, ... or whatever strange things plugins could do using custom post types)

@gchtr
Copy link
Member

gchtr commented Aug 30, 2024

Alright, I’m totally fine with not checking for the attachment post type explicitly. But there’s some functionality in Timmy that relies on an object being a WordPress post. So I added that check in #85.

This is now released in https://github.com/mindkomm/timmy/releases/tag/v2.1.0.

@gchtr gchtr closed this Aug 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants