Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[4.x] Split global set variables into its own repository and stache store #8343

Conversation

ryanmitchell
Copy link
Contributor

@ryanmitchell ryanmitchell commented Jun 23, 2023

There seems to be a lot of people who want to split config from data when using eloquent driver, which is already mostly possible apart from forms and globals. Forms will be possible if/when this merges.

See for reference:
statamic/eloquent-driver#172
statamic/eloquent-driver#128
statamic/eloquent-driver#147
statamic/eloquent-driver#168

This PR splits global variables out into its own repository and store but maintains sharing the same directory as globals, so at a file-level nothing changes. All tests are passing locally which is a good sign that I haven't broken anything, and I was trying not to affect the existing structure in any way so made changes around it rather than simply adding a new directory for variables.

Could you let me know if this is something you would consider merging, and if so I'll spend some more time ensure the test coverage is up to scratch.

[Also, sorry. I know my PRs are always time consuming to review]

@jasonvarga
Copy link
Member

Also, sorry. I know my PRs are always time consuming to review

The best ones are.

@fdeneux
Copy link

fdeneux commented Jul 27, 2023

Thanks @ryanmitchell , our team is missing this feature. @jasonvarga any idea when your team will be able to review (and consider merging) this PR?

We work with multiple environments so we like to split config from data, with config being version controlled and data environment specific. We've managed to so for all stores except globals, meaning that our team is required to replicate config changes for globals on all environments.

@ryanmitchell
Copy link
Contributor Author

If its critical to you then composer patch it in.

@jasonvarga
Copy link
Member

I'm running into an issue while working on #8505 and I think this PR would help. So I'm working on this now. 👍

- Globals no longer hold the localizations in a property. They are retrieved on demand from the variables repo.
- The GlobalSet class will save and delete localizations rather than the repo. It'll need to happen regardless of the repo implementation.
- Make the globals repo dumber - the way it was. Let it just get the global sets from the store. Don't worry about the variables.
- Call ->values() in findBySet. Since it filters, the keys could be whack.
@jasonvarga jasonvarga merged commit 424fc84 into statamic:4.x Aug 8, 2023
16 checks passed
@ryanmitchell
Copy link
Contributor Author

Thank you. Did it end up helping fix the other issue (#8505)?

@ryanmitchell ryanmitchell deleted the feature/allow-split-globals-variables-respositories branch August 8, 2023 17:59
@jasonvarga
Copy link
Member

Yep it did, and it's what I focused most on with this PR.

But right after merging this I realized this PR totally doesn't work when you have a single site and with the variables directory different from the globals. The variables store doesn't find the base file. Kinda had my multi-site blinders on while reviewing this.

I'm thinking that a simple temporary solution is to throw an exception when you're using the stache and have different directories defined for globals and global-variables.

@ryanmitchell
Copy link
Contributor Author

Yeah that’s a good point. Never thought of trying different directories as the files are merged in that situation.

I think that’s a reasonable solution. Maybe in statamic 5 you always split the files even in single site mode?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants