Skip to content
This repository was archived by the owner on Feb 22, 2023. It is now read-only.

[path_provider_foundation] Adding support for getContainerPath on iOS #7114

Closed
leighajarett wants to merge 2 commits into from
Closed

[path_provider_foundation] Adding support for getContainerPath on iOS #7114

leighajarett wants to merge 2 commits into from

Conversation

@leighajarett
Copy link

@leighajarett leighajarett commented Feb 7, 2023

This PR adds a method getContainerPath in the path_provider_foundation plugin, which returns the path to the container for a given App Group. It uses the getContainerUrl API.
This is only implemented for iOS, since App Groups works a bit differently on macOS.

Issue fixed: flutter/flutter#117685

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 [relevant style guides] and ran [the auto-formatter]. (Unlike the flutter/flutter repo, the flutter/plugins repo does use dart format.)
  • I signed the [CLA].
  • The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
  • I listed at least one issue that this PR fixes in the description above.
  • I updated pubspec.yaml with an appropriate new version according to the [pub versioning philosophy], or this PR is [exempt from version changes].
  • I updated CHANGELOG.md to add a description of the change, [following repository CHANGELOG style].
  • 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].
  • All existing and new tests are passing.

@leighajarett leighajarett requested a review from jmagman as a code owner February 7, 2023 13:31
@leighajarett
Copy link
Author

leighajarett commented Feb 7, 2023

To add tests for this, I added an App Group to the Apple Developer Flutter team and used that in the example app -- but not sure if that's the best approach :)

@leighajarett leighajarett mentioned this pull request Feb 14, 2023
Merged
3 tasks
jmagman
jmagman reviewed Feb 14, 2023
View reviewed changes
Copy link
Member

@jmagman jmagman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To add tests for this, I added an App Group to the Apple Developer Flutter team and used that in the example app -- but not sure if that's the best approach :)

I think that's okay, @stuartmorgan do you have an opinion?

@jmagman jmagman mentioned this pull request Feb 15, 2023
Open
jmagman
jmagman reviewed Feb 15, 2023
View reviewed changes
packages/path_provider/path_provider_foundation/test/path_provider_foundation_test.dart

test('getContainerPath', () async {
const String appGroupIdentifier = 'group.example.test';
if (Platform.isIOS) {
Copy link
Member

@jmagman jmagman Feb 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stuartmorgan is there a suggested way to mock out the platform in these tests? I know how to do it in the flutter_tool but not here.

Copy link
Contributor

@stuartmorgan-g stuartmorgan-g Feb 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It usually doesn't come up here given the ways plugins are structured; I would just write a trivial object-based wrapper around the Platform check and make it injectable in the constructor. (And then move the PathProviderFoundation construction out of setUp, which is something I general prefer anyway.)

Copy link
Author

@leighajarett leighajarett Feb 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain what that means? 🙈 Constructor for what, the PathProviderFoundation?

Copy link
Contributor

@stuartmorgan-g stuartmorgan-g Feb 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, exactly; it would look like this, with the parameter being some little class you define (e.g., PathProviderPlatformProvider) that is itself @visibleForTesting. The Platform call would move into that helper class, so you could inject a FakePathProviderPlatformProvider in tests that gives whatever answer the test case requires.

@stuartmorgan-g
Copy link
Contributor

stuartmorgan-g commented Feb 22, 2023

We've just completed the migration of the plugin code to the flutter/packages repository, as described in https://flutter.dev/go/flutter-plugins-repo-migration, and this repository is now being archived. Unfortunately that means that all in-progress PRs here must be moved to flutter/packages.

Please see our instructions for an explanation of how to move your PR, and if you have any issues moving your PR please don't hesitate to reach out in the #hackers-ecosystem channel in Discord.

Our apologies that your PR was caught in this one-time transition. We're aware that it's disruptive in the short term, and appreciate your help in getting us to a better long-term state!

@leighajarett leighajarett mentioned this pull request Mar 13, 2023
Merged
11 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@stuartmorgan-g stuartmorgan-g stuartmorgan-g left review comments

@jmagman jmagman jmagman left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

p: path_provider platform-ios

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Get AppGroup container path from path_provider

3 participants

@leighajarett @stuartmorgan-g @jmagman