SymfonyCasts / sass-bundle

Delightful Sass Support for Symfony + AssetMapper
https://symfony.com/bundles/SassBundle/current/index.html
MIT License
37 stars 18 forks source link

Incorrect Sass binary path construction results in repetitive download of the binary #66

Open FlyingDR opened 7 months ago

FlyingDR commented 7 months ago

The current implementation of the construction of the path for Sass binary relies on the expectation that the main entry point is always named sass:

https://github.com/SymfonyCasts/sass-bundle/blob/aa47da8ae43136a21a764eedb1471f2a168359cb/src/SassBinary.php#L162-L165

But this is not true for Windows, where it is a sass.bat. Effectively it results in a re-download of the Sass distributive for each build due to this logic:

https://github.com/SymfonyCasts/sass-bundle/blob/aa47da8ae43136a21a764eedb1471f2a168359cb/src/SassBinary.php#L37-L40

Correct implementation of the getDefaultBinaryPath method would be:

private function getDefaultBinaryPath(): string
{
    return $this->binaryDownloadDir.'/dart-sass/sass'.(str_contains(strtolower(\PHP_OS), 'win') ? '.bat' : '');
}

There is also one more related and Windows-specific issue:

Normally after download bundle tests that download results in the appearance of the Sass binary and throws an exception if it is not so:

https://github.com/SymfonyCasts/sass-bundle/blob/aa47da8ae43136a21a764eedb1471f2a168359cb/src/SassBinary.php#L109-L112

However, for Windows, no such check is applied because there is a separate branch for archive extraction that ends with unconditional return.

https://github.com/SymfonyCasts/sass-bundle/blob/aa47da8ae43136a21a764eedb1471f2a168359cb/src/SassBinary.php#L87-L97

Because of this, in case if Sass binary download will not cause the Sass compiler to actually appear in the filesystem it will not be reported.

bocharsky-bw commented 7 months ago

Hey @FlyingDR ,

Thank you for reporting these Windows-specific bugs! Would you like to create PRs for them? It would be much appreciated.