Unverified Commit dc6f94a8 authored by August's avatar August Committed by GitHub

refactor: Remove `throwOnPluginPubspecError` flag for plugin validation (#144214)

Part of #137040 and #80374

The flag `throwOnPluginPubspecError` never actually was enabled during production in #79669, but only in some dart plugin tests. And in the tests the case of the error when enabling the flag was not explicitly tested. The only thing tested was, that it is not thrown when disabled.

As explained [here](https://github.com/flutter/flutter/pull/142035#discussion_r1484237904) the only case, where this error could be thrown is, when a dart implementation and a native inline implementation are provided simultaneously. But throwing an exception there is a wrong behavior, as both can coexist in a plugin package, thus in the pubspec file.

Disabling the flag means, that the error is not thrown and not shown to the user. This is the case in production (contrary to the dart plugin tests), which acts like these plugin cases of implementations are just skipped. And this is what actually should be done.

In conclusion, I think the case of coexisting dart and native implementation in pubspec was just overlooked and therefore this error validation was introduced, which is not necessary or even valid.

For more discussion, see: https://discord.com/channels/608014603317936148/608022056616853515/1200194937791205436

  - This is tricky: I already added a test in #142035, which finally complies with the other tests, by removing the flag. So I think it falls in the category of "remove dead code".
  - Theoretically this is a breaking change, as removing / altering some tests. But the flag actually was never valid or used, so IDK. But this may not does fall in the category of "contributed tests".
parent 36e09ff3
...@@ -1238,12 +1238,9 @@ bool hasPlugins(FlutterProject project) { ...@@ -1238,12 +1238,9 @@ bool hasPlugins(FlutterProject project) {
/// For more details, https://flutter.dev/go/federated-plugins. /// For more details, https://flutter.dev/go/federated-plugins.
// TODO(stuartmorgan): Expand implementation to apply to all implementations, // TODO(stuartmorgan): Expand implementation to apply to all implementations,
// not just Dart-only, per the federated plugin spec. // not just Dart-only, per the federated plugin spec.
// TODO(gustl22): The flag [throwOnPluginPubspecError] is currently only used
// for testing dart plugins as the logic is not working correctly.
List<PluginInterfaceResolution> resolvePlatformImplementation( List<PluginInterfaceResolution> resolvePlatformImplementation(
List<Plugin> plugins, { List<Plugin> plugins
bool throwOnPluginPubspecError = true, ) {
}) {
const Iterable<String> platformKeys = <String>[ const Iterable<String> platformKeys = <String>[
AndroidPlugin.kConfigKey, AndroidPlugin.kConfigKey,
IOSPlugin.kConfigKey, IOSPlugin.kConfigKey,
...@@ -1259,47 +1256,19 @@ List<PluginInterfaceResolution> resolvePlatformImplementation( ...@@ -1259,47 +1256,19 @@ List<PluginInterfaceResolution> resolvePlatformImplementation(
return '$packageName:$platform'; return '$packageName:$platform';
} }
bool hasPubspecError = false;
for (final String platformKey in platformKeys) { for (final String platformKey in platformKeys) {
for (final Plugin plugin in plugins) { for (final Plugin plugin in plugins) {
if (plugin.platforms[platformKey] == null && final String? defaultImplementation = plugin.defaultPackagePlatforms[platformKey];
plugin.defaultPackagePlatforms[platformKey] == null) { if (plugin.platforms[platformKey] == null && defaultImplementation == null) {
// The plugin doesn't implement this platform. // The plugin doesn't implement this platform.
continue; continue;
} }
String? implementsPackage = plugin.implementsPackage; String? implementsPackage = plugin.implementsPackage;
if (implementsPackage == null || implementsPackage.isEmpty) { if (implementsPackage == null || implementsPackage.isEmpty) {
final String? defaultImplementation =
plugin.defaultPackagePlatforms[platformKey];
final bool hasInlineDartImplementation = final bool hasInlineDartImplementation =
plugin.pluginDartClassPlatforms[platformKey] != null; plugin.pluginDartClassPlatforms[platformKey] != null;
if (defaultImplementation == null && !hasInlineDartImplementation) { if (defaultImplementation == null && !hasInlineDartImplementation) {
if (throwOnPluginPubspecError) { // Skip native inline PluginPlatform implementation
globals.printError(
"Plugin `${plugin.name}` doesn't implement a plugin interface, nor does "
'it specify an implementation in pubspec.yaml.\n\n'
'To set an inline implementation, use:\n'
'flutter:\n'
' plugin:\n'
' platforms:\n'
' $platformKey:\n'
' $kDartPluginClass: <plugin-class>\n'
'\n'
'To set a default implementation, use:\n'
'flutter:\n'
' plugin:\n'
' platforms:\n'
' $platformKey:\n'
' $kDefaultPackage: <plugin-implementation>\n'
'\n'
'To implement an interface, use:\n'
'flutter:\n'
' plugin:\n'
' implements: <plugin-interface>'
'\n',
);
}
hasPubspecError = true;
continue; continue;
} }
final String defaultImplementationKey = getResolutionKey(platform: platformKey, packageName: plugin.name); final String defaultImplementationKey = getResolutionKey(platform: platformKey, packageName: plugin.name);
...@@ -1348,9 +1317,6 @@ List<PluginInterfaceResolution> resolvePlatformImplementation( ...@@ -1348,9 +1317,6 @@ List<PluginInterfaceResolution> resolvePlatformImplementation(
)); ));
} }
} }
if (hasPubspecError && throwOnPluginPubspecError) {
throwToolExit('Please resolve the pubspec errors');
}
// Now resolve all the possible resolutions to a single option for each // Now resolve all the possible resolutions to a single option for each
// plugin, or throw if that's not possible. // plugin, or throw if that's not possible.
...@@ -1417,21 +1383,16 @@ List<PluginInterfaceResolution> resolvePlatformImplementation( ...@@ -1417,21 +1383,16 @@ List<PluginInterfaceResolution> resolvePlatformImplementation(
/// A successful run will create a new generate_main.dart file or update the existing file. /// A successful run will create a new generate_main.dart file or update the existing file.
/// Throws [ToolExit] if unable to generate the file. /// Throws [ToolExit] if unable to generate the file.
/// ///
/// This method also validates each plugin's pubspec.yaml, but errors are only
/// reported if [throwOnPluginPubspecError] is [true].
///
/// For more details, see https://flutter.dev/go/federated-plugins. /// For more details, see https://flutter.dev/go/federated-plugins.
Future<void> generateMainDartWithPluginRegistrant( Future<void> generateMainDartWithPluginRegistrant(
FlutterProject rootProject, FlutterProject rootProject,
PackageConfig packageConfig, PackageConfig packageConfig,
String currentMainUri, String currentMainUri,
File mainFile, { File mainFile,
bool throwOnPluginPubspecError = false, ) async {
}) async {
final List<Plugin> plugins = await findPlugins(rootProject); final List<Plugin> plugins = await findPlugins(rootProject);
final List<PluginInterfaceResolution> resolutions = resolvePlatformImplementation( final List<PluginInterfaceResolution> resolutions = resolvePlatformImplementation(
plugins, plugins,
throwOnPluginPubspecError: throwOnPluginPubspecError,
); );
final LanguageVersion entrypointVersion = determineLanguageVersion( final LanguageVersion entrypointVersion = determineLanguageVersion(
mainFile, mainFile,
......
...@@ -139,7 +139,6 @@ void main() { ...@@ -139,7 +139,6 @@ void main() {
appDependencies: directDependencies, appDependencies: directDependencies,
), ),
], ],
throwOnPluginPubspecError: false,
); );
expect(resolutions.length, equals(1)); expect(resolutions.length, equals(1));
...@@ -833,7 +832,6 @@ void main() { ...@@ -833,7 +832,6 @@ void main() {
packageConfig, packageConfig,
'package:app/main.dart', 'package:app/main.dart',
mainFile, mainFile,
throwOnPluginPubspecError: true,
); );
expect(flutterProject.dartPluginRegistrant.readAsStringSync(), expect(flutterProject.dartPluginRegistrant.readAsStringSync(),
'//\n' '//\n'
...@@ -957,7 +955,6 @@ void main() { ...@@ -957,7 +955,6 @@ void main() {
packageConfig, packageConfig,
'package:app/main.dart', 'package:app/main.dart',
mainFile, mainFile,
throwOnPluginPubspecError: true,
), throwsToolExit(message: ), throwsToolExit(message:
'Invalid plugin specification url_launcher_macos.\n' 'Invalid plugin specification url_launcher_macos.\n'
'Invalid "macos" plugin specification.' 'Invalid "macos" plugin specification.'
...@@ -998,7 +995,6 @@ void main() { ...@@ -998,7 +995,6 @@ void main() {
packageConfig, packageConfig,
'package:app/main.dart', 'package:app/main.dart',
mainFile, mainFile,
throwOnPluginPubspecError: true,
), throwsToolExit(message: ), throwsToolExit(message:
'Invalid plugin specification url_launcher_macos.\n' 'Invalid plugin specification url_launcher_macos.\n'
'Cannot find the `flutter.plugin.platforms` key in the `pubspec.yaml` file. ' 'Cannot find the `flutter.plugin.platforms` key in the `pubspec.yaml` file. '
...@@ -1011,35 +1007,6 @@ void main() { ...@@ -1011,35 +1007,6 @@ void main() {
ProcessManager: () => FakeProcessManager.any(), ProcessManager: () => FakeProcessManager.any(),
}); });
testUsingContext('Does not show error messages if throwOnPluginPubspecError is false', () async {
final Set<String> directDependencies = <String>{
'url_launcher_windows',
};
resolvePlatformImplementation(<Plugin>[
Plugin.fromYaml(
'url_launcher_windows',
'',
YamlMap.wrap(<String, dynamic>{
'platforms': <String, dynamic>{
'windows': <String, dynamic>{
'dartPluginClass': 'UrlLauncherPluginWindows',
},
},
}),
null,
<String>[],
fileSystem: fs,
appDependencies: directDependencies,
),
],
throwOnPluginPubspecError: false,
);
expect(testLogger.errorText, '');
}, overrides: <Type, Generator>{
FileSystem: () => fs,
ProcessManager: () => FakeProcessManager.any(),
});
testUsingContext('Does not create new entrypoint if there are no platform resolutions', () async { testUsingContext('Does not create new entrypoint if there are no platform resolutions', () async {
flutterProject.isModule = false; flutterProject.isModule = false;
...@@ -1057,7 +1024,6 @@ void main() { ...@@ -1057,7 +1024,6 @@ void main() {
packageConfig, packageConfig,
'package:app/main.dart', 'package:app/main.dart',
mainFile, mainFile,
throwOnPluginPubspecError: true,
); );
expect(flutterProject.dartPluginRegistrant.existsSync(), isFalse); expect(flutterProject.dartPluginRegistrant.existsSync(), isFalse);
}, overrides: <Type, Generator>{ }, overrides: <Type, Generator>{
...@@ -1097,7 +1063,6 @@ void main() { ...@@ -1097,7 +1063,6 @@ void main() {
packageConfig, packageConfig,
'package:app/main.dart', 'package:app/main.dart',
mainFile, mainFile,
throwOnPluginPubspecError: true,
); );
expect(flutterProject.dartPluginRegistrant.existsSync(), isTrue); expect(flutterProject.dartPluginRegistrant.existsSync(), isTrue);
...@@ -1113,7 +1078,6 @@ void main() { ...@@ -1113,7 +1078,6 @@ void main() {
packageConfig, packageConfig,
'package:app/main.dart', 'package:app/main.dart',
mainFile, mainFile,
throwOnPluginPubspecError: true,
); );
expect(flutterProject.dartPluginRegistrant.existsSync(), isFalse); expect(flutterProject.dartPluginRegistrant.existsSync(), isFalse);
}, overrides: <Type, Generator>{ }, overrides: <Type, Generator>{
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment