Skip to content
This repository has been archived by the owner on Jan 25, 2024. It is now read-only.

Settings enforcer #7

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Settings enforcer #7

wants to merge 2 commits into from

Conversation

schoenkaft
Copy link
Contributor

@schoenkaft schoenkaft commented Dec 6, 2019

First version without any settings 'normalization', so takes only Simply Static settings and applies them. Testend and seems to work fine, also with modifying stuff in Archiver.

For now this would be used as...

add_filter('grrr_simply_static_deploy_settings', function (array $settings) {
    $basePath = realpath(rtrim(ABSPATH, '/') . '/../app');
    return [
        'destination_url_type' => 'relative',
        'delivery_method' => 'local',
        'local_dir' => $basePath . '/static/',
        'temp_files_dir' => $basePath . '/static-temp/',
        'delete_temp_files' => '1',
        'additional_urls' =>
            rtrim(get_home_url(), '/') . '/sitemap_index.xml' . PHP_EOL .
            rtrim(get_home_url(), '/') . '/main-sitemap.xsl' . PHP_EOL
        ,
        'additional_files' =>
            rtrim(get_template_directory(), '/') . '/assets/build/' . PHP_EOL
        ,
        'urls_to_exclude' => [
            [
                'url' => '/app/uploads',
                'do_not_save' => '1',
                'do_not_follow' => '1',
            ],
            [
                'url' => '/app/themes/skd/assets/build/',
                'do_not_save' => '0',
                'do_not_follow' => '1',
            ],
        ],
    ];
}):

... although I'd prefer to make this a bit more user friendly if we're going forward with this. Any thoughts?

@harmenjanssen
Copy link
Member

... although I'd prefer to make this a bit more user friendly if we're going forward with this. Any thoughts?

I would accept additional_urls and additional_files as arrays, that looks a little more natural.

And this is before having looked at the code, but what does do_not_save imply, literally? I mean, being in urls_to_exclude seems to imply exactly that.

The $settings parameter contains the settings from the CMS (database), right?

foreach ($this->settings as $key => $value) {
Simply_Static\Options::instance()->set($key, $value);
}
Simply_Static\Options::instance()->save();
Copy link
Member

Choose a reason for hiding this comment

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

We have to save this back to the database, right? We cannot use a runtime instance in memory to finish the process?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No we don't, but I strongly think we should. And actually we can't 'not do it' easily since the Options::instance() is being saved lots of times during several 'native tasks'.

Plus it would be weird if we're enforcing settings from the code, and someone is able to update the UI settings and they would appear to be saved. If you save, and it immediately reverts back to whatever is enforced, that would make more sense to me. There might be other options, but when using this in practice, it felt pretty natural.

@schoenkaft
Copy link
Contributor Author

... although I'd prefer to make this a bit more user friendly if we're going forward with this. Any thoughts?

I would accept additional_urls and additional_files as arrays, that looks a little more natural.

Yep, that's what I'm saying. But we would have to think about this a bit more and I think it's not necessary right not to have this feature in the first release.

And this is before having looked at the code, but what does do_not_save imply, literally? I mean, being in urls_to_exclude seems to imply exactly that.

You can exclude URLs from being 'indexed' or you can exclude them from being followed. It replicates the settings UI of the plugin.

The $settings parameter contains the settings from the CMS (database), right?

In this 'example' it does. If we're going to abstract/normalize it, stuff will probably change, so would have to think about it if this would make sense.

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

Successfully merging this pull request may close these issues.

None yet

2 participants