We are in the process of migrating away from the CMS filesystem package to use the framework version of it. This PR converts all instances where File::makesafe() is used to use the framework version. In that process, all other calls to the CMS File class in that file were converted to properly handle possible exceptions from the framework equivalent. This intentionally did not add additional error handling. I expect that to be a separate effort in the future.
Testing Instructions
Codereview.
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
I have tested this item ? unsuccessfully on f33810e
OK System Tests (excluding installation step):
I ran the system tests both before and after applying the PR. All specifications passed in both runs.
Code validation failed: should exceptions be caught without any action? That may be fine in LocalAdapter.php:859 when deleting a TMP file fails, but even there it's not good practice to comment why the exception is ignored? and why is the exception ignored in LocalAdapter.php:306?
Failed: Since no tests are defined. Isn't the code review meant for theory and needs to be checked in practice? If test cases are added, I would test again.
* ✅ php-cs-fixer phpcs unit lint:css lint:js lint:testjs system
passed successfully with actual 5.3-dev branch
and passed successfully after applying this PR with Patch Tester
(have to delete 'automated test feed' feed in between, but this is another story of my "With every test, I discover an additional error." series -> setting on my TODO list)
✅ Checked that all files using File::makeSafe are included in this PR.
✅ Code Review
Visited all File:: usages in the five modified files
As it is stated "intentionally did not add additional error handling", there are some exceptions being caught away; could they be commented? e.g.
// TODO: Switched from CMS filesystem package to use the framework version, please implement me
🟡 I tried to use the code (by checking with Xdebug), but I could only run into MediaHelper.php by uploading an image. It would be helpful to run something more ...
🟡 I am not a PHP developer, so it would be helpful if the second test is reviewed by someone with PHP expertise.
joomla-cms-bot
- 
missing test instructions, you change the template and the media manager plugins and more at the same time.