add scss support via leafo/scssphp #549
Conversation
the functionality we are using is present
this is needed for @import to be able to resolve included files
tested cache dependencies
| * | ||
| * @link https://github.com/leafo/scssphp/ | ||
| */ | ||
| class Minify_ScssCssSource extends Minify_Source { |
There was a problem hiding this comment.
Should it be Minify_ScssCssSource or Minify_ScssSource? If Minify_ScssCssSource, shouldn't it extend Minify_CssSource?
There was a problem hiding this comment.
I think they both should rather depend on new Base Source class or the support that these classes have added to Source class. i.e some kind of cleanup/refactoring (in separate PR)
| * | ||
| * @var array | ||
| */ | ||
| private $parsed; |
There was a problem hiding this comment.
If the class isn't final, Why have private properties in favor of protected ones?
| } | ||
|
|
||
| /** | ||
| * Make a unique cache id for for this source. |
There was a problem hiding this comment.
"unique" amounts to common string + md5 hash these days?
Either provide some actual uniqueness (via UUID or similar) or drop the word unique
| } | ||
| } | ||
|
|
||
| $input = $cache ? $cache : $this->filepath; |
There was a problem hiding this comment.
Lines 90 - 96 look like something that belongs inside a cache adapter
| } | ||
| } | ||
|
|
||
| return false; |
There was a problem hiding this comment.
from the implementation of this method, if $cache is an array with at least one value OR an instance of ArrayAccess , it isn't considered "stale", regardless of contents (as long as there's no 'files' index).
There was a problem hiding this comment.
The code assumes the cache is not corrupted. so there is always 'files' key present with at least one entry.
| * | ||
| * @return boolean True if compile required. | ||
| */ | ||
| public function cacheIsStale($cache) |
There was a problem hiding this comment.
Why the public visibility all of a sudden, if most of the rest of this class seems to be private?
There was a problem hiding this comment.
noticed this myself too. will fix.
| $scss->setImportPaths(dirname($filename)); | ||
|
|
||
| $css = $scss->compile(file_get_contents($filename), $filename); | ||
| $elapsed = round((microtime(true) - $start), 4); |
There was a problem hiding this comment.
STYLE: seems that half of this method is shifted by one space, here's where the shift starts
| $mtime1 = filemtime($mainLess); | ||
| var_dump($mtime1); | ||
| $mtime2 = filemtime($includedLess); | ||
| var_dump($mtime2); |
There was a problem hiding this comment.
Why does test include var_dumps ?
There was a problem hiding this comment.
will remove from here and other tests after PR being merged.
| @@ -34,6 +34,7 @@ | |||
| }, | |||
| "require-dev": { | |||
| "leafo/lessphp": "~0.4.0", | |||
| "leafo/scssphp": "^0.3.0", | |||
There was a problem hiding this comment.
I find "~0.3" a bit more expressive than this.
This gives off the notion of a very specific version, but actually, it's ">= 0.3.0 <1.0.0" (at least since composer 1.0.0 ) https://getcomposer.org/doc/articles/versions.md#caret
There was a problem hiding this comment.
I assume the library is semver compliant and pick 0.3 for php 5.3 and 0.6 for php 5.4+
|
@mrclay i would like to get this merged. the PR is ready for merge. |
Implemented by same design as lib/Minify/LessCssSource.php