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

Version 1.0.1 #22

Merged
merged 7 commits into from
Mar 24, 2023
Merged

Version 1.0.1 #22

merged 7 commits into from
Mar 24, 2023

Conversation

lukasbestle
Copy link
Member

@lukasbestle lukasbestle commented Mar 11, 2023

Fixes

Documentation

  • Add more information to the README on required changes to the server setup for custom roots
  • The PHP loader example code now also tries to determine the content type from the static files
  • Clean up indentation in code examples

After merging:

@lukasbestle
Copy link
Member Author

@jaro-io @jonasfeige @bnomei @fabianmichael Could you please test the develop branch to see if this fixes the issue for you?

@bnomei
Copy link

bnomei commented Mar 11, 2023

i guess it would work if the plugins do honor the array format the pages cache expects for the data, not a plain string.

CleanShot 2023-03-11 at 12 47 50

@bnomei
Copy link

bnomei commented Mar 11, 2023

CleanShot 2023-03-11 at 12 49 14

@lukasbestle
Copy link
Member Author

@bnomei Thanks for testing. You are right, this requires additional changes to the plugin. See my comment in #20 (comment).

@bnomei
Copy link

bnomei commented Mar 11, 2023

apart from that it works. thanks.

src/StatiCache.php Outdated Show resolved Hide resolved
@fabianmichael
Copy link

fabianmichael commented Mar 13, 2023

@lukasbestle Another issue that still exists is, that the PHP-loader currently does not work for content representations, as it does not send any content-type header on its own … it should be possible to use e.g. PHP’s built-in mimetype functions to bypass the loading of Kirby’s toolkit, though I don’t have any experience with using them. Please be a ware, that this function can throw an E_WARNING that has to be dealt with:

    if ($mime = @mime_content_type($path)) {
      header("Content-type: $mime");
    };
  
    die(file_get_contents($path));

Apart from that, it seems to work great so far. I’ll revert my updates to the meta plugin, so it uses staticache again.

@lukasbestle
Copy link
Member Author

@fabianmichael Which PHP loader are you referring to? Staticache does not provide one, do you mean @bnomei's?

@fabianmichael
Copy link

fabianmichael commented Mar 13, 2023

@fabianmichael Which PHP loader are you referring to? Staticache does not provide one, do you mean @bnomei's?

@lukasbestle I’m talking about that loader mentioned in the README: https://github.com/getkirby/staticache/#:~:text=index.php%3F%24query_string%3B%0A%7D-,PHP%20loader%3A,-If%20you%20don%27t

@lukasbestle
Copy link
Member Author

Ah, yes. Completely forgot about that one. 🙈😅

I'll see what I can do. The header mode is certainly more reliable in this case, but automatic detection could already be good enough in 99% of cases.

Copy link

@fabianmichael fabianmichael left a comment

Choose a reason for hiding this comment

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

@lukasbestle It works much better now, but when enabling the headers option, it still fails with virtual pages:

Undefined array key "response" (StatiCache.php L58)

It would probably make sense use automatic testing at this point and provide a few common use cases (built-in pages, content representations, virtual pages) as test scenarios.

I am also wondering, if there are any situations, where one does not want fo use the headers option when using the PHP loader. I guess it’s more performant than using PHP’s built-in mime config and very likely way more reliable (I don’t have much experience with PHP’s mime functions, but there’s probably a reason why almost every CMS has its own implementation).

@lukasbestle
Copy link
Member Author

TBH I feel like we are mixing different issues into the discussion. If this version solves the compatibility issues with your Meta plugin, I suggest we already publish it. We can then work on fixes for any other issues separately for 1.0.2 and in separate GitHub issues.

It works much better now, but when enabling the headers option, it still fails with virtual pages:

Could you please open an issue for that with steps to reproduce?

It would probably make sense use automatic testing at this point and provide a few common use cases (built-in pages, content representations, virtual pages) as test scenarios.

I agree. So far we did not find the time to write tests, but we need them anyway before we can include Staticache in the Kirby core at some point.

I am also wondering, if there are any situations, where one does not want fo use the headers option when using the PHP loader. I guess it’s more performant than using PHP’s built-in mime config and very likely way more reliable (I don’t have much experience with PHP’s mime functions, but there’s probably a reason why almost every CMS has its own implementation).

We already state the performance disadvantages of the PHP loader, but I think you are right: If one wants to use the PHP loader, it is better in most cases to go for the header variant. I've added a note to the README.

@fabianmichael
Copy link

Could you please open an issue for that with steps to reproduce?

@lukasbestle Thanks for the update, unfortunately I don’t really have time at the moment for dealing with additional contributions. Test setup for the issue is: Install plainkit, staticache and meta plugin, enable static cache with headers, use PHP loader and point for browser to plainkit.whatever/sitemap.xml to reproduce the issue.

@lukasbestle
Copy link
Member Author

@fabianmichael Ah, I see. If I understand it correctly, this is not actually about virtual pages but about manually storing cache values in the page cache. See #22 (comment).

@bnomei
Copy link

bnomei commented Mar 19, 2023

its a bit about virtual pages since the sitemap would be treated like a page and be available for apache just like any other statically cached page as html document.
but yes technically its just about storing the data in cache properly.

@fabianmichael
Copy link

@lukasbestle @bnomei Would it be safer to just use a custom page model for the Sitemap and a template? That way, caching and rendering would be handled by $page->render() like for any other page? That would probably make the whole implementation more robust and future-proof than trying to replicate that by myself. What would you recommend? It’s probably a good idea to come up with a general solution for virtual pages and add a paragraph to the readme about how to deal with them.

@lukasbestle
Copy link
Member Author

I wouldn't expect changes to Kirby's storage format for the page cache, however I cannot guarantee that we will never need to make any changes there. But if we did, it would be clearly communicated as a breaking change.

So I think both ways are fine: Either you adapt your plugin to the existing storage format for the cache or you perform a larger rewrite (which may be a breaking change for the users of your plugin though).

@bnomei
Copy link

bnomei commented Mar 24, 2023

i just tested dev branches of both plugins together and it works fine.

@lukasbestle lukasbestle merged commit b16c5e3 into main Mar 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants