-
Notifications
You must be signed in to change notification settings - Fork 3.6k
REST API: Always register the media creation arguments #12833
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
Closed
adamsilverstein
wants to merge
6
commits into
WordPress:trunk
from
adamsilverstein:fix/65517-register-url-arg
+234
−45
Closed
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
afca1e9
REST API: Always register the media `url` argument.
adamsilverstein 7c9b824
Merge branch 'trunk' into fix/65517-register-url-arg
adamsilverstein fc06aed
Merge branch 'trunk' into fix/65517-register-url-arg
adamsilverstein b1a1d07
Merge branch 'trunk' into fix/65517-register-url-arg
adamsilverstein ee6d4c8
Remove the explanatory comment above the url argument registration.
adamsilverstein 63f6f1a
Register the media creation arguments unconditionally.
adamsilverstein File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if my earlier question got lost: but why do we guard
generate_sub_sizesandconvert_formatbehind the presence of client side media processing? Should we register these unconditionally, too?(Not a blocker, you've already explained part of the reasoning in the code comments. My only hesitation was really if it's a strange thing for a REST API to conditionally have some args available only some of the time)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, and thanks for asking twice - I've registered all three unconditionally now.
The thing that convinced me is that
wp_is_client_side_media_processing_enabled()comes fromis_ssl()and the host, so gating registration on it means the same site advertises different args over http vs https, or behind a proxy that doesn't setHTTPS. That's request context leaking into the schema, and since an unregistered param is still readable we were duplicating the check at runtime anyway.I did keep one guard:
generate_sub_sizesoffalseno longer relaxes the unsupported image type check increate_item_permissions_check()unless client side media processing is enabled. That check is there because the server can't process the image, so it seems worth only relaxing when the client can - otherwise we store something nothing can process. Skipping sub-sizes or conversion is fine either way, and they can still be regenerated withwp_update_image_subsizes().Does that split make sense to you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, that split makes sense to me! And thanks for digging in further there, that's exactly the nuance I was wondering about, but unsure of 😄