Use Symfony/filesystem #1601
Use Symfony/filesystem #1601
Conversation
3bdb5c1 to
1f907fa
Compare
Codecov Report
@@ Coverage Diff @@
## master #1601 +/- ##
============================================
+ Coverage 42.33% 42.36% +0.02%
+ Complexity 3457 3449 -8
============================================
Files 143 143
Lines 10029 10039 +10
============================================
+ Hits 4246 4253 +7
- Misses 5783 5786 +3 |
| $string = str_replace($formats, $replacements, $string); | ||
| file_put_contents($this->logFile, $string . \PHP_EOL, FILE_APPEND); | ||
| if (preg_match('/^php:\/\//', $this->logFile)) { | ||
| // Dirty hack to get unit tests for Windows working.. Symfony doesn't deal well with them. |
There was a problem hiding this comment.
There was a problem hiding this comment.
Because I didn't get a response, I've opened an issue for this: symfony/symfony#45749
0466064 to
8b7736f
Compare
|
Still have to test this, but a first review wouldn't hurt |
There was a problem hiding this comment.
Looks good in principle. It does save some code we can reuse from Symfony. I would think to depend as much as possible on this code: one advantage of changing this is that we can use the error handling that the Symfony functions provide over the builtin php functions. So I'd tend to 'just call' whatever operation is what we want to do and trust that symfony/filesystem will throw the appropriate excetion e.g. on unreadability.
| try { | ||
| $this->fileSystem->mkdir($loc, 0777); | ||
| } catch (IOException $e) { | ||
| Logger::error('Failed to create directory ' . $loc . ': ' . $e->getMessage()); |
There was a problem hiding this comment.
If this creates an error we can also just use exception mkdir will generate when we don't put it in a try/catch block? Same in instances below.
There was a problem hiding this comment.
In this file, we catch the error and return either null or false.. If we don't do that, the exception will break execution. I didn't want to change the original behaviour.
There was a problem hiding this comment.
I don't follow: Logger:error will break execution, also?
There was a problem hiding this comment.
Now I don't follow.. Logger::error only logs something.. The line underneath false is returned and the caller can handle that... If we don't catch the IOException, the caller never regains control.. We can let the caller catch it if you like that better?
There was a problem hiding this comment.
This explains the confusion. I was under the impression that an error halts execution. While it apparently doesn't.
| try { | ||
| $this->fileSystem->mkdir($loc, 0777); | ||
| } catch (IOException $e) { | ||
| Logger::error('Failed to create directory ' . $loc . ': ' . $e->getMessage()); |
There was a problem hiding this comment.
If this creates an error we can also just use exception mkdir will generate when we don't put it in a try/catch block?
Instead of making our lives difficult, we should be using the already available
symfony/filesystem.Added bonus is that these classes can be DI'ed for unit testing purposes.
Note: I haven't field-tested this change yet and we may want to add more cases.