[Gutenberg] Image Size networking (native) by etoledom · Pull Request #12714 · wordpress-mobile/WordPress-iOS

etoledom · 2019-10-17T07:26:59Z

Alternative to #12706

As a proof of concept, this PR implements the minimum necessary to make Image Size works.
In the future we might need to create a more generic api call with a dynamic request method (given from gutenberg). We also might need to handle other api version Paths.

Related PRs:
gutenberg-mobile: wordpress-mobile/gutenberg-mobile#1455

To test:

Run metro server on [try] Image Size with networking on native client gutenberg-mobile#1455
Add a image block to gutenberg.
Select an image from the WordPress media Library.
Select settings and choose a different image setting.
Change to HTML mode.
Check that the image URL has changes according to the selected image size.

Update release notes:

I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

To allow networking.

peril-wordpress-mobile · 2019-11-07T08:00:17Z

You can trigger an installable build for these changes by visiting CircleCI here.

Now it properly checks if the url is the one of an image

…e-size-networking-native

This is only required if the network call is started from the JS side.

koke · 2019-11-08T11:34:01Z

WordPress/Classes/ViewRelated/Gutenberg/GutenbergNetworking.swift

+        self.blog = blog
+    }
+
+    func request(response: @escaping Response) {


I found quite confusing the response/Response naming for a callback, when the argument to that callback will be the actual response. Maybe we can use the more generic completion?

sounds good! 👍

koke · 2019-11-08T11:43:19Z

WordPress/Classes/ViewRelated/Gutenberg/GutenbergNetworking.swift

+    }
+
+    private var selfHostedPath: String {
+        let removedEditContext = path.replacingOccurrences(of: "context=edit", with: "context=view")


Switching the context to view happens to work fine for this specific use case, but it likely won't for others. I don't like the idea of implementing it generically and pretending that apiFetch can now work for anything else, as it might lead to subtle undetected bugs.

Since I understand this is a temporary solution to an immediate problem, I think we should enforce that this system only works with a known set of API paths. So for any request other than getting media sizes, I would probably return a Not implemented error, or ignore the request and just log a warning.

I think adding this kind of guard makes sense. Assuming we always expect the platforms to be in sync, it would be nice to have that guard on the JS-side so that we don't have to replicate it on both platforms.

I thought on having some kind of white list of paths that we accept to avoid surprises.
This implementation also only do GET requests so far, so there's another reason for this. 👍
I agree that is probably better to do this on JS side, then probably this context change should also go on the JS side?

I agree that is probably better to do this on JS side, then probably this context change should also go on the JS side?

👍 The more path manipulation we move to the JS side (i.e., the siteId if that's available in JS), the better imo. I was planning to investigate that once I got the Android implementation wrapped up.

The SiteID is a WPCom thing, probably we don't want it on gutenberg repo.

Good point @etoledom

koke · 2019-11-08T11:47:29Z

WordPress/Classes/ViewRelated/Gutenberg/GutenbergNetworking.swift

@@ -0,0 +1,62 @@
+import Alamofire
+
+struct GutenbergNetworkRequest {


This is just a bit of a rant, but specially after doing JS for a while, I find interesting how reluctant we all are to use top level functions on Swift. There's no point in rewriting this, but it's something that keeps catching my attention 😁

I like the Struct to pack some properties inside it, and have methods with less arguments.
At first I did static functions (that would work as well as top level functions), but I didn't like having all the internal helper functions needing the whole set of arguments.

…ry/gutenberg-image-size-networking-native

mchowning · 2019-11-11T21:37:34Z

I'm currently seeing an error when try to update the size of an image for a self-hosted site. I'll try to investigate this some tomorrow. I am seeing this show up in the logs, although I don't know whether it is related yet:

2019-11-11 16:35:36.819134-0500 WordPress[15184:4171347] CredStore - performQuery - Error copying matching creds.  Error=-50, query={
    "m_Limit" = "m_LimitAll";
    ptcl = htps;
    srvr = "damp-ducks.jurassic.ninja";
    sync = syna;
}

mchowning · 2019-11-11T21:52:23Z

Hey @etoledom ! I merged gutenberg/release-1.17.0 into this and updated the PR to target that release branch.

…name

Helps to avoid getting unexpected responses and hard to track down bugs: wordpress-mobile/WordPress-iOS#12714 (comment)

…ry/gutenberg-image-size-networking-native

etoledom · 2019-11-18T09:35:22Z

There are a few known issues: wordpress-mobile/gutenberg-mobile#1455 (comment) - I'm hoping those are not blockers for this PR (I'm currently working on those).

Keeping that in mind, I believe this is ready for another 👀
(cc @SergioEstevao @koke )

loremattei · 2019-11-18T10:04:30Z

Hey there! I've moved this to 13.8 since 13.7 has been cut. If you want this to make it to 13.7, please feel free to ping me.

SergioEstevao

Working fine!

Tested it on WP.com regular, WP.com Atomic and self-hosted sites.

…e-size-networking-native

etoledom added the Gutenberg compatibility label Oct 17, 2019

etoledom added this to the 13.6 milestone Oct 17, 2019

etoledom self-assigned this Oct 17, 2019

etoledom mentioned this pull request Oct 17, 2019

[try] Image Size with networking on native client wordpress-mobile/gutenberg-mobile#1455

Merged

1 task

loremattei modified the milestones: 13.6 ❄️, 13.7 Nov 4, 2019

etoledom added 4 commits November 7, 2019 08:25

Gutenberg: Send httpHeaders and site id to Gutenberg

570bf35

To allow networking.

Avoid RN from loading any non-file URL with ImageLoader

deac0fa

Gutenberg: Handle fetch requests on native side

3854736

Update gutenberg ref

cf662f2

etoledom force-pushed the try/gutenberg-image-size-networking-native branch from 10e3238 to cf662f2 Compare November 7, 2019 08:00

etoledom added 4 commits November 7, 2019 09:48

[Gutenberg] Basic non-auth networking on Self-Hosted sites

a4c6342

[Gutenberg] Refactor network request

e28569f

Revert WPKit version change

b0d6b52

[Gutenberg] Fix GutenbergImageLoader canLoadImageURL method.

c6e7266

Now it properly checks if the url is the one of an image

etoledom marked this pull request as ready for review November 8, 2019 08:53

etoledom requested review from SergioEstevao and koke November 8, 2019 08:53

etoledom added 2 commits November 8, 2019 09:56

Merge remote-tracking branch 'origin/develop' into try/gutenberg-imag…

d74f42d

…e-size-networking-native

Update gutenberg ref

f43f766

etoledom mentioned this pull request Nov 8, 2019

[RNMobile] Implement getMedia on Image block. WordPress/gutenberg#17979

Merged

etoledom added 2 commits November 8, 2019 12:06

[Gutenberg] Revert change on ImageLoader

e039ccf

This is only required if the network call is started from the JS side.

Remove Alamofire import not needed on GutenbergViewController

12b8cef

koke reviewed Nov 8, 2019

View reviewed changes

mchowning added 2 commits November 11, 2019 15:57

Merge remote-tracking branch 'origin/gutenberg/release-1.17.0' into t…

6a47ff4

…ry/gutenberg-image-size-networking-native

Update gutenberg ref

a2a63ab

mchowning changed the base branch from develop to gutenberg/release-1.17.0 November 11, 2019 21:51

etoledom added 2 commits November 15, 2019 15:12

[Gutenberg] update gutenberg networking completion handler parameter …

3878c82

…name

Update gutenberg ref

a6fd855

mchowning added a commit to wordpress-mobile/gutenberg-mobile that referenced this pull request Nov 15, 2019

Have fetch handler immediately reject unsupported paths

a4aaac9

Helps to avoid getting unexpected responses and hard to track down bugs: wordpress-mobile/WordPress-iOS#12714 (comment)

Merge remote-tracking branch 'origin/gutenberg/release-1.17.0' into t…

67065a0

…ry/gutenberg-image-size-networking-native

mchowning force-pushed the try/gutenberg-image-size-networking-native branch from ee6b8c8 to 67065a0 Compare November 15, 2019 15:47

Update gutenberg ref

28e6162

etoledom added Gutenberg Editing and display of Gutenberg blocks. and removed Gutenberg compatibility labels Nov 18, 2019

etoledom requested a review from koke November 18, 2019 09:35

loremattei modified the milestones: 13.7 ❄️, 13.8 Nov 18, 2019

SergioEstevao approved these changes Nov 18, 2019

View reviewed changes

This was referenced Nov 19, 2019

[try] Image Size networking wordpress-mobile/gutenberg-mobile#1453

Closed

[Gutenberg] Image Size networking #12706

Closed

Merge remote-tracking branch 'origin/develop' into try/gutenberg-imag…

819b0ba

…e-size-networking-native

etoledom changed the base branch from gutenberg/release-1.17.0 to develop November 19, 2019 07:03

etoledom added 2 commits November 19, 2019 08:30

Update gutenberg ref

b6ef672

Update gutenberg ref

057df85

etoledom merged commit e0702a4 into develop Nov 20, 2019

etoledom deleted the try/gutenberg-image-size-networking-native branch November 20, 2019 09:40

koke mentioned this pull request Nov 28, 2019

[PoC] WP-API and Cookie Authentication wordpress-mobile/WordPressKit-iOS#202

Merged

1 task

		@@ -0,0 +1,62 @@
		import Alamofire

		struct GutenbergNetworkRequest {

Conversation

etoledom commented Oct 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

peril-wordpress-mobile bot commented Nov 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

koke Nov 8, 2019

Choose a reason for hiding this comment

Uh oh!

etoledom Nov 9, 2019

Choose a reason for hiding this comment

Uh oh!

koke Nov 8, 2019

Choose a reason for hiding this comment

Uh oh!

mchowning Nov 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

etoledom Nov 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mchowning Nov 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

etoledom Nov 10, 2019

Choose a reason for hiding this comment

Uh oh!

mchowning Nov 10, 2019

Choose a reason for hiding this comment

Uh oh!

koke Nov 8, 2019

Choose a reason for hiding this comment

Uh oh!

etoledom Nov 9, 2019

Choose a reason for hiding this comment

Uh oh!

mchowning commented Nov 11, 2019

Uh oh!

mchowning commented Nov 11, 2019

Uh oh!

etoledom commented Nov 18, 2019

Uh oh!

loremattei commented Nov 18, 2019

Uh oh!

SergioEstevao left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

etoledom commented Oct 17, 2019 •

edited

Loading

peril-wordpress-mobile bot commented Nov 7, 2019 •

edited

Loading

mchowning Nov 8, 2019 •

edited

Loading

etoledom Nov 9, 2019 •

edited

Loading

mchowning Nov 9, 2019 •

edited

Loading