You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cordova.apache.org by rakatyal <gi...@git.apache.org> on 2016/03/25 01:20:17 UTC
[GitHub] cordova-plugin-file-transfer pull request: CB-10771: Fixing failur...
GitHub user rakatyal opened a pull request:
https://github.com/apache/cordova-plugin-file-transfer/pull/133
CB-10771: Fixing failure when empty string passed as a value for opti…
…on parameter in upload function
CreateUploadAsync fails when passed an empty string as a value for SetText function. It also fails when we skip specifying a value for it. So when a user passes an empty string, pass " " instead.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/MSOpenTech/cordova-plugin-file-transfer raghav-CB-10771
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cordova-plugin-file-transfer/pull/133.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #133
----
commit 45c98415c4e56ac7cc014c23b7453eddacc332ef
Author: Raghav Katyal <ra...@microsoft.com>
Date: 2016-03-25T00:17:15Z
CB-10771: Fixing failure when empty string passed as a value for option parameter in upload function
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-file-transfer pull request: CB-10771: Fixing failur...
Posted by rakatyal <gi...@git.apache.org>.
Github user rakatyal commented on the pull request:
https://github.com/apache/cordova-plugin-file-transfer/pull/133#issuecomment-201594752
Updated to ignoring the parameter all together.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-file-transfer pull request: CB-10771: Fixing failur...
Posted by rakatyal <gi...@git.apache.org>.
Github user rakatyal closed the pull request at:
https://github.com/apache/cordova-plugin-file-transfer/pull/133
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-file-transfer pull request: CB-10771: Fixing failur...
Posted by rakatyal <gi...@git.apache.org>.
Github user rakatyal commented on the pull request:
https://github.com/apache/cordova-plugin-file-transfer/pull/133#issuecomment-202575695
I tested it on Android and apparently we do accept non string values and I don't see any reason why we should not. So your suggestion sounds fair to me.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-file-transfer pull request: CB-10771: Fixing failur...
Posted by jasongin <gi...@git.apache.org>.
Github user jasongin commented on the pull request:
https://github.com/apache/cordova-plugin-file-transfer/pull/133#issuecomment-202597643
The update to check for an empty toString() result (as discussed) looks good.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-file-transfer pull request: CB-10771: Fixing failur...
Posted by rakatyal <gi...@git.apache.org>.
Github user rakatyal commented on the pull request:
https://github.com/apache/cordova-plugin-file-transfer/pull/133#issuecomment-201082490
@riknoll, @jasongin: Please review.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-file-transfer pull request: CB-10771: Fixing failur...
Posted by rakatyal <gi...@git.apache.org>.
Github user rakatyal commented on the pull request:
https://github.com/apache/cordova-plugin-file-transfer/pull/133#issuecomment-201103598
I believe so. As mentioned, passing empty string as a value for SetText function. It also fails when we skip specifying a value for it. One way to avoid this is leaving out this chunk, but then we lose sending the key for the option parameter. I can document it as a quirk.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-file-transfer pull request: CB-10771: Fixing failur...
Posted by riknoll <gi...@git.apache.org>.
Github user riknoll commented on the pull request:
https://github.com/apache/cordova-plugin-file-transfer/pull/133#issuecomment-201085977
Is this the only workaround? If so, this quirk should definitely be documented.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-file-transfer pull request: CB-10771: Fixing failur...
Posted by rakatyal <gi...@git.apache.org>.
Github user rakatyal commented on the pull request:
https://github.com/apache/cordova-plugin-file-transfer/pull/133#issuecomment-202599984
Merged [here](https://github.com/apache/cordova-plugin-file-transfer/commit/584754869befd2d899f8e20270ba086ffe525625).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-file-transfer pull request: CB-10771: Fixing failur...
Posted by rakatyal <gi...@git.apache.org>.
Github user rakatyal commented on a diff in the pull request:
https://github.com/apache/cordova-plugin-file-transfer/pull/133#discussion_r57475146
--- Diff: src/windows/FileTransferProxy.js ---
@@ -359,7 +359,13 @@ exec(win, fail, 'FileTransfer', 'upload',
if (params.hasOwnProperty(key)) {
var contentPart = new Windows.Networking.BackgroundTransfer.BackgroundTransferContentPart();
contentPart.setHeader("Content-Disposition", "form-data; name=\"" + key + "\"");
- contentPart.setText(params[key]);
+ // CreateUploadSync fails when supplied with empty value for setText function. Pass " " instead when not specified.
+ if (params[key]) {
+ contentPart.setText(params[key]);
+ }
+ else {
+ contentPart.setText(" ");
--- End diff --
That's an option. But we would lose sending the 'key' as well in that case. I thought this was the lesser of the two evils.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-file-transfer pull request: CB-10771: Fixing failur...
Posted by paulhickman-a365 <gi...@git.apache.org>.
Github user paulhickman-a365 commented on the pull request:
https://github.com/apache/cordova-plugin-file-transfer/pull/133#issuecomment-202054865
Won't using "params[key]" as the test make it also ignore non-string values such as 0 or boolean false? Testing for undefined, null and "" explicitly would be better.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-file-transfer pull request: CB-10771: Fixing failur...
Posted by rakatyal <gi...@git.apache.org>.
Github user rakatyal commented on the pull request:
https://github.com/apache/cordova-plugin-file-transfer/pull/133#issuecomment-202545461
Yes it would but the options parameter is supposed to have only string key-value pairs as mentioned in the README.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-file-transfer pull request: CB-10771: Fixing failur...
Posted by jasongin <gi...@git.apache.org>.
Github user jasongin commented on a diff in the pull request:
https://github.com/apache/cordova-plugin-file-transfer/pull/133#discussion_r57474870
--- Diff: src/windows/FileTransferProxy.js ---
@@ -359,7 +359,13 @@ exec(win, fail, 'FileTransfer', 'upload',
if (params.hasOwnProperty(key)) {
var contentPart = new Windows.Networking.BackgroundTransfer.BackgroundTransferContentPart();
contentPart.setHeader("Content-Disposition", "form-data; name=\"" + key + "\"");
- contentPart.setText(params[key]);
+ // CreateUploadSync fails when supplied with empty value for setText function. Pass " " instead when not specified.
+ if (params[key]) {
+ contentPart.setText(params[key]);
+ }
+ else {
+ contentPart.setText(" ");
--- End diff --
Should a null/empty parameter just be omitted from the transfer instead?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org