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

GH-359: Fix beforeload to work with POST requests #367

Merged
merged 2 commits into from
Dec 20, 2018

Conversation

dpa99c
Copy link
Contributor

@dpa99c dpa99c commented Dec 10, 2018

Platforms affected

iOS and Android

What does this PR do?

Fixes the behaviour of beforeload to resolve the problem with POST requests outlined in #359.

The beforeload parameter has been changed from taking only a boolean (yes or not defined) to a discrete string with possible values of get, post, or yes which correspond to request types of GET, POST or GET&POST respectively. The README.md has been updated to reflect this.

Note that use of beforeload to intercept POST requests is currently not supported on Android or iOS, so if beforeload=yes is specified and a POST request is detected as the HTTP request method, beforeload behaviour will not be applied. If beforeload=post is specified, a loaderror event will be dispatched which states that POST requests are not yet supported.

Notes for Android

The shouldOverrideUrlLoading() override method has been updated to support the new method interface added in API 24 / Android 7 which receives the WebResourceRequest instead of just the String url, enabling the HTTP method of the request to be determined. The deprecated method interface has also been preserved for API <=23, but in this case the HTTP method cannot be determined so is passed as null.

Also note that due to a Chromium bug, shouldOverrideUrlLoading() is currently not called for POST requests. It's possible this may be resolved in a future Chromium version in the Android System Webview (given that this is now self-updating and independent of Android version since Android 5.0) - in prospective anticipation of this, code to handle POST requests has been added to shouldOverrideUrlLoading().

However, it seems more likely that this won't be resolved any time soon given that a Chromium dev said:

We're looking at implementing a better way to handle request interception in a future OS version. There's no way to just "fix" this, the API doesn't accommodate this usage at all. This will not be something you can use any time soon.

Therefore if we want to go ahead and use beforeload to intercept request types other than GET, it's likely we'll instead need to use the shouldInterceptRequest() method override. As with shouldOverrideUrlLoading(), there are a two variants: the new method interface added in API 21 / Android 5.0 which which receives the WebResourceRequest object and the deprecated one which receives only String url. If we want to determine the HTTP request method, we'll need to use the new implementation. This has been empirically tested and is called for POST requests so would allow the possibility to intercept, delay, modify and send the POST request and its data via beforeload.
Both shouldInterceptRequest() method interfaces have been exposed in the Android implentation for potential future use but they currently do nothing other than return the unadulterated request object.

What testing has been done on this change?

Manual testing of POST and GET requests on both platforms using a test app container:
https://github.com/dpa99c/cordova-plugin-inappbrowser-test

…beforeload.

For now only GET is supported with beforeload, so document this.
@keenan35i
Copy link

Any updates on getting this PR merged? @janpio

@janpio
Copy link
Member

janpio commented Dec 17, 2018

Feel free to test it and describe how and what you did, then leave a pull request review in the "Files changed" tab reflecting your results.

(But please don't @ mention people here that were not active in a issue or pull request before - if everyone did that I would get even less work done each day...)

@keenan35i
Copy link

You where active in the issue that this pr addresses... #359. In which i tested and left a comment that its working as correctly as architected in the issue thread...

Copy link

@keenan35i keenan35i left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works correctly as designed and described in issue #359, POST is ignored when you set beforeload=get, but GET requests trigger the event correctly.

@dpa99c
Copy link
Contributor Author

dpa99c commented Dec 20, 2018

@janpio Anything more needed here or can this be merged?
I'm keen to get it merged to master before any merge conflicts arise...

@janpio janpio merged commit 632a395 into apache:master Dec 20, 2018
@janpio
Copy link
Member

janpio commented Dec 20, 2018

Nah, all good.

@dpa99c
Copy link
Contributor Author

dpa99c commented Dec 20, 2018

😎 nice one

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants