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

Automate sites.yaml config #81

Merged
merged 17 commits into from
Nov 6, 2024
Merged

Automate sites.yaml config #81

merged 17 commits into from
Nov 6, 2024

Conversation

kevinmeijer97
Copy link
Member

We can automate the generation of the new sites.yaml.
By doing this we reduce the amount of config needed to configure the sites, only keeping a few settings in the sites array of config/rapidez/statamic.php.

Not sure yet if setting this during boot is the way to go, could also generate it through a command when setting up the site.
Let me know what you would prefer!

src/RapidezStatamicServiceProvider.php Outdated Show resolved Hide resolved
src/RapidezStatamicServiceProvider.php Outdated Show resolved Hide resolved
Copy link
Collaborator

@BobWez98 BobWez98 left a comment

Choose a reason for hiding this comment

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

This feels a bit manual, maybel we can add some magic to do it on the fly, but this is just food for thought.

src/Commands/CreateSites.php Outdated Show resolved Hide resolved
src/Commands/CreateSites.php Outdated Show resolved Hide resolved
}

Site::setSites($sites);
Site::save();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since we're getting this from the database anyway, can't we maybe just overwrite the getSavedSites function in Statamic\Sites\Sites to just return a cached array with the queries already done? Then we set the sites on the fly and we can get rid of the sites.yaml all together 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes i agree, i feel like this might be the best way to go.
I cached the sites array for now because of the setStore() function which is needed to get the correct site data.
After rapidez/core#567 gets merged we can use getValue() function without having to do the setStore().

BobWez98
BobWez98 previously approved these changes Oct 18, 2024
config/rapidez/statamic.php Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/Extend/SitesLinkedToMagentoStores.php Outdated Show resolved Hide resolved
src/Extend/SitesLinkedToMagentoStores.php Show resolved Hide resolved
src/Extend/SitesLinkedToMagentoStores.php Show resolved Hide resolved
src/Extend/SitesLinkedToMagentoStores.php Show resolved Hide resolved
src/RapidezStatamicServiceProvider.php Outdated Show resolved Hide resolved
Co-authored-by: Roy Duineveld <[email protected]>
@kevinmeijer97
Copy link
Member Author

I think we should wait with merging so we can release it in bulk along with the Hybrid runway changes.

BobWez98
BobWez98 previously approved these changes Oct 22, 2024
README.md Outdated
It will mainly setup the [Eloquent driver](https://github.com/statamic/eloquent-driver) and publish the necessary vendor from the rapidez/statamic repo.
It will mainly setup the [Eloquent driver](https://github.com/statamic/eloquent-driver) and publish the necessary vendor from the rapidez/statamic repo.

Before running the install script, make sure you have an existing User model as Statamic requires this. See: [Statamic docs](https://statamic.dev/tips/storing-users-in-a-database)
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed the readme with a clearer explanation for how and what with the user model!

@kevinmeijer97
Copy link
Member Author

Moved most of the configuration guide to the Rapidez docs (rapidez/docs#56).

BobWez98
BobWez98 previously approved these changes Oct 30, 2024
'locale' => 'en_EN',
'lang' => 'en_EN',
'url' => '/',
'attributes' => [
'magento_store_id' => 1,
'group' => 'default',
'disabled' => false,
],
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we still need this whole sites config?

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed this around, see #81 (comment)

$configModel = config('rapidez.models.config');

foreach ($stores as $store) {
if (config('rapidez.statamic.sites.' . $store['code'] . '.attributes.disabled')) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can't we safely say we always want this to be enabled? And otherwise maybe just have an array that includes the disabled stores?

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed this around, see #81 (comment)

@BobWez98
Copy link
Collaborator

We can merge this in my opinion, what about yours @royduin

@royduin
Copy link
Member

royduin commented Oct 31, 2024

We've a conflict but besides that I'm also curious about the answers on your latest questions. It's currently unclear where the sites in the config is for or when to use it.

@kevinmeijer97
Copy link
Member Author

We've a conflict but besides that I'm also curious about the answers on your latest questions. It's currently unclear where the sites in the config is for or when to use it.

Conflict has been fixed!
Also refactored the sites array and replaced it with a disabled_sites array, there is no need to have a sites array in the config anymore when the only thing configurable is the disabled option.
You can add the store_codes of the stores you don't want in Statamic to the disabled_sites array.

BobWez98
BobWez98 previously approved these changes Nov 6, 2024
config/rapidez/statamic.php Show resolved Hide resolved
src/Extend/SitesLinkedToMagentoStores.php Show resolved Hide resolved
@royduin royduin merged commit 2c527ca into master Nov 6, 2024
@royduin royduin deleted the feature/site-config branch November 6, 2024 09:31
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