Add generic pipe filters #60
Conversation
Codecov Report
@@ Coverage Diff @@
## master #60 +/- ##
============================================
+ Coverage 73.16% 76.87% +3.70%
- Complexity 445 453 +8
============================================
Files 50 52 +2
Lines 1226 1241 +15
============================================
+ Hits 897 954 +57
+ Misses 329 287 -42
Continue to review full report at Codecov.
|
|
As discussed in markstory/asset_compress#344. |
src/Filter/PipeInputFilter.php
Outdated
| */ | ||
| protected $optionalDependencyPrefix = null; | ||
|
|
||
| public function getDependencies(\MiniAsset\AssetTarget $file) |
There was a problem hiding this comment.
Could this class be added to the use list? That would be more consistent with the rest of this project.
src/Filter/PipeInputFilter.php
Outdated
| public function getDependencies(\MiniAsset\AssetTarget $file) | ||
| { | ||
| if ($this->_settings['dependencies'] !== 'none' && $this->_settings['dependencies'] !== 'css') { | ||
| return false; |
There was a problem hiding this comment.
You should return [] here to be consistent with other implementations.
src/Filter/PipeInputFilter.php
Outdated
| if (substr($filename, strlen($this->_settings['ext']) * -1) !== $this->_settings['ext']) { | ||
| return $input; | ||
| } | ||
| $filename = preg_replace('/ /', '\\ ', $filename); |
There was a problem hiding this comment.
You could use escapeshellarg() instead.
src/Output/FreshTrait.php
Outdated
| $dependencies = $filter->getDependencies($target); | ||
| if ($dependencies === false) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
This feels like conflating concerns. Overloading the dependency detection to hand mtime overrides feels wrong. Could we add a new filter hook that allows filters to overrule the mtime value and only implement in the pipe filters.
Codecov Report
@@ Coverage Diff @@
## master #60 +/- ##
============================================
+ Coverage 73.16% 76.43% +3.26%
- Complexity 445 459 +14
============================================
Files 50 52 +2
Lines 1226 1256 +30
============================================
+ Hits 897 960 +63
+ Misses 329 296 -33
Continue to review full report at Codecov.
|
| if ($this->_settings['dependencies']) { | ||
| $this->optionalDependencyPrefix = $this->_settings['optional_dependency_prefix']; | ||
|
|
||
| return $this->getCssDependencies($file); |
There was a problem hiding this comment.
How do you know that the file being processed is CSS?
There was a problem hiding this comment.
When the dependencies setting is true the CSS-like dependencies are used. I was previously using a text setting in case there were more dependency types added later but I've decided to simplify. It should be set to false when there's no CSS-like imports.
There was a problem hiding this comment.
Wouldn't a safer default be false then? In the docblock for the settings/filter the usage could be explained.
There was a problem hiding this comment.
Wouldn't a safer default be
falsethen? In the docblock for the settings/filter the usage could be explained.
I think I did it because the default extension is .css. Maybe I should change that too.
There was a problem hiding this comment.
@markstory, please, review the settings changes in the new commit.
There's an input and an output filter that can be configured to execute an external command passing the file as a parameter. I've called them pipes although I'm not using system pipes.
In the input filter, the
dependenciessetting changes how dependencies are handled. We can choose not having dependencies calculated but use the source file timestamp (none), use CSS dependency calculation (css), or always run the filter (other).