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

Create "cache" folder if it doesn't exist before trying to write to it #21952

Merged
merged 4 commits into from Apr 29, 2019

Conversation

Projects
None yet
6 participants
@AndySDH
Copy link
Contributor

commented Sep 1, 2018

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.

Create "cache" folder if it doesn't exist before trying to write to it
This fixes this issue:

### Steps to reproduce the issue

- Have cache enabled
- Delete cache folder

### Expected result

- Joomla creates "cache" folder if it doesn't exist

### Actual result

- Sites breaks. Goes blank.

### 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.

@AndySDH AndySDH referenced this pull request Sep 1, 2018

Closed

created PR #21951

if (!is_dir($folder))
{
@mkdir($folder);
}

This comment has been minimized.

Copy link
@brianteeman

brianteeman Sep 1, 2018

Contributor

Don't we need some code then if for any reason joomla cannot create the folder

@mbabker

This comment has been minimized.

Copy link
Member

commented Sep 1, 2018

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 isSupported method 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).

@mbabker

This comment has been minimized.

Copy link
Member

commented Sep 1, 2018

Taking some inspiration from https://github.com/joomla-framework/cache/blob/2.0-dev/src/Adapter/File.php

  1. Make the isSupported method 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)
  2. 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)
  3. 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)
Update FileStorage.php
By removing the isSupported method from that file, it will fall back on its parent isSupported method, which simply returns true.
@AndySDH

This comment has been minimized.

Copy link
Contributor Author

commented Sep 1, 2018

@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.

zwiastunsw pushed a commit to zwiastunsw/joomla-cms that referenced this pull request Sep 2, 2018

[4.0] Workflow - list and edit (joomla#21795)
* [4.0] Workflow - list and edit

I can't see any point in having the author or creation/modified dates. We dont have them for stages/transitions and unless I have missed something they are not needed.

So this PR simplifies the edit form (inline with joomla#21952) and updates the list view accordingly

NOTE I removed the manage text link as its really a duplicate of the other stages link that has the number in it - although that might need tweaking in the UI - as you should not have two links to do the same thing for a11y

* typo - thanks @alikon
@franz-wohlkoenig

This comment has been minimized.

Copy link
Member

commented Apr 28, 2019

@HLeithner do you plan to merge this in J3 or is it for J4?

@HLeithner

This comment has been minimized.

Copy link
Member

commented Apr 29, 2019

I will merge it. @AndySDH please resolve the conflict

@AndySDH

This comment has been minimized.

Copy link
Contributor Author

commented Apr 29, 2019

Done (I think. First time I resolve conflicts stuff)

@HLeithner HLeithner merged commit d678266 into joomla:staging Apr 29, 2019

4 checks passed

Hound No violations found. Woof!
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/drone/pr the build was successful
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@HLeithner

This comment has been minimized.

Copy link
Member

commented Apr 29, 2019

thx

@HLeithner HLeithner added this to the Joomla 3.9.6 milestone Apr 29, 2019

tecpromotion added a commit to tecpromotion/joomla-cms that referenced this pull request May 23, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.