-
Notifications
You must be signed in to change notification settings - Fork 37
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
[Media] Migrate WPCom media upload to /wp/v2
endpoints
#3109
Conversation
ed5eaa7
to
6cd1ad6
Compare
} catch (e: CancellationException) { | ||
// Do nothing (the flow has been cancelled) | ||
} |
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.
I removed these parts about catching the CancellationException, I was the one who added these, but they are not useful, as the exception is automatically ignored by the Coroutine's exception handler.
} | ||
|
||
return callbackFlow { | ||
val url = WPAPI.media.getFullUrl(site) | ||
val body = WPRestUploadRequestBody(media) { media, progress -> | ||
if (!isClosedForSend) { |
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.
I also removed all statements of isClosedForSend
, it's marked as delicate API, and it's not needed, as now we use trySend
instead of the old offer
API.
params["mime_type"] = mimeType.value | ||
params["media_type"] = mimeType.value |
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.
Working on this PR identified a bug with our previous implementation, when I worked on this in the first time, I based my implementation on the WPCom one and then used the mime_type
argument, but it turns out it's not working the same between the two APIs:
- In WPCom, there is only
mime_type
, and supports both a fully qualified type, or just the base type (image/jpeg
is fully qualified, whileimage
is the base type) - In WordPress Core, we have two arguments,
media_type
which supports the base type, andmime_type
which requires the fully qualified type, so when we passed the base type here, the filtering wasn't working, which meant that in the WordPress Media Library screen we would list non-image files.
I believe the same issue is happening on iOS as well (here), I'll create an issue for them to fix it.
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.
Great job @hichamboushaba I tested it on the Woo app with XML RPC disabled/enabled and everything worked well
Part of woocommerce/woocommerce-android#12836
This PR updates the handling of the media operations (upload and fetch) to the user the
/wp/v2
endpoints instead of the legacy/v1.1
ones, this is needed to fix upload problems for sites with XMLRPC issues.Note: Since WCAndroid is the only client app using FluxC now, the change is applied without any conditions.
For testing, please check: woocommerce/woocommerce-android#12938