WyriHaximus / HtmlCompress

MIT License
78 stars 17 forks source link

Patterns::add method + refactor Pattern and HtmlCompressor #87

Closed abuyoyo closed 4 years ago

abuyoyo commented 4 years ago

This pr addresses issue #86.

I've consolidated both HtmlCompressor classes into one. And consolidated Patterns to be a direct implementation of HtmlMinObserverInterface.

Added methods compressor and patterns to HtmlCompressor. These provide references to HtmlMin instance and Patterns instance (respectively). Added method add to Patterns to allow adding custom PatternInterface implementations.

abuyoyo commented 4 years ago

resolves #86

abuyoyo commented 4 years ago

I've updated the HtmlCompressor class reference in HtmlCompressorTest.php That fixed all the errors. I still get a failed test, tho I can't see how that has to do with the code introduced in this pr. I'm running on a Windows machine - so maybe that has something to do with it.

1) WyriHaximus\HtmlCompress\Tests\EdgeCasesTest::testEdgeCase with data set "asciinema" ('\Html...inema\')
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-</code></pre> <script async id=asciicast-18487 src=https://asciinema.org/a/18487.js></script>'
+</code></pre>
+
+<script type="text/javascript" src="https://asciinema.org/a/18487.js" id="asciicast-18487" async></script>'

\HtmlCompress\tests\EdgeCasesTest.php:45

FAILURES!
Tests: 56, Assertions: 78, Failures: 1.
abuyoyo commented 4 years ago

About the code styling/standards. I've run nmake from the Developer Command Prompt for VS on my machine (which AFAIK is the closest i can get to running make on Windows). It produces fatal errors that are beyond my current knowledge on how to fix.

> composer validate --ansi
Fatal Error Base address marks unusable memory region. Please setup opcache.file_cache and opcache.file_cache_fallback directives for more convenient Opcache usage
Script composer validate --ansi handling the qa-all event returned with error code -2
NMAKE : fatal error etc. etc.

Setting opcache.file_cache and opcache.file_cache_fallback in php.ini did not fix the errors. So I don't know how to proceed.

WyriHaximus commented 4 years ago

About the code styling/standards. I've run nmake from the Developer Command Prompt for VS on my machine (which AFAIK is the closest i can get to running make on Windows). It produces fatal errors that are beyond my current knowledge on how to fix.

> composer validate --ansi
Fatal Error Base address marks unusable memory region. Please setup opcache.file_cache and opcache.file_cache_fallback directives for more convenient Opcache usage
Script composer validate --ansi handling the qa-all event returned with error code -2
NMAKE : fatal error etc. etc.

Setting opcache.file_cache and opcache.file_cache_fallback in php.ini did not fix the errors. So I don't know how to proceed.

If you're interested, have a look at WSL, using it on my gaming rig and it basically runs an actual Linux kernel and lets you run Linux on Windows: https://en.wikipedia.org/wiki/Windows_Subsystem_for_Linux

No worries, I'll fix them for you 👍

WyriHaximus commented 4 years ago

I've updated the HtmlCompressor class reference in HtmlCompressorTest.php That fixed all the errors. I still get a failed test, tho I can't see how that has to do with the code introduced in this pr. I'm running on a Windows machine - so maybe that has something to do with it.

1) WyriHaximus\HtmlCompress\Tests\EdgeCasesTest::testEdgeCase with data set "asciinema" ('\Html...inema\')
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-</code></pre> <script async id=asciicast-18487 src=https://asciinema.org/a/18487.js></script>'
+</code></pre>
+
+<script type="text/javascript" src="https://asciinema.org/a/18487.js" id="asciicast-18487" async></script>'

\HtmlCompress\tests\EdgeCasesTest.php:45

FAILURES!
Tests: 56, Assertions: 78, Failures: 1.

Interestingly enough it runs just fine on Travis

abuyoyo commented 4 years ago

I'm running Widows 7 - so no WSL on my current machine anytime soon :(

My guess about the failed test is one of the upstream libraries not replacing Windows \r\n end-of-line properly.

abuyoyo commented 4 years ago

Not sure how I feel about how you proposed to change HtmlMin settings but we can iterate over that later

Do you have any ideas in mind about the API for accessing the HtmlMin settings? I'd be willing to write a PR for this if it might expedite the next major release.

WyriHaximus commented 4 years ago

Not sure how I feel about how you proposed to change HtmlMin settings but we can iterate over that later

Do you have any ideas in mind about the API for accessing the HtmlMin settings? I'd be willing to write a PR for this if it might expedite the next major release.

Leaning towards an immutable way of settings those settings. But don't have an API in mind yet

FYI the next major release will be PHP 7.4+ https://github.com/WyriHaximus/HtmlCompress/pull/75

abuyoyo commented 4 years ago

Leaning towards an immutable way of settings those settings.

That would mean a lot of upkeep, no? You'd need to release a new version whenever HtmlMin changes/adds settings. I see how having an immutable API would be cleaner. My 2-cents on this would be to still allow direct access to the HtmlMin instance to allow for edge-cases that might not be covered.

WyriHaximus commented 4 years ago

Leaning towards an immutable way of settings those settings.

That would mean a lot of upkeep, no? You'd need to release a new version whenever HtmlMin changes/adds settings. I see how having an immutable API would be cleaner. My 2-cents on this would be to still allow direct access to the HtmlMin instance to allow for edge-cases that might not be covered.

Yup that's the potential issue, maybe that could be solved with a Github Actions that watches it. Not having an issue with some extra upkeep tbh, it's possibly missing a new settings that worries me