[5.1] Jooa11y plugin and page cache conflicts

[Denitz]

Pull Request for Issue #37068.

Summary of Changes

1. Jooa11y plugin has issues with IDE (unknown classes and methods):
   1.1. getSubscribedEvents() throws unknown \Joomla\Plugin\System\Jooa11y\Extension\Exception
   1.2. initJooa11y() should return void
   1.3. Force $document class for IDE, now $document->addScriptOptions() and $document->getWebAssetManager() are detected by IDE
2. Extra useless cache files are created if system page cache is enabled.
3. The article updates are not visible on the next accessibility check after article save.

Testing Instructions

1. Enable Joomla system page cache plugin (browser caching should be disabled in the plugin settings).
2. Ensure that you are not logged in frontend.
3. Clear Joomla cache
4. Load article in frontend
5. See a single .php file in /administrator/cache/page
6. Edit the same article in backend, click "Accessibility Check" button to explore the checks
7. See another .php file in /administrator/cache/page
8. Now change the article text, click Save button
9. Click "Accessibility Check" again and see the old article version, no changes are applied.

Actual result BEFORE applying this Pull Request

System page cache plugin creates an extra file for each accessibility check.
No way to see the changes after article save without clearing 'page' cache manually.

Expected result AFTER applying this Pull Request

System page cache is not active on accessibility check,
The changes in articles are displayed after article save.

Link to documentations

Please select:

• Documentation link for docs.joomla.org:

• No documentation changes for docs.joomla.org needed

• Pull Request link for manual.joomla.org:

• No documentation changes for manual.joomla.org needed

[brianteeman]

Surely this is the wrong approach to handling the system page cache issue and it should be addressed in that plugin and not in jooa11y. Jooa11y only opens an accessibility check button from jooa11y that is the same as the preview button. In other words the issue you are describing should be handled at the source and not here.

[Denitz]

@brianteeman Sorry, but I disagree: system page plugin is a core plugin which works with all extensions, it can't contain any extension-specific logic, it should provide the common interface to disable caching if requested by any extension, both core and 3rd-party.

And it already has the onPageCacheSetCaching event which provides this functionality. My patch is based on this approach.

No sense to add the special checks for Jooa11y in system cache plugin, it creates additional useless code dependencies.

For instance, my extension also uses Jooa11y plugin, nobody will add the special checks for my extension in the core Joomla code, but I can easily utilize the onPageCacheSetCaching in my code, and it works fine, and it's normal from the code approach.

Same for Preview Article, it's another issue and it should be addressed in the com_content code but not in the cache plugin checking for special case if an article is previewed from backend.

[brianteeman]

Sorry but I disagree. The problem is in the system cache plugin. It simply is not good enough and should be aware when a page has been changed to refresh the cache,

The problem is not restricted to jooa11y. The view you see when you click on the accessibility check button is exactly the same as when you click on the preview button - just without jooa11y. The problem you describe exists in both. Dont play whack-a-mole. Solve the problem at the source.

If you look at the system cache plugin you can see that it already has options for it to be disabled at certain times. Simply put the system page cache plugin should not exist as its not very good and causes all sorts of problems. If it didnt then there would be no need for cache clean plugins.

[Denitz]

System cache plugin doesn't have issues here, it caches the page unless it's not possible or any other extension informs that the caching is not allowed due to extension-specific logic.

Adding the code into cache plugin will be exact case of whack-a-mole game.

An extension itself should inform the core that caching is not allowed on specific case, it's not the core which should detect all the cases. Moreover, it's just impossible to predict all the cases.

onPageCacheSetCaching is a clear and simple interface for an extension to prevent the page caching if an extension-specific logic demand such behaviour.

You want to clear the page cache once an article is saved? Sounds good, but it's another issue, not for this PR.

Here, I want to disable page caching on accessibility check preview, I don't need the useless cache files created on each check preview.

The problem origin is that the caching should be disabled, but not that the cache files should be cleared.

[brianteeman]

You want to clear the page cache once an article is saved? Sounds good, but it's another issue, not for this PR.

It is the only way to truly resolve the problem

Here, I want to disable page caching on accessibility check preview, I don't need the useless cache files created on each check preview.

But you are ok for those cache files to be created for a regular preview?

The problem origin is that the caching should be disabled, but not that the cache files should be cleared.

If you want to go down that route then I still say that it should be done in the cache plugin not here. It already has code to be disabled in selected conditions.

[Denitz]

Clearing the page cache on article save can help for sure, but once again, we don't need the cache created on accessibility check and cache enabled on accessibility check - that's the purpose of this patch.

And clearing page cache on article save won't fix the issue with extra cache files created on accessibility check.

Caching preview is another issue, I can't include everything into a single PR, otherwise it will be reviewed for years.

My patch exactly uses the code from the cache plugin via onPageCacheSetCaching event.

[brianteeman]

and finally after testing this doesnt work.

Expected result AFTER applying this Pull Request
System page cache is not active on accessibility check,
The changes in articles are displayed after article save.

if the page cache already exists (for example you did a preview) and then make some changes to the content and do an accessibility check then you will not see the changes. For all the reasons I already explained

[Denitz]

Once I save an article, I only see the cached verson of preview, but the accessibility check displays the new version.

[brianteeman]

and when you view the preview again then you see the old one. problem confirmed. you are solving a valid problem but in the wrong place

chrome_2023-09-28_10-17-43.mp4

[Denitz]

@brian, this PR is not about cached preview but about accessibility check. Preview requires separate PR.

In your video at about 1:34, you typed the new chars but didn't save article, hence see the old version in checked.

I am solving the one exact problem about checker requests being cached by page cache plugin.

[brianteeman]

In your video at about 1:34, you typed the new chars but didn't save article, hence see the old version in checked.

correct - thats a mostake by mwe but it still checked the old version before the save made at 1:10

I am solving the one exact problem about checker requests being cached by page cache plugin.

and as I have said it make no difference if you dont create a new cache if you are still serving an existing cache so you are not solving your own problem here at all

This PR is simply wrong

[Denitz]

correct - that's a mistake by me but it still checked the old version before the save made at 1:10

Sorry, I don't issues with accessibility checks before 1:10, the button click at 0:45 displayed the correct previously saved contents.

[wilsonge]

I mean I think you're both kinda wrong. It doesn't belong in system cache plugin itself - it's true that the cache should be kinda unaware - it should just respect the flag. Brian is right however that preview as a whole needs to ignore cache though - otherwise preview as a function is pointless.

[bembelimen]

So after discussing this in the maintainer team we see here two issues, while this PR fixes one of it.

So we would suggest the following approach:

• we should not register the listener on the fly but do a clean subscription in the getSubscribedEvents
• we should in a new PR handle the preview issue, where on preview the cache is disabled, too

So @Denitz could you add the first point of my list here in this PR? Then it's good to go. After that we can look into the preview issue.

[brianteeman]

if you dont address the preview cache then you cant solve anything for the reasons already explained

[HLeithner]

if you dont address the preview cache then you cant solve anything for the reasons already explained

the preview works different (no plugin, no extra parameter) so have to be fixed in a different way (adding parameter for example) but it's "unrelated" to this pr.

[brianteeman]

oh well I tried to explain but I have clearly failed. There are not enough bricks in the wall. When the pr is rewritten as per your request you will see that the preview still prevents the jooa11y check to be an uncached view

[HLeithner]

ok will try to reproduce this my self and comeback when I can reproduce it and or solve it

[HLeithner]

Ok I tested it my self and the pr does what it says, it fixes the jooa11y plugin, it's not fixing the default preview, for this it has to be solved in it's own pr.

[brianteeman]

if the page has already been cached then even with this pr you will still see the cached file

[HLeithner]

I tested it once again and tried to create a video for you but wayland is not so nice for such feature... So it took longer and looks not so nice^^ anyway it hopefully proofs my point

video.mp4

You can see that the hit counter on preview doesn't increase but it increases on pressing accessibility button. In your video you for got one time to press the save button before hitting the a11y button

[brianteeman]

I still say that its fixing the problem in the wrong place and it should be fixed once in the cache plugin. Although I'd prefer that that plugin was simply killed