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

Consider removing type declaration from attachment_is_pdf() parameter. #710

Closed
1 task done
peterwilsoncc opened this issue Feb 12, 2024 · 1 comment · Fixed by #719
Closed
1 task done

Consider removing type declaration from attachment_is_pdf() parameter. #710

peterwilsoncc opened this issue Feb 12, 2024 · 1 comment · Fixed by #719
Assignees
Labels
type:enhancement New feature or request.
Milestone

Comments

@peterwilsoncc
Copy link
Contributor

Is your enhancement related to a problem? Please describe.

The Classifai\attachment_is_pdf() is currently declared as:

function attachment_is_pdf( \WP_Post $post ): bool

As the helper function is essentially a wrapper for get_post_mime_type(), it can accept either the post ID as an integer or a post object.

Within WordPress Core, most functions receiving a post as an argument accept either an ID or an integer, so it's intuitive for developers to reproduce this in plugins.

As the plugin supports PHP 7.4+, it's not possible to use a union type declaration introduced in PHP 8.0.

Designs

No response

Describe alternatives you've considered

No response

Code of Conduct

  • I agree to follow this project's Code of Conduct
@peterwilsoncc peterwilsoncc added the type:enhancement New feature or request. label Feb 12, 2024
@dkotter
Copy link
Collaborator

dkotter commented Feb 12, 2024

Yes, this sounds like the right approach to me. As part of the refactoring done in #611, I added type declaration for all of our function arguments and return values. But any function that is hooked to WordPress could have potential problems here (a few of which we've already found and fixed), as sometimes WordPress accepts multiple argument types (and isn't always clear about this in the docblock) as well as anyone else could hook into this and return an unexpected value.

I had considered not adding type declaration to any function or method hooked to WordPress but for consistency sake, I decided to add it to everything. All that said, in this instance I think we should remove that and there may be other places we might want to consider doing the same. Feel free to open a PR to at least solve the attachment_is_pdf function but can also look at other areas if you're interested

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:enhancement New feature or request.
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants