Skip to content
This repository was archived by the owner on Feb 22, 2023. It is now read-only.

[url_launcher] Add a workaround for Uri encoding #3817

Merged

Conversation

stuartmorgan-g
Copy link
Contributor

@stuartmorgan-g stuartmorgan-g commented Apr 21, 2021

Uri's constructor doesn't handle query parameters correctly for
non-http(s) schemes, so the mailto example in the README is
misleading. This updates the README to show using a simple method
to work around that bug, and a warning about the need to use it.

Fixes flutter/flutter#75552
Fixes flutter/flutter#73717

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides. (Note that unlike the flutter/flutter repo, the flutter/plugins repo does use dart format. See plugin_tool format)
  • I signed the CLA.
  • The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
  • I listed at least one issue that this PR fixes in the description above.
  • I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy.
  • I updated CHANGELOG.md to add a description of the change.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test exempt.
  • All existing and new tests are passing.

`Uri`'s constructor doesn't handle query parameters correctly for
non-http(s) schemes, so the `mailto` example in the README is
misleading. This adds a new utility method to do query string
construction correctly, and updates the README to show using it and
warning about the need to use it in general.

If/when `Uri` is fixed to handle generic URI query parameters correctly,
this utility method can be deprecated.

Fixes flutter/flutter#75552
Fixes flutter/flutter#73717
@patreu22
Copy link

patreu22 commented Jun 6, 2021

Hey @stuartmorgan , I'm not sure if you are still working on that fix here, but I'm running into an issue that looks like your PR could possibly fix my issue with the encoding of linebreaks: https://stackoverflow.com/questions/67859136/flutter-url-launcher-line-break-doesnt-work-in-ios-mail-program

Maybe you find some time to finalize your PR, I think it could be very helpful!

Best regards
Patrick

@stuartmorgan-g
Copy link
Contributor Author

A decision on what to do here is pending a decision in dart-lang/sdk#43838

I may just do a README update in the short term.

@patreu22
Copy link

patreu22 commented Jun 6, 2021

I see, thx for coming back to me so quickly! Will use the suggested workaround from there and just replace the + with a %20. Still not fixing my line break issue but I'll look out for other fixes to that.

@stuartmorgan-g
Copy link
Contributor Author

Based on discussion with Hixie we're not going to put the workaround in the plugin itself, where we'll need to maintain it (or break people) even after the Dart bug is fixed, so I've updated this to just show the workaround in the README. This will avoid the issue of people following the README and getting the wrong behavior.

Copy link
Member

@ditman ditman left a comment

Choose a reason for hiding this comment

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

There's a simpler version where people can call Uri.toString().replace("+", "%20"), but this one is more "standard" (especially in JS you'd end up some code like this if you were concatenating your own URLs).

Anyway, ship it! 🚀

@stuartmorgan-g
Copy link
Contributor Author

There's a simpler version where people can call Uri.toString().replace("+", "%20")

Only in the same sense that there's a simpler version where you write 'mailto:$emailAddress?subject=$subject. I.e., it's easier to write, but is incorrect and may not actually work.

but this one is more "standard"

If by "standard" you mean "correct", then yes 🙂 The URI spec allows for + to exist unencoded in certain places, and also specifically says that "URIs that differ in the replacement of a reserved character with its corresponding percent-encoded octet are not equivalent. Percent-encoding a reserved character, or decoding a percent-encoded octet that corresponds to a reserved character, will change how the URI is interpreted by most applications."

Which means that your replace has the potential to corrupt a URI. People should not be writing that.

@stuartmorgan-g
Copy link
Contributor Author

Landing this simple PR on red to force post-submit CI to run, since we believe the last round of post-submit failures was due to an infra migration that was happening that impacted Cirrus jobs.

@stuartmorgan-g stuartmorgan-g merged commit f73fe8f into flutter:master Jun 16, 2021
@stuartmorgan-g stuartmorgan-g deleted the url-launcher-uri-workaround branch June 16, 2021 18:53
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 16, 2021
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 16, 2021
fotiDim pushed a commit to fotiDim/plugins that referenced this pull request Sep 13, 2021
`Uri`'s constructor doesn't handle query parameters correctly for
non-http(s) schemes, so the `mailto` example in the README is
misleading. This updates the README to show using a simple method
to work around that bug, and a warning about the need to use it.

Fixes flutter/flutter#75552
Fixes flutter/flutter#73717
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
4 participants