Skip to content

PaywallEventStore: changed container to use URL.applicationSupportDirectory#3289

Merged
NachoSoto merged 3 commits into
mainfrom
nacho/sdk-3273-revenuecat-appears-in-documents-folder-in-container
Oct 9, 2023
Merged

PaywallEventStore: changed container to use URL.applicationSupportDirectory#3289
NachoSoto merged 3 commits into
mainfrom
nacho/sdk-3273-revenuecat-appears-in-documents-folder-in-container

Conversation

@NachoSoto

@NachoSoto NachoSoto commented Oct 7, 2023

Copy link
Copy Markdown
Contributor

See https://nemecek.be/blog/57/making-files-from-your-app-available-in-the-ios-files-app#writing-files-into-the-application-support-directory

Unfortunately the choice of URL.documentsDirectory meant that any app wanting to make its documents accessible via the Files app,
would have exposed these events.

…Directory`

See https://nemecek.be/blog/57/making-files-from-your-app-available-in-the-ios-files-app#writing-files-into-the-application-support-directory

Unfortunately the choice of `URL.documentsDirectory` meant that any app wanting to make its documents accessible via the Files app,
would have exposed these events.
@NachoSoto NachoSoto added the pr:fix A bug fix label Oct 7, 2023
@NachoSoto NachoSoto requested a review from a team October 7, 2023 00:04
@NachoSoto NachoSoto force-pushed the nacho/sdk-3273-revenuecat-appears-in-documents-folder-in-container branch from 6cf4a33 to 7a9b75b Compare October 9, 2023 17:00
@NachoSoto NachoSoto force-pushed the nacho/sdk-3273-revenuecat-appears-in-documents-folder-in-container branch from 7a9b75b to c83596b Compare October 9, 2023 17:02

let documentsDirectory = try documentsDirectory ?? Self.documentsDirectory
let url = documentsDirectory
Self.removeStoreIfExists(Self.url(in: documentsDirectory))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

maybe we can call this something like removeLegacyDirectoryIfExists so it's obvious at first glance what we're doing and why?

@NachoSoto NachoSoto enabled auto-merge (squash) October 9, 2023 17:57
@NachoSoto NachoSoto merged commit ceae31f into main Oct 9, 2023
@NachoSoto NachoSoto deleted the nacho/sdk-3273-revenuecat-appears-in-documents-folder-in-container branch October 9, 2023 18:05
NachoSoto added a commit that referenced this pull request Oct 18, 2023
Fixes #3316. Follow up to #3289.

That previous fix was only removing the store itself, but not the `revenuecat` folder.

Running an app with 4.27.2 produces this:
```bash
$ tree -L 3 ~/Library/Developer/CoreSimulator/Devices/.../data/Containers/Data/Application/.../
~/Library/Developer/CoreSimulator/Devices/.../data/Containers/Data/Application/.../
├── Documents
│   └── revenuecat
│       └── paywall_event_store
```

After this fix, the Documents directory is empty.
```bash
tree -L 4 ~/Library/Developer/CoreSimulator/Devices/.../data/Containers/Data/Application/.../
/Users/ignacio/Library/Developer/CoreSimulator/Devices/.../data/Containers/Data/Application/.../
├── Documents
├── Library
│   ├── Application Support
│   │   └── revenuecat
│   │       └── paywall_event_store
```
NachoSoto added a commit that referenced this pull request Oct 18, 2023
…ory (#3317)

Fixes #3316. Follow up to #3289.

That previous fix was only removing the store itself, but not the
`revenuecat` folder.

Running an app with 4.27.2 produces this:
```bash
$ tree -L 3 ~/Library/Developer/CoreSimulator/Devices/.../data/Containers/Data/Application/.../
~/Library/Developer/CoreSimulator/Devices/.../data/Containers/Data/Application/.../
├── Documents
│   └── revenuecat
│       └── paywall_event_store
```

After this fix, the Documents directory is empty.
```bash
tree -L 4 ~/Library/Developer/CoreSimulator/Devices/.../data/Containers/Data/Application/.../
/Users/ignacio/Library/Developer/CoreSimulator/Devices/.../data/Containers/Data/Application/.../
├── Documents
├── Library
│   ├── Application Support
│   │   └── revenuecat
│   │       └── paywall_event_store
```

This is still covered by the same test introduced in #3289.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr:fix A bug fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants