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

Fix caching #79

Merged
merged 1 commit into from
May 7, 2023
Merged

Fix caching #79

merged 1 commit into from
May 7, 2023

Conversation

sizzname
Copy link
Contributor

Users who can add pages have seen a cached page for users who are not allowed to add pages

@dregad dregad changed the title Fix chaching Fix caching May 7, 2023
@dregad dregad added the bug label May 7, 2023
@dregad dregad merged commit 52cbbe7 into dregad:master May 7, 2023
@@ -83,7 +83,8 @@ public function render($mode, Doku_Renderer $renderer, $data) {
global $lang;

if($mode == 'xhtml') {
$disablecache = null;
$disablecache = true;
if($disablecache) $renderer->info['cache'] = false;
$namespaceinput = $this->_htmlNamespaceInput($data['namespace'], $disablecache);
Copy link
Contributor

Choose a reason for hiding this comment

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

Please note, there was some decision making in the _htmlNamespaceInput() method, which modifies $disablecache by reference.

Copy link
Owner

Choose a reason for hiding this comment

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

Damn, I failed to consider that, thanks for the heads up @Klap-in.

So in fact this change is not only useless, but also harmful because $renderer->info['cache'] will no longer be set correctly.

I'll revert.

dregad added a commit that referenced this pull request May 10, 2023
This reverts commit 52cbbe7.

As @Klap-in pointed out [1] this change was incorrect as $disablecache
is passed by reference and updated by _htmlNamespaceInput().

Conflicts:
	syntax.php

[1]: #79 (comment)
@dregad
Copy link
Owner

dregad commented Sep 27, 2023

@Klap-in I've been looking into this some more, and to be honest I can't think of any way to fix the caching the problem, other than always calling $renderer->nocache();...

Of course, that makes the code you added in _htmlNamespaceInput() with the $disablecache byref parameter useless, and that should be cleaned up.

I know it was a long time ago that you contributed that (716e610 was committed in march 2014), but before I make any changes, maybe you remember and can share some insight as to why you decided to only selectively disable the cache that way, vs doing it systematically.

As a side note, I also read the Dokuwiki documentation on caching, and to be honest I find it a bit difficult to understand. In particular, I am confused with the recommended usage of the PARSER_CACHE_USE event, i.e. how it should be implemented and why... Since you authored that doc, Maybe you can offer some advice or point me to some examples.

Thanks in advance for your help !

@Klap-in
Copy link
Contributor

Klap-in commented Sep 27, 2023

In general you like to have caching to speed up the loading of a page. It is especially useful for pages that are bigger and and show often. If it becomes ACL dependent we can consider different things, e.g. creating caches per group or per user (you modify then the key e.g. the indexmenu is doing that, disadvantage of that solution is that you add extra configuration option), or just load this piece of information by ajax, such that it can be generated fresh everytime.

EDIT (dregad): fix link to new plugin version

@dregad
Copy link
Owner

dregad commented Sep 27, 2023

Thanks for the feedback and the link to the tindexmenu plugin source code.

At first I was very confused because it seemed like a very old, obsolete plugin, until I found the related extension page linking to a newer, maintained version ;-)

I'll have a look and get back to you if I have more questions.

@Klap-in
Copy link
Contributor

Klap-in commented Sep 27, 2023

Sorry, I meant indexmenu plugin indeed. The indexmenu plugin is receiving a overhaul of its code (see PR #254). The javascript version will switch to ajax loading of the nodes (default per one level). The fallback version and the non-javascript tree are still included in the page, and need a fine-grained cache.

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

Successfully merging this pull request may close these issues.

3 participants