Repository navigation
Add a additional_mimes parameter to the add_image_size function - #3028
adamsilverstein wants to merge 26 commits into
Conversation
… (and adjust all uses)
felixarntz
left a comment
felixarntz
left a comment
There was a problem hiding this comment.
@adamsilverstein This looks solid mostly. I left some feedback below.
Let's also add some test coverage, but I don't think it's worth updating all the other tests unnecessarily - the parameter is optional, so it seems pointless to set it in tests where it's not relevant.
… add_image_size additional_mimes parameter
|
@felixarntz Addressed your feedback here & ready for another review whenever you have a chance |
felixarntz
left a comment
felixarntz
left a comment
There was a problem hiding this comment.
@adamsilverstein Mostly looks good to me. I left some minor feedback.
My main point is actually to focus on improving the parameter documentation. The special behavior we intend for this parameter should be clearly explained.
Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>
…additional_mime_type_support (added via add_image_size)
…ter_image_sizes_additional_mime_type_support_with_add_image_size
…tional_mime_type_support_with_add_image_size
|
@felixarntz updated based on your feedback, ready for another review |
Avoid a “doing it wrong” message - “You should provide a value for the `additional_mimes` parameter.”
adamsilverstein
changed the title
add_image_size function
adamsilverstein
changed the title
add_image_size functionadditional_mimes parameter to the add_image_size function
felixarntz
left a comment
felixarntz
left a comment
There was a problem hiding this comment.
@adamsilverstein Just few final fixes to make here.
Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>
Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>
Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>
Reconsidering this: the additional error logging might be excessive for anyone who has error logging enabled - any existing image sizes need to get updated or they will throw a warning... I feel we should give developers one release cycle (at least) before adding this notice. what do you think? So my idea would be to add this in 6.1 with a Dev Note, but no warning. The in 6.2 we can add the |
|
I removed the |
|
@adamsilverstein Now that https://core.trac.wordpress.org/changeset/54097 has been committed, I suggest we refresh this PR against latest Most of the code here is still relevant as is, we just need to change the integration point which now is no longer in |
|
@adamsilverstein Does this PR still remain relevant? If so, could you please update it and mark it as ready for review? |
|
Closing for now. Maybe later. |
Trac ticket: https://core.trac.wordpress.org/ticket/56288
Related to: https://core.trac.wordpress.org/ticket/55443
Testing Instructions
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.