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

Sitemap cache not working with getkirby/staticache plugin #39

Open
bnomei opened this issue Mar 8, 2023 · 9 comments
Open

Sitemap cache not working with getkirby/staticache plugin #39

bnomei opened this issue Mar 8, 2023 · 9 comments

Comments

@bnomei
Copy link

bnomei commented Mar 8, 2023

CleanShot 2023-03-08 at 19 48 38

1.) key should not contain a dot since the staticache uses that instead of /
https://github.com/getkirby/staticache/blob/9ef2d7195b5c8212fcf53194407cc28aec0e3901/src/StatiCache.php#L144

2.) i think the data set should not be a string but an array.
https://github.com/getkirby/kirby/blob/9ecd11cd5b7d4875d4212f767a2b3ad3e52b3528/src/Cms/Page.php#L1030

BUT the staticache plugin expects a real page object (cache key => page id) so it will all fail anyway
https://github.com/getkirby/staticache/blob/9ef2d7195b5c8212fcf53194407cc28aec0e3901/src/StatiCache.php#L106

how to fix this? add a plugin cache and check for staticache plugin but match array for pages cache just to be safe

App::plugin('fabianmichael/meta', [
    'options' => [
       //...
        'cache' => true,
    ],
$routes[] = [
        'pattern' => 'sitemap.xml',
        'action' => function () use ($kirby) {
            if ($kirby->option('fabianmichael.meta.sitemap') === false && SiteMeta::robots('index') === false) {
                $this->next();
            }

            $sitemap = [];
            $cache = $kirby->cache(
+                option('cache.pages.type') !== 'static' ? 'pages' : 'fabianmichael.meta'
            );
            $cacheKey = 'sitemap.xml';

            if (option('debug') === true || ! ($sitemap = $cache->get($cacheKey))) {
                $sitemap = Sitemap::factory()->generate();
+              $cache->set($cacheKey, [
+                    'html' => $sitemap,
+               ]);
            }

            return new Response($sitemap, 'application/xml');
        },
    ];
@bnomei
Copy link
Author

bnomei commented Mar 8, 2023

ah the cache needs an expire if it will not be flushed with the pages cache. but i think thats an okayish compromise.

               $cache->set($cacheKey, [
                    'html' => $sitemap,
               ], option('cache.pages.type') !== 'static' ? 0 : 15);

@fabianmichael
Copy link
Owner

@bnomei Thanks for your research an the decvelopment of a solution. But I am hesitating to incorporate this solution since it makes everything more fragile and would make it dependant on one specific other plugin. I’d really prefer, if the staticache plugin would offer a solution that also works with "virtual" pages.

Could the caching issue maybe resolved by using hooks that listen to events like page.update:after etc.?

@bnomei
Copy link
Author

bnomei commented Mar 9, 2023

yes you could just flush your plugins custom cache on each page updated hook. no performance drawbacks with that.

@fabianmichael
Copy link
Owner

@bnomei I’ve updated the plugin with the lastest commit based on your code. However, I’ve opted for always using the plugin’s own cache. (see dd6bdec)

Please give it a try. If it works, I can create a new release.

@bnomei
Copy link
Author

bnomei commented Mar 11, 2023

added a comment to the commit for error in src/config/hooks.php. but apart from that it seems to work fine. thanks.

@CHE1RON
Copy link
Contributor

CHE1RON commented Mar 24, 2023

Would be great to get confirmation on this .. is everything working as expected? 😉

@bnomei
Copy link
Author

bnomei commented Mar 24, 2023

it does. i have dev branches of both staticache and meta running side by side successfully.

@fabianmichael
Copy link
Owner

Would be great to get confirmation on this .. is everything working as expected? 😉

As far as I am aware, the only thing that does not work correctly at the moment is when the headers option is active.

@CHE1RON
Copy link
Contributor

CHE1RON commented Mar 24, 2023

Thanks guys! I've got trouble right now getting staticache to produce HTML files to begin with, but after that I'll see for myself 😀

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

No branches or pull requests

3 participants