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

Gustl22 · 2024-02-27T09:15:10Z

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 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

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I signed the CLA.
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making, or this PR is test-exempt.
- This is tricky: I already added a test in refactor: Differentiate pubspec and resolution errors for plugins #142035, which finally complies with the other tests, by removing the flag. So I think it falls in the category of "remove dead code".
I followed the breaking change policy and added Data Driven Fixes where supported.
- 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".
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

Gustl22 · 2024-02-27T10:05:57Z

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

@@ -139,7  139,6 @@ void main() {
              appDependencies: directDependencies,
            ),
          ],
-          throwOnPluginPubspecError: false,


Note that this test failed before, when setting throwOnPluginPubspecError: true, which should not be the case.
This test was introduced by myself by purpose in #142035 to show the misbehavior of the flag.

Gustl22 · 2024-02-27T10:07:27Z

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

-            appDependencies: directDependencies,
-          ),
-        ],
-          throwOnPluginPubspecError: false,


Note that this test always succeeded, when setting throwOnPluginPubspecError either to true or to false, which makes this test kind of "useless", because there's no such valid case, which could actually raise the error.

stuartmorgan

LGTM. My recollection is that this was disabled pretty much immediately because the logic wasn't actually right.

I think the only case I've seen in recent memory where an error message would have been useful is the case where a desktop plugin doesn't get autoregistration because of a lack of implements, but even that is unlikely to be useful going forward since new plugins are unlikely to support older than Flutter 2.11.

Given that, just removing it as cruft seems like the right choice.

Gustl22 · 2024-02-29T21:05:15Z

I appreciate your speedy review and taking the time!

christopherfujino

(un)-ship it!

auto-submit · 2024-03-07T22:00:13Z

auto label is removed for flutter/flutter/144214, due to - The status or check suite Google testing has failed. Please fix the issues identified (or deflake) before re-applying this label.

…validation (flutter/flutter#144214)

flutter/flutter@471a828...7c89ec8 2024-03-08 [email protected] Remove `toString()` overrides in `dart:ui`/`package:flutter` in profile/release mode on wasm/vm targets (flutter/flutter#144763) 2024-03-08 [email protected] Roll Flutter Engine from 80cd7981043f to bbb1ed00af49 (3 revisions) (flutter/flutter#144813) 2024-03-08 65758246 [email protected] Update documentation of `AlertDialog`'s default `TextStyle` for Material 3 (flutter/flutter#144697) 2024-03-08 [email protected] Roll Flutter Engine from 175069397a40 to 80cd7981043f (5 revisions) (flutter/flutter#144804) 2024-03-07 [email protected] Enable asset transformation for `flutter run -d <browser>` and `flutter test` (flutter/flutter#144734) 2024-03-07 [email protected] Roll Flutter Engine from ec4f9af3d53b to 175069397a40 (2 revisions) (flutter/flutter#144799) 2024-03-07 [email protected] refactor: Remove `throwOnPluginPubspecError` flag for plugin validation (flutter/flutter#144214) 2024-03-07 [email protected] Roll Flutter Engine from 305333c50191 to ec4f9af3d53b (1 revision) (flutter/flutter#144797) 2024-03-07 [email protected] Roll Flutter Engine from c2863530ce39 to 305333c50191 (1 revision) (flutter/flutter#144793) 2024-03-07 [email protected] Fixed -> DropdownMenu throws exception when it is in any scrollable lâ�¦ (flutter/flutter#140566) 2024-03-07 [email protected] Roll Flutter Engine from 9e8ccaa3842e to c2863530ce39 (2 revisions) (flutter/flutter#144792) 2024-03-07 [email protected] Roll Flutter Engine from 12828950890a to 9e8ccaa3842e (1 revision) (flutter/flutter#144784) 2024-03-07 [email protected] Marks Mac_x64 module_test_ios to be flaky (flutter/flutter#144681) 2024-03-07 [email protected] [flutter_tools] add custom tool analysis to analyze.dart, lint Future.catchError (flutter/flutter#140122) 2024-03-07 [email protected] Roll Flutter Engine from f8c3b2db8cd1 to 12828950890a (1 revision) (flutter/flutter#144775) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/ doc/main/autoroll/README.md

…validation (flutter/flutter#144214)

flutter/flutter@471a828...7c89ec8 2024-03-08 [email protected] Remove `toString()` overrides in `dart:ui`/`package:flutter` in profile/release mode on wasm/vm targets (flutter/flutter#144763) 2024-03-08 [email protected] Roll Flutter Engine from 80cd7981043f to bbb1ed00af49 (3 revisions) (flutter/flutter#144813) 2024-03-08 65758246 [email protected] Update documentation of `AlertDialog`'s default `TextStyle` for Material 3 (flutter/flutter#144697) 2024-03-08 [email protected] Roll Flutter Engine from 175069397a40 to 80cd7981043f (5 revisions) (flutter/flutter#144804) 2024-03-07 [email protected] Enable asset transformation for `flutter run -d <browser>` and `flutter test` (flutter/flutter#144734) 2024-03-07 [email protected] Roll Flutter Engine from ec4f9af3d53b to 175069397a40 (2 revisions) (flutter/flutter#144799) 2024-03-07 [email protected] refactor: Remove `throwOnPluginPubspecError` flag for plugin validation (flutter/flutter#144214) 2024-03-07 [email protected] Roll Flutter Engine from 305333c50191 to ec4f9af3d53b (1 revision) (flutter/flutter#144797) 2024-03-07 [email protected] Roll Flutter Engine from c2863530ce39 to 305333c50191 (1 revision) (flutter/flutter#144793) 2024-03-07 [email protected] Fixed -> DropdownMenu throws exception when it is in any scrollable lâ�¦ (flutter/flutter#140566) 2024-03-07 [email protected] Roll Flutter Engine from 9e8ccaa3842e to c2863530ce39 (2 revisions) (flutter/flutter#144792) 2024-03-07 [email protected] Roll Flutter Engine from 12828950890a to 9e8ccaa3842e (1 revision) (flutter/flutter#144784) 2024-03-07 [email protected] Marks Mac_x64 module_test_ios to be flaky (flutter/flutter#144681) 2024-03-07 [email protected] [flutter_tools] add custom tool analysis to analyze.dart, lint Future.catchError (flutter/flutter#140122) 2024-03-07 [email protected] Roll Flutter Engine from f8c3b2db8cd1 to 12828950890a (1 revision) (flutter/flutter#144775) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/ doc/main/autoroll/README.md

fix: Remove throwOnPluginPubspecError flag for plugins

ce0b72d

github-actions bot added the tool Affects the "flutter" command-line tool. See also t: labels. label Feb 27, 2024

Gustl22 changed the title ~~fix: Remove throwOnPluginPubspecError flag for plugins~~ fix: Remove throwOnPluginPubspecError flag for plugins Feb 27, 2024

Gustl22 changed the title ~~fix: Remove throwOnPluginPubspecError flag for plugins~~ fix: Remove throwOnPluginPubspecError flag for plugin validation Feb 27, 2024

Gustl22 changed the title ~~fix: Remove throwOnPluginPubspecError flag for plugin validation~~ refactor: Remove throwOnPluginPubspecError flag for plugin validation Feb 27, 2024

Gustl22 commented Feb 27, 2024

View reviewed changes

Gustl22 added 3 commits February 27, 2024 12:40

Merge branch 'master' into 80374-refactor-3

907d701

Merge branch 'master' into 80374-refactor-3

71f4597

Merge branch 'master' into 80374-refactor-3

dcf6b9c

stuartmorgan approved these changes Feb 29, 2024

View reviewed changes

Gustl22 mentioned this pull request Mar 2, 2024

refactor: Perform plugin resolution per platform #144506

Merged

9 tasks

Merge branch 'master' into 80374-refactor-3

9b0005c

christopherfujino approved these changes Mar 7, 2024

View reviewed changes

christopherfujino added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 7, 2024

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Mar 7, 2024

Merge branch 'master' into 80374-refactor-3

63d4069

christopherfujino added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 7, 2024

auto-submit bot merged commit dc6f94a into flutter:master Mar 7, 2024
120 checks passed

Gustl22 deleted the 80374-refactor-3 branch March 8, 2024 00:09

engine-flutter-autoroll mentioned this pull request Mar 8, 2024

Roll Flutter from 471a82856d86 to 7c89ec8bbc6d (15 revisions) flutter/packages#6288

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 8, 2024

dc6f94a refactor: Remove throwOnPluginPubspecError flag for plugin …

d33afba

…validation (flutter/flutter#144214)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 14, 2024

dc6f94a refactor: Remove throwOnPluginPubspecError flag for plugin …

917d659

…validation (flutter/flutter#144214)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Gustl22 commented Feb 27, 2024 •

edited

Loading

Gustl22 Feb 27, 2024 •

edited

Loading

Gustl22 Feb 27, 2024 •

edited

Loading

stuartmorgan left a comment

Gustl22 commented Feb 29, 2024

christopherfujino left a comment

auto-submit bot commented Mar 7, 2024

refactor: Remove throwOnPluginPubspecError flag for plugin validation #144214

refactor: Remove throwOnPluginPubspecError flag for plugin validation #144214

Conversation

Gustl22 commented Feb 27, 2024 • edited Loading

Pre-launch Checklist

Gustl22 Feb 27, 2024 • edited Loading

Choose a reason for hiding this comment

Gustl22 Feb 27, 2024 • edited Loading

Choose a reason for hiding this comment

stuartmorgan left a comment

Choose a reason for hiding this comment

Gustl22 commented Feb 29, 2024

christopherfujino left a comment

Choose a reason for hiding this comment

auto-submit bot commented Mar 7, 2024

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

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

Gustl22 commented Feb 27, 2024 •

edited

Loading

Gustl22 Feb 27, 2024 •

edited

Loading

Gustl22 Feb 27, 2024 •

edited

Loading