[#21952] - Create "cache" folder if it doesn't exist before trying to write to it
- Fixed in Code Base
- 29 Apr 2019
- Medium
- Build: staging
- # 21952
- Diff
- AndySDH:patch-1
- Success Hound No violations found. Woof! Details
- Success continuous-integration/travis-ci/pr The Travis CI build passed Details
- Success continuous-integration/drone/pr the build was successful Details
- Success continuous-integration/appveyor/pr AppVeyor build succeeded Details
User tests: Successful: Unsuccessful:
This fixes this issue:
Steps to reproduce the issue
- Have cache enabled
- Delete cache folder
Result before PR
Sites breaks. Goes blank.
With error reporting on, throws error:
"Error: Call to a member function getTag() on null: The file Cache Storage is not supported on this platform."
Result after PR
Joomla creates "cache" folder if it doesn't exist
Additional comments
Now, of course there is no reason why one should delete the cache folder but it can happen by accident or for whatever other reason. The principle that Joomla doesn't check if the folder is there or not (and breaks the site if it's not) before trying to write to it is wrong.
| Status | New | ⇒ | Pending |
joomla-cms-bot
- | Category | ⇒ | Libraries |
Taking some inspiration from https://github.com/joomla-framework/cache/blob/2.0-dev/src/Adapter/File.php
- Make the
isSupportedmethod only return true, don't check config or anything like that (especially because the cache path is injectable in the handler's constructor, which may be different from the config path) - Check that the path exists and is readable in the handler's constructor, if not throw a
CacheConnectingException(this is in line with how we fail on the Memcached handler if the connection can't be created at runtime) - The same changes should be made to the Cache_Lite handler (it too is a filesystem cache API just using a third party PEAR library)
| Labels |
Added:
?
|
||
@mbabker Thanks for the comment. Deleting the method solves the issue too. Updated.
Beyond this, I'm not that knowledgeable, so this is as far as I can go with this. If you want to implement a different/more thorough solution, please go ahead.
But let's not take months for a simple fix, this is already better than before when it was breaking the site.
@HLeithner do you plan to merge this in J3 or is it for J4?
| Labels |
Removed:
J3 Issue
|
||
Done (I think. First time I resolve conflicts stuff)
| Status | Pending | ⇒ | Fixed in Code Base |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2019-04-29 14:30:41 |
| Closed_By | ⇒ | HLeithner |
thx
This is the wrong place to be doing this type of thing. A check for platform support shouldn't have side effects, in this case the side effect is creating the directory.
This type of directory creation is better suited in com_config when the global configuration is saved (which, we already do except for trying to (re-)create the core cache directory).
TBH, the more I think about this the more I think this particular
isSupportedmethod should just return true and leave it to runtime to raise issues if there are problems reading from or writing to the configured cache directory (we don't try to create the root cache path at runtime, which IMO should correctly be an error, but we do create the subdirectories as needed).