From dc6f94a878443e9dd8d8585793a6e0f9af17f19d Mon Sep 17 00:00:00 2001 From: August Date: Fri, 8 Mar 2024 00:04:08 +0100 Subject: [PATCH] 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". --- .../lib/src/flutter_plugins.dart | 53 +++---------------- .../test/general.shard/dart_plugin_test.dart | 36 ------------- 2 files changed, 7 insertions(+), 82 deletions(-) diff --git a/packages/flutter_tools/lib/src/flutter_plugins.dart b/packages/flutter_tools/lib/src/flutter_plugins.dart index e9fcb2d7a4a..72f7dbb123d 100644 --- a/packages/flutter_tools/lib/src/flutter_plugins.dart +++ b/packages/flutter_tools/lib/src/flutter_plugins.dart @@ -1238,12 +1238,9 @@ bool hasPlugins(FlutterProject project) { /// For more details, https://flutter.dev/go/federated-plugins. // TODO(stuartmorgan): Expand implementation to apply to all implementations, // 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 resolvePlatformImplementation( - List plugins, { - bool throwOnPluginPubspecError = true, -}) { + List plugins +) { const Iterable platformKeys = [ AndroidPlugin.kConfigKey, IOSPlugin.kConfigKey, @@ -1259,47 +1256,19 @@ List resolvePlatformImplementation( return '$packageName:$platform'; } - bool hasPubspecError = false; for (final String platformKey in platformKeys) { for (final Plugin plugin in plugins) { - if (plugin.platforms[platformKey] == null && - plugin.defaultPackagePlatforms[platformKey] == null) { + final String? defaultImplementation = plugin.defaultPackagePlatforms[platformKey]; + if (plugin.platforms[platformKey] == null && defaultImplementation == null) { // The plugin doesn't implement this platform. continue; } String? implementsPackage = plugin.implementsPackage; if (implementsPackage == null || implementsPackage.isEmpty) { - final String? defaultImplementation = - plugin.defaultPackagePlatforms[platformKey]; final bool hasInlineDartImplementation = plugin.pluginDartClassPlatforms[platformKey] != null; if (defaultImplementation == null && !hasInlineDartImplementation) { - if (throwOnPluginPubspecError) { - 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: \n' - '\n' - 'To set a default implementation, use:\n' - 'flutter:\n' - ' plugin:\n' - ' platforms:\n' - ' $platformKey:\n' - ' $kDefaultPackage: \n' - '\n' - 'To implement an interface, use:\n' - 'flutter:\n' - ' plugin:\n' - ' implements: ' - '\n', - ); - } - hasPubspecError = true; + // Skip native inline PluginPlatform implementation continue; } final String defaultImplementationKey = getResolutionKey(platform: platformKey, packageName: plugin.name); @@ -1348,9 +1317,6 @@ List resolvePlatformImplementation( )); } } - if (hasPubspecError && throwOnPluginPubspecError) { - throwToolExit('Please resolve the pubspec errors'); - } // Now resolve all the possible resolutions to a single option for each // plugin, or throw if that's not possible. @@ -1417,21 +1383,16 @@ List resolvePlatformImplementation( /// A successful run will create a new generate_main.dart file or update the existing 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. Future generateMainDartWithPluginRegistrant( FlutterProject rootProject, PackageConfig packageConfig, String currentMainUri, - File mainFile, { - bool throwOnPluginPubspecError = false, -}) async { + File mainFile, +) async { final List plugins = await findPlugins(rootProject); final List resolutions = resolvePlatformImplementation( plugins, - throwOnPluginPubspecError: throwOnPluginPubspecError, ); final LanguageVersion entrypointVersion = determineLanguageVersion( mainFile, diff --git a/packages/flutter_tools/test/general.shard/dart_plugin_test.dart b/packages/flutter_tools/test/general.shard/dart_plugin_test.dart index b53f2bc1f79..88ec09565c8 100644 --- a/packages/flutter_tools/test/general.shard/dart_plugin_test.dart +++ b/packages/flutter_tools/test/general.shard/dart_plugin_test.dart @@ -139,7 +139,6 @@ void main() { appDependencies: directDependencies, ), ], - throwOnPluginPubspecError: false, ); expect(resolutions.length, equals(1)); @@ -833,7 +832,6 @@ void main() { packageConfig, 'package:app/main.dart', mainFile, - throwOnPluginPubspecError: true, ); expect(flutterProject.dartPluginRegistrant.readAsStringSync(), '//\n' @@ -957,7 +955,6 @@ void main() { packageConfig, 'package:app/main.dart', mainFile, - throwOnPluginPubspecError: true, ), throwsToolExit(message: 'Invalid plugin specification url_launcher_macos.\n' 'Invalid "macos" plugin specification.' @@ -998,7 +995,6 @@ void main() { packageConfig, 'package:app/main.dart', mainFile, - throwOnPluginPubspecError: true, ), throwsToolExit(message: 'Invalid plugin specification url_launcher_macos.\n' 'Cannot find the `flutter.plugin.platforms` key in the `pubspec.yaml` file. ' @@ -1011,35 +1007,6 @@ void main() { ProcessManager: () => FakeProcessManager.any(), }); - testUsingContext('Does not show error messages if throwOnPluginPubspecError is false', () async { - final Set directDependencies = { - 'url_launcher_windows', - }; - resolvePlatformImplementation([ - Plugin.fromYaml( - 'url_launcher_windows', - '', - YamlMap.wrap({ - 'platforms': { - 'windows': { - 'dartPluginClass': 'UrlLauncherPluginWindows', - }, - }, - }), - null, - [], - fileSystem: fs, - appDependencies: directDependencies, - ), - ], - throwOnPluginPubspecError: false, - ); - expect(testLogger.errorText, ''); - }, overrides: { - FileSystem: () => fs, - ProcessManager: () => FakeProcessManager.any(), - }); - testUsingContext('Does not create new entrypoint if there are no platform resolutions', () async { flutterProject.isModule = false; @@ -1057,7 +1024,6 @@ void main() { packageConfig, 'package:app/main.dart', mainFile, - throwOnPluginPubspecError: true, ); expect(flutterProject.dartPluginRegistrant.existsSync(), isFalse); }, overrides: { @@ -1097,7 +1063,6 @@ void main() { packageConfig, 'package:app/main.dart', mainFile, - throwOnPluginPubspecError: true, ); expect(flutterProject.dartPluginRegistrant.existsSync(), isTrue); @@ -1113,7 +1078,6 @@ void main() { packageConfig, 'package:app/main.dart', mainFile, - throwOnPluginPubspecError: true, ); expect(flutterProject.dartPluginRegistrant.existsSync(), isFalse); }, overrides: {