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

Conversation

@collinjackson
Copy link
Contributor

@collinjackson collinjackson commented Nov 27, 2019

This is an attempt to address what I understand to be the underlying motivation of @Hixie's comment on #2230.

Problem

The isMock API is currently in the public docs (despite being @visibleForTesting) and we're not thrilled about it.

The @visibleForTesting annotation on isMock results in a warning that can be easily ignored by the plugin consumer. This could create a maintenance burden if platform implementers create packages that use the isMock backdoor to implement rather than extend the platform interface.

Furthermore, it is currently possible to bypass isMock trivially with noSuchMethod.

Solution

This PR proposes a base class PlatformInterface that plugins can extend. It provides a static validation method that is used to ensure that subclasses use extends rather than implements. The verification is performed using an identical comparison on a private Object token.

I believe that this approach makes it impossible to bypass the validation check using noSuchMethod.

The base classes should be extracted into a common package, see flutter/flutter#43368

To avoid introducing new API surface area that we later have to remove we should probably move the PlatformInterface class into either the Flutter SDK or a package.

/cc @amirh @goderbauer

@collinjackson collinjackson changed the title Fix to prevent isMock from being used in release builds [url_launcher] Prevent isMock from being used in release builds Nov 27, 2019
@collinjackson collinjackson requested a review from amirh November 27, 2019 20:07
@collinjackson collinjackson changed the title [url_launcher] Prevent isMock from being used in release builds [url_launcher] Prevent isMock from being used in release builds Nov 27, 2019
Copy link
Contributor

@amirh amirh left a comment

Choose a reason for hiding this comment

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

LGTM

Thanks!

@collinjackson
Copy link
Contributor Author

@Hixie I've updated this PR based on our discussion, PTAL

@collinjackson collinjackson changed the title [url_launcher] Prevent isMock from being used in release builds [url_launcher] Alternative to isMock Nov 28, 2019
@collinjackson
Copy link
Contributor Author

collinjackson commented Nov 28, 2019

After discussing with Amir I think we'll want to copy these new base classes into a new first-party package (verify_extends? platform_interface?) and also add the base classes to the Flutter SDK. We'll need to wait until after next Flutter stable release to switch over to using the Flutter SDK versions of the classes in federated plugins and then we can deprecate the first-party package.

url_launcher_platform_interface could export the MockXXX base class, with a comment that it is likely to be removed from the exported API and replaced with the Flutter SDK class in the future; this would not be considered a semantically breaking change to the platform interface since it is a test-only dependency and unlikely to break anyone except for the federated plugin owner.

I'm closing this PR since it's not able to land in the current state, @amirh can you take it from here?

@amirh amirh mentioned this pull request Dec 4, 2019
13 tasks
amirh added a commit that referenced this pull request Dec 10, 2019
Adds a package with a common base class for platform interface.

The PlatformInterface class provides common functionality for enforcing that platform implementations extend the platform interface with extends rather than implement it with implements.

This is based on the existing code from the url_laucher_platform_interface, and the improvements proposed in #2326
FlutterSu pushed a commit to FlutterSu/flutter-plugins that referenced this pull request Nov 20, 2020
Adds a package with a common base class for platform interface.

The PlatformInterface class provides common functionality for enforcing that platform implementations extend the platform interface with extends rather than implement it with implements.

This is based on the existing code from the url_laucher_platform_interface, and the improvements proposed in flutter#2326
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants