search

akeeba / engage

Akeeba Engage - Comments for Joomla!™ articles made easy
GNU General Public License v3.0
16 stars 10 forks source link

Cache is not properly handled on ?akengage_cid= links #305

Closed NicolasDerumigny closed 8 months ago

NicolasDerumigny commented 8 months ago

Please read the README.md in the repository's root and the support resources before submitting an issue.

Steps to reproduce the issue

Set up Akeeba engage with conservative caching on. Open a link to a comment containing akengage_cid parameter (for example using the latest comments module) that points to a comment figuring on the second page of the comment pager. Try to go back to the first comment page.

Expected result

The first comment page is displayed.

Actual result

The second comment page is displayed.

Troubleshooting already performed

Disabling the cache makes the issue disappear. I believe the page cache entry for any akengage_cid page is considered as the first page and not the actual displayed page, hence the bug.

System information

Mandatory information

Issues without this information can not and will not be replied to.

Good to have information

You can skip some or all of this information. However, the more information you provide the faster and better we can help.

Additional comments

None

nikosdion commented 8 months ago

The parameters which matter are akengage_limitstart and akengage_limit. They are added to Joomla's caching parameters by the System – Akeeba Engage cache support plugin. If that plugin is disabled, Joomla! does not know which page of comments it's rendering; it will just cache the last page of comments it rendered most recently and display that.

NicolasDerumigny commented 8 months ago

Yes, therefore there is a bug, as akengage_cid=42 comment may lead to a comment page that is not the the first (e.g. being equivalent to akengage_limitstart=10 for example). As it is not taken into account, the cache mechanism will wrongly assume that akengage_cid=42 is akengage_limitstart=0 instead of akengage_limitstart=10, and thus mess up the cache? Or I have not understood things correctly? (EDIT: System – Akeeba Engage cache support plugin is enable on my system)

nikosdion commented 8 months ago

Hm, we've hit a limitation of how Joomla! works. Interesting.

We cannot know which page of comments to present when akengage_cid is non-empty until we are in the controller method handling the main display task, as we do not have the article ID before this point in time, therefore we don't even know which comments we are talking about (the CID may or may not be valid, and it may or may not point to a comment belonging to the current article, and the page may or may not be a valid com_content page). However, this Engage controller method is only called by the cache controller. The cache controller has to calculate the caching key before that, otherwise it cannot know if it should call the Engage controller method or just fetch something from the cache.

This problem gets worse. For caching to work correctly when using the conservative cache and, very importantly, the page cache plugin we need to pass the cache-important parameters in the onAfterInitialise event, before SEF URLs are even parsed. Therefore, we can't weasel our way around and determine the akengage_limitstart from the akengage_cid even if we would accept doing the work twice, slowing everything down..

So, we need something we don't have to determine if we need to run what will give us what we need to have. Chicken, meet egg.

Moreover, because Joomla! treats the the after-article text as static text we cannot apply our own caching separately from the article caching, even when using progressive caching.

The only thing I can think of is set akengage_limitstart to -1000000 when the akengage_cid is non-empty and try to remove the akengage_cid from pagination, even though I am not sure if the latter is at all possible because of the way Joomla! handles the pagination links.

Let me try that, noting that there are limits to the dark magic fuckery I use to work around decisions made in Joomla! 18 years ago when it was still called Mambo.

NicolasDerumigny commented 8 months ago

How about using akengage_cid as input to the caching key, just as akengage_limitstart and akengage_limit? This will lead to caching twice the same page, once for the cid links and one for the limitstart, but that would be the only downside if I understood everything correctly?

nikosdion commented 8 months ago

That would not help. A non-zero akengage_cid without akengage_limitstart is identical to the first page of comments as seen at this point in time.

nikosdion commented 8 months ago

Wait a second.

I cannot actually reproduce this issue without making any changes. I just took your word for it.

You need to enable the System – Akeeba Engage plugin. That's all.

nikosdion commented 8 months ago

To make this more clear.

The plugin's onAfterInitialise method adds the akengage_limitstart and akengage_limit to the request.

The \Akeeba\Component\Engage\Site\View\Comments\HtmlView::display method removes the akengage_cid parameter from the pagination.

So, we have two distinct types of URLs.

  1. The URL with akengage_cid (not participating in the caching key), with akengage_limitstart set to NULL gets a certain caching key.
  2. The paginated URLs without akengage_cid but also with non-NULL akengage_limitstart get a completely different caching key, even when akengage_limitstart is 0 (first page of comments).

Therefore doing this:

Set up Akeeba engage with conservative caching on. Open a link to a comment containing akengage_cid parameter (for example using the latest comments module) that points to a comment figuring on the second page of the comment pager. Try to go back to the first comment page.

actually does work. Why does it work? Because the first link I visited had a non-empty akengage_cid, but no akengage_limitstart (NULL), whereas the second link I visited did not have akengage_cid, but it did have a non-NULL akengage_limitstart. They caching keys are different, therefore I can see the correct page.

Note that I tried this with conservative caching as per the reproduction instructions.

NicolasDerumigny commented 8 months ago

I do not understand. The plug-in is enabled on my test system, page cache is disabled and conservative caching is set (cache handler file, 1 minute cache time). Note that the issue is only present in guest (non logged-in) visits ; I do not reproduce once logged in. EDIT: I modify some links to include a reference to an if (#akengage-comments-section), can it mess up the caching system?

NicolasDerumigny commented 8 months ago

https://github.com/akeeba/engage/blob/8be3b5de07a041e8a6837221503d78f2f099a79b/plugins/system/engagecache/src/Extension/Engagecache.php#L64 Indeed, akengage_limitstart is set to 0, not NULL when not in the parameters. When I correct the line to be set to -1, the issue goes away. Is it a problem with my setup?

EDIT: forget it, I am mixing default values with url-related ones

nikosdion commented 8 months ago

What can I tell you? I tried the same thing you did, exactly as you described, and with a non-logged-in user. For good measure, I tested twice. Once, I cleaned the cache, visited all comment pages (I have ten of them), then visited a comment on page 8 with a URL that has the akengage_cid and went back to page 1. For the next test I cleaned the cache, visited the comment on page 8 with a URL that has the akengage_cid and went back to page 1.

I modify some links to include a reference to an if (#akengage-comments-section), can it mess up the caching system?

It shouldn't. Anything after the pound sign is not even seen by web server. The web server is only sent the URL before the fragment (pound sign and everything that follows it).

Indeed, akengage_limitstart is set to 0, not NULL when not in the parameters. When I correct the line to be set to -1, the issue goes away. Is it a problem with my setup?

You are missing something :) Look at lines 76 to 79. If $limitStart is 0 the if-block's condition is false, therefore the akengage_limitstart is NOT set to the application's registered URL parameters which are used for calculating the cache ID.

By setting it to -1 you did what I described above. However, this should not be necessary.

Instead of doing this, try the following which is a more sane solution. Replace line 64 with:

$cid        = $app->input->getInt('akengage_cid', 0);
        $limitStart = $app->input->getInt('akengage_limitstart', $cid > 0 ? -1 : 0);

This will set $limitStart to -1 only if there is a non-empty comment ID.

NicolasDerumigny commented 8 months ago

This fix corrects my issue. However, I have no clue how that works on your system:

You are missing something :) Look at lines 76 to 79. If $limitStart is 0 the if-block's condition is false, therefore the akengage_limitstart is NOT set to the application's registered URL parameters which are used for calculating the cache ID.

If ?limitstart is unset, then $limitStart = $app->input->getInt('akengage_limitstart', 0) will be set to 0, which is exactly the same than a 0 limitStart, and no limitStart URL register parameter will be saved. So I do not get how page with non-NULL akengage_cid (and no limitStart) gets resolved differently than the vanilla page with no parameters (which resolves the same than the page with ?akengage_limitstart=0)...

NicolasDerumigny commented 8 months ago

Moreover, doesn't that mean that there is a key conflict when two akengage_cid lead to different pages? Because they will resolve to limitStart=NULL and therefore the same cache key as far as I understand... and reproduce on my system.

nikosdion commented 8 months ago

Replace the entire caching plugin with the new version I just pushed.

NicolasDerumigny commented 8 months ago

That works flawlessly. Thanks a lot for your dark magic!

nikosdion commented 8 months ago

Awesome! Thank you for the feedback :)