[#40113] - [4.4] Converting from File::exists() to is_file()
- Fixed in Code Base
- 15 Mar 2023
- Medium
- Build: 4.4-dev
- # 40113
- Diff
- Hackwar:4.4-filesystem-file
- Pending continuous-integration/drone/pr Build is pending Details
User tests: Successful: Unsuccessful:
Summary of Changes
In the process of deprecating the CMS Filesystem package, this PR replaces all cases of File::exists() with is_file(). File::exists() is a wrapper for is_file(Path::clean($path)), so by removing it, we are removing the Path::clean($path). However, as far as I can see, we either have a safely secure path anyway or it is already wrapped with Path::clean(). Where it isn't wrapped, it actually is hardly usefull, because we do a File::exists($path) and shortly after use the original value of $path to include that file. Long story short, I think this is a safe change.
I still would like the @joomla/security-leads to have a look at this.
Testing Instructions
After applying this, nothing should have changed. Thus, a codereview would probably make the most sense here.
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
joomla-cms-bot
- | Category | ⇒ | Administration com_admin com_config com_content com_contenthistory com_installer com_joomlaupdate com_postinstall com_templates Front End Installation Libraries Plugins |
| Status | New | ⇒ | Pending |
| Labels |
Added:
PR-4.4-dev
|
||
Are there any occurrences of File::exists left in the core? If not, then I would add the deprecation message right into this pr.
No, all occurences have been handled with this. But I would really like to keep the deprecation messages in one PR, among other things because it means I only have to make one PR to the manual repository. If you want me to make 40 PRs to deprecate each method individually, then I would rather defer all that work to you.
You can make one pr in the manual and then reference it from each of these subsequent pr's. Would that be ok?
Not really, because it would mean that I would have to update #40111 several times for each case where a method was deprecated individually. I also would not simply close #40111, because there are several methods and whole classes which we aren't using at all in core. And it means dozens of PRs poluting our history with something being deprecated here and then something there. And if we don't get all existing code converted in time for 4.4, we wouldn't be able to remove it all in 6.0, just because deprecation is supposed to be done in time with migration of the core. I'm not saying that I wouldn't be able to write all that code in time, but considering that I'm waiting for 2.5 years now for people to test the install progress bar, I have zero confidence in that necessary PRs would be merged in time.
Ok, then leave it as it is.
I still would like the https://github.com/orgs/joomla/teams/security-leads to have a look at this.
Looks ok from the security side of things
All changes have been applied. Ready to test/codereview/merge.
| Status | Pending | ⇒ | Fixed in Code Base |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2023-03-15 16:25:22 |
| Closed_By | ⇒ | laoneo |
Thanks!
I admit, I did not check these cases, but only checked when the variable was last written to.