Skip to content

Commit dc6f94a

Browse files
authored
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](flutter/flutter#142035 (comment)) 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".
1 parent 36e09ff commit dc6f94a

File tree

2 files changed

+7
-82
lines changed

2 files changed

+7
-82
lines changed

packages/flutter_tools/lib/src/flutter_plugins.dart

Lines changed: 7 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -1238,12 +1238,9 @@ bool hasPlugins(FlutterProject project) {
12381238
/// For more details, https://flutter.dev/go/federated-plugins.
12391239
// TODO(stuartmorgan): Expand implementation to apply to all implementations,
12401240
// not just Dart-only, per the federated plugin spec.
1241-
// TODO(gustl22): The flag [throwOnPluginPubspecError] is currently only used
1242-
// for testing dart plugins as the logic is not working correctly.
12431241
List<PluginInterfaceResolution> resolvePlatformImplementation(
1244-
List<Plugin> plugins, {
1245-
bool throwOnPluginPubspecError = true,
1246-
}) {
1242+
List<Plugin> plugins
1243+
) {
12471244
const Iterable<String> platformKeys = <String>[
12481245
AndroidPlugin.kConfigKey,
12491246
IOSPlugin.kConfigKey,
@@ -1259,47 +1256,19 @@ List<PluginInterfaceResolution> resolvePlatformImplementation(
12591256
return '$packageName:$platform';
12601257
}
12611258

1262-
bool hasPubspecError = false;
12631259
for (final String platformKey in platformKeys) {
12641260
for (final Plugin plugin in plugins) {
1265-
if (plugin.platforms[platformKey] == null &&
1266-
plugin.defaultPackagePlatforms[platformKey] == null) {
1261+
final String? defaultImplementation = plugin.defaultPackagePlatforms[platformKey];
1262+
if (plugin.platforms[platformKey] == null && defaultImplementation == null) {
12671263
// The plugin doesn't implement this platform.
12681264
continue;
12691265
}
12701266
String? implementsPackage = plugin.implementsPackage;
12711267
if (implementsPackage == null || implementsPackage.isEmpty) {
1272-
final String? defaultImplementation =
1273-
plugin.defaultPackagePlatforms[platformKey];
12741268
final bool hasInlineDartImplementation =
12751269
plugin.pluginDartClassPlatforms[platformKey] != null;
12761270
if (defaultImplementation == null && !hasInlineDartImplementation) {
1277-
if (throwOnPluginPubspecError) {
1278-
globals.printError(
1279-
"Plugin `${plugin.name}` doesn't implement a plugin interface, nor does "
1280-
'it specify an implementation in pubspec.yaml.\n\n'
1281-
'To set an inline implementation, use:\n'
1282-
'flutter:\n'
1283-
' plugin:\n'
1284-
' platforms:\n'
1285-
' $platformKey:\n'
1286-
' $kDartPluginClass: <plugin-class>\n'
1287-
'\n'
1288-
'To set a default implementation, use:\n'
1289-
'flutter:\n'
1290-
' plugin:\n'
1291-
' platforms:\n'
1292-
' $platformKey:\n'
1293-
' $kDefaultPackage: <plugin-implementation>\n'
1294-
'\n'
1295-
'To implement an interface, use:\n'
1296-
'flutter:\n'
1297-
' plugin:\n'
1298-
' implements: <plugin-interface>'
1299-
'\n',
1300-
);
1301-
}
1302-
hasPubspecError = true;
1271+
// Skip native inline PluginPlatform implementation
13031272
continue;
13041273
}
13051274
final String defaultImplementationKey = getResolutionKey(platform: platformKey, packageName: plugin.name);
@@ -1348,9 +1317,6 @@ List<PluginInterfaceResolution> resolvePlatformImplementation(
13481317
));
13491318
}
13501319
}
1351-
if (hasPubspecError && throwOnPluginPubspecError) {
1352-
throwToolExit('Please resolve the pubspec errors');
1353-
}
13541320

13551321
// Now resolve all the possible resolutions to a single option for each
13561322
// plugin, or throw if that's not possible.
@@ -1417,21 +1383,16 @@ List<PluginInterfaceResolution> resolvePlatformImplementation(
14171383
/// A successful run will create a new generate_main.dart file or update the existing file.
14181384
/// Throws [ToolExit] if unable to generate the file.
14191385
///
1420-
/// This method also validates each plugin's pubspec.yaml, but errors are only
1421-
/// reported if [throwOnPluginPubspecError] is [true].
1422-
///
14231386
/// For more details, see https://flutter.dev/go/federated-plugins.
14241387
Future<void> generateMainDartWithPluginRegistrant(
14251388
FlutterProject rootProject,
14261389
PackageConfig packageConfig,
14271390
String currentMainUri,
1428-
File mainFile, {
1429-
bool throwOnPluginPubspecError = false,
1430-
}) async {
1391+
File mainFile,
1392+
) async {
14311393
final List<Plugin> plugins = await findPlugins(rootProject);
14321394
final List<PluginInterfaceResolution> resolutions = resolvePlatformImplementation(
14331395
plugins,
1434-
throwOnPluginPubspecError: throwOnPluginPubspecError,
14351396
);
14361397
final LanguageVersion entrypointVersion = determineLanguageVersion(
14371398
mainFile,

packages/flutter_tools/test/general.shard/dart_plugin_test.dart

Lines changed: 0 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -139,7 +139,6 @@ void main() {
139139
appDependencies: directDependencies,
140140
),
141141
],
142-
throwOnPluginPubspecError: false,
143142
);
144143

145144
expect(resolutions.length, equals(1));
@@ -833,7 +832,6 @@ void main() {
833832
packageConfig,
834833
'package:app/main.dart',
835834
mainFile,
836-
throwOnPluginPubspecError: true,
837835
);
838836
expect(flutterProject.dartPluginRegistrant.readAsStringSync(),
839837
'//\n'
@@ -957,7 +955,6 @@ void main() {
957955
packageConfig,
958956
'package:app/main.dart',
959957
mainFile,
960-
throwOnPluginPubspecError: true,
961958
), throwsToolExit(message:
962959
'Invalid plugin specification url_launcher_macos.\n'
963960
'Invalid "macos" plugin specification.'
@@ -998,7 +995,6 @@ void main() {
998995
packageConfig,
999996
'package:app/main.dart',
1000997
mainFile,
1001-
throwOnPluginPubspecError: true,
1002998
), throwsToolExit(message:
1003999
'Invalid plugin specification url_launcher_macos.\n'
10041000
'Cannot find the `flutter.plugin.platforms` key in the `pubspec.yaml` file. '
@@ -1011,35 +1007,6 @@ void main() {
10111007
ProcessManager: () => FakeProcessManager.any(),
10121008
});
10131009

1014-
testUsingContext('Does not show error messages if throwOnPluginPubspecError is false', () async {
1015-
final Set<String> directDependencies = <String>{
1016-
'url_launcher_windows',
1017-
};
1018-
resolvePlatformImplementation(<Plugin>[
1019-
Plugin.fromYaml(
1020-
'url_launcher_windows',
1021-
'',
1022-
YamlMap.wrap(<String, dynamic>{
1023-
'platforms': <String, dynamic>{
1024-
'windows': <String, dynamic>{
1025-
'dartPluginClass': 'UrlLauncherPluginWindows',
1026-
},
1027-
},
1028-
}),
1029-
null,
1030-
<String>[],
1031-
fileSystem: fs,
1032-
appDependencies: directDependencies,
1033-
),
1034-
],
1035-
throwOnPluginPubspecError: false,
1036-
);
1037-
expect(testLogger.errorText, '');
1038-
}, overrides: <Type, Generator>{
1039-
FileSystem: () => fs,
1040-
ProcessManager: () => FakeProcessManager.any(),
1041-
});
1042-
10431010
testUsingContext('Does not create new entrypoint if there are no platform resolutions', () async {
10441011
flutterProject.isModule = false;
10451012

@@ -1057,7 +1024,6 @@ void main() {
10571024
packageConfig,
10581025
'package:app/main.dart',
10591026
mainFile,
1060-
throwOnPluginPubspecError: true,
10611027
);
10621028
expect(flutterProject.dartPluginRegistrant.existsSync(), isFalse);
10631029
}, overrides: <Type, Generator>{
@@ -1097,7 +1063,6 @@ void main() {
10971063
packageConfig,
10981064
'package:app/main.dart',
10991065
mainFile,
1100-
throwOnPluginPubspecError: true,
11011066
);
11021067
expect(flutterProject.dartPluginRegistrant.existsSync(), isTrue);
11031068

@@ -1113,7 +1078,6 @@ void main() {
11131078
packageConfig,
11141079
'package:app/main.dart',
11151080
mainFile,
1116-
throwOnPluginPubspecError: true,
11171081
);
11181082
expect(flutterProject.dartPluginRegistrant.existsSync(), isFalse);
11191083
}, overrides: <Type, Generator>{

0 commit comments

Comments
 (0)