[path_provider] Removed pre-screening for platform

[technolion]

Description

I removed the checks for platforms when calling to retrieve a path.
As @stuartmorgan suggested, a plugin's platform dependent implementation should check whether the calls returns a valid result or not. So the pre-screening in the interface would be a bad practice.

Related Issues

flutter/flutter#41715

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

• I read the Contributor Guide and followed the process outlined there for submitting PRs.
• My PR includes unit or integration tests for all changed/updated/fixed behaviors (See Contributor Guide).
• All existing and new tests are passing.
• I updated/added relevant documentation (doc comments with ///).
• The analyzer (flutter analyze) does not report any problems on my PR.
• I read and followed the Flutter Style Guide.
• The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
• 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 signed the CLA.
• I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

• Yes, this is a breaking change (please indicate a breaking change in CHANGELOG.md and increment major revision).
• No, this is not a breaking change.

[technolion]

This PR is no longer needed, since path_provider 1.6.16 perfectly works on Windows including getDownloadPath()

[stuartmorgan-g]

The fact that it works now is just a side-effect of the implementation details of the Windows and Linux path_provider implementations; the code in question is still wrong.

[technolion]

@stuartmorgan Do you want me to reopen this PR? I think my understanding of the platform handling here is somewhat limited. When is the code at
https://github.com/flutter/plugins/blob/master/packages/path_provider/path_provider_platform_interface/lib/src/method_channel_path_provider.dart
used anyhow? Just on platforms that have no specific implementation?

[stuartmorgan-g]

When is the code at
https://github.com/flutter/plugins/blob/master/packages/path_provider/path_provider_platform_interface/lib/src/method_channel_path_provider.dart
used anyhow?

On any platform whose implementation is based on the existing platform channels.

[technolion]

OK, reopened. Then all methods of MethodChannelPathProvider should be stripped of the platform screening.

[stuartmorgan-g]

Then all methods of MethodChannelPathProvider should be stripped of the platform screening.

Agreed; do you want to update the PR to do that? I can review and land it promptly if so. (It's arguably a breaking change, since the error type is changing, so doing it now while we're in nullsafety prerelease mode would be ideal.)

[technolion]

Then all methods of MethodChannelPathProvider should be stripped of the platform screening.

Agreed; do you want to update the PR to do that? I can review and land it promptly if so. (It's arguably a breaking change, since the error type is changing, so doing it now while we're in nullsafety prerelease mode would be ideal.)

OK, will do. I don't know why the test were failing the last time, since this was a very trivial change.

[technolion]

@stuartmorgan I am done. Please review.
About the two failing checks I am clueless. They seem to be unrelated to my changes.

[stuartmorgan-g]

I'll look into the CI failures if they repeat on the next iteration of the PR; it's not obvious to me what failed from an initial glance.

[technolion]

by the way, when building, these files got changed, too. Do you want me to commit them, too?

# Changes not staged for commit:
#       modified:   packages/path_provider/path_provider/example/ios/Flutter/Debug.xcconfig
#       modified:   packages/path_provider/path_provider/example/ios/Flutter/Release.xcconfig
#       modified:   packages/path_provider/path_provider/example/macos/Flutter/Flutter-Debug.xcconfig
#       modified:   packages/path_provider/path_provider/example/macos/Flutter/Flutter-Release.xcconfig
#       modified:   packages/path_provider/path_provider/example/windows/flutter/generated_plugin_registrant.cc

[technolion]

I don't know why the test Cirrus CI / build-apks+java-test+firebase-test-lab CHANNEL:master PLUGIN_SHARDING:--shardIndex 0 --shardCount 4 failed. It looks like the test is not applicable for this plugin. Can you please check @stuartmorgan ?

[stuartmorgan-g]

Having spent more time looking at this API while doing the null-safety migration, and thinking about various error/exception cases, I'm reconsidering the approach we should take here. The current filtering is problematic because it opts platforms in, which breaks extensibility.

Just removing them, however, means that instead of a clear error message when calling an unsupported method, developers will get the "no implementation found" exception. I had been thinking about that as essentially equivalent, but given how many issues come in where people see that exception and don't understand why I think that change would actually cause a lot more confusion.

I don't have a great solution for this problem, unfortunately. Probably the best option, structurally, would be for each platform to have their own implementation override that throws user-friendly errors, and forwards to the method channel implementation for everything supported. However, there's not a great way to do that without dartPluginClass support for mobile yet. (/cc @blasten: a use case for mobile support 🙂 ). Since we can't do that, I think the best option here is to invert the logic, so that platforms that we know don't support specific methods throw errors, but everything else falls through. It still has platform logic in the interface, but it has the substantial improvement of not shutting out unendorsed implementations.

Sorry for the churn on this. Let me know if you are interested in updating to that version; if not I totally understand, and can start a new PR instead.

[technolion]

Sorry for the churn on this. Let me know if you are interested in updating to that version; if not I totally understand, and can start a new PR instead.

Dom't worry, I learned a lot doing this exercise ;-). But for future development it's probably much more efficient, if you do it. Closing this one.