[plugin_platform_interface] Add a new method verify that prevents use of const Object() as token.
#4640
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat. If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
| /// This is implemented as a static method so that it cannot be overridden | ||
| /// with `noSuchMethod`. | ||
| static void verifyToken(PlatformInterface instance, Object token) { | ||
| assert(token != const Object()); |
There was a problem hiding this comment.
Per discussion in the issue, we can't add this without making a breaking version change. What we can do though is soft-deprecate this (i.e., put a comment that it shouldn't be used any more, but not an annotation since that would break our own builds) and make a new method that has this assertion then calls verifyToken.
(We can then update all the 1P plugins to call the new method, then make it a real deprecation and move the implementation to a shared private method so we're not calling the deprecated version.)
There was a problem hiding this comment.
I've implemented your suggested changes, PTAL
collinjackson
force-pushed
the
static-final
branch
from
f0ff58e to
af1b670
Compare
collinjackson
changed the title
PlatformInterface.verify that prevents use of const Object() as token.
| # Please consider carefully before bumping the major version of this package, ideally it should only | ||
| # be done when absolutely necessary and after the ecosystem has already migrated to 1.X.Y version | ||
| # that is forward compatible with 2.0.0 (ideally the ecosystem have migrated to depend on: | ||
| # `plugin_platform_interface: >=1.X.Y <3.0.0`). |
There was a problem hiding this comment.
All the versions in this comment need to be updated as well (1->2, 2->3, 3->4)
There was a problem hiding this comment.
Good catch, thanks.
| PlatformInterface.verify(instance, _token); | ||
| // A real implementation would set a static instance field here. | ||
| } | ||
| } |
There was a problem hiding this comment.
We should make a version of this that still uses verifyToken so we're still exercising the codepath that >100 plugins are likely currently using, to make sure we don't break it later.
|
@stuartmorgan: Can you please land and publish this for me when you get a chance? I think it's done. Thanks for the feedback! |
|
I didn't realize you didn't have commit permissions still; if you need a sponsor to be added back just let me know :)
We live in the future now! PRs with version changes are automatically published once post-submit tests pass. |
…events use of `const Object()` as token. (flutter/plugins#4640)
I will take you up on that! But I have to land "several" PRs to be eligible, so please review #4643 next.
🎉 |
…events use of `const Object()` as token. (flutter/plugins#4640)
🤣
|
verify, a more future-proof name. It throwsAssertionErrorifconst Object()used as the instance's token.verifyTokenwith a comment. It will actually deprecated in a future release, to avoid breaking tests with this minor change.PlatformInterfaceto show new usage ofverifyand other cosmetic fixes.Fixes flutter/flutter#96178.
Pre-launch Checklist
dart format.)[shared_preferences]pubspec.yamlwith an appropriate new version according to the [pub versioning philosophy], or this PR is [exempt from version changes].CHANGELOG.mdto add a description of the change, [following repository CHANGELOG style].///).