HTMLMin / Laravel-HTMLMin

A simple HTML minifier for Laravel 5, 6, 7, 8 & 9.
MIT License
1.02k stars 119 forks source link

[2.0] Blade View Minification Broken #11

Closed AntoineAugusti closed 10 years ago

AntoineAugusti commented 10 years ago

Hi,

Is it possible to cache blade views, just after they have been minified? I don't want to minify each view on the fly.

GrahamCampbell commented 10 years ago

That is exactly what happens already. The views are minified when the blade in compiler is fired, and then they are saved to the disk in the minified state.

AntoineAugusti commented 10 years ago

Okay, I'm missing something. I have added the ServiceProvider and set blade to true. My views are not compiled as minified, do I need to do something else?

AntoineAugusti commented 10 years ago

I'm using the develop version with Laravel 4.2.1

GrahamCampbell commented 10 years ago

Are your views .blade.php files?

AntoineAugusti commented 10 years ago

Yes, they are all .blade.php On 7 Jun 2014 20:30, "Graham Campbell" notifications@github.com wrote:

Are your views .blade.php files?

— Reply to this email directly or view it on GitHub https://github.com/GrahamCampbell/Laravel-HTMLMin/issues/11#issuecomment-45417566 .

GrahamCampbell commented 10 years ago

Have you registered the service provider? Do you have any other packages that could be overriding the blade compiler?

GrahamCampbell commented 10 years ago

Hmmm. I'll pull down a fresh laravel install and see if I can replicate this.

AntoineAugusti commented 10 years ago

The enableBladeOptimisationsfunction is called but the function compileMinify from the MinifyCompiler is never called. My app/storage/views directory is empty.

GrahamCampbell commented 10 years ago

I think I know what the issue is. The ironic thing is this was broken by a change I made to the laravel core...

AntoineAugusti commented 10 years ago

Gotcha, that's not a common thing!

GrahamCampbell commented 10 years ago

I'll patch it in this package for now, and I'll submit a pull to laravel to revert to fix this properly.

AntoineAugusti commented 10 years ago

I'm not using this package for now, you don't need do a quick fix here if you plan to submit a PR on the Laravel framework. I definitely can wait a few weeks!

AntoineAugusti commented 10 years ago

Just for the information, which PR caused this bug?

GrahamCampbell commented 10 years ago

This was very noisy. It took 5 issues with a tonne of comments on each. It wasn't helped by the trolling of a certain somebody too. https://github.com/laravel/framework/pull/3690

AntoineAugusti commented 10 years ago

Just saw your new PR https://github.com/laravel/framework/pull/4605, seems legit to not hardcode the array. It was a very convenient way indeed to add a compiler! Let's hope it will be merged shortly.

GrahamCampbell commented 10 years ago

Yep, that won't actually fix this issue though due to the changes I made in that original pull. I'm working on a fix for this package now...

GrahamCampbell commented 10 years ago

Ahhh. I see the issue now. I wasn't registering the engine correctly. I changed the way I did it on the develop branch, and I see now why my new way doesn't work. I'm reverting it now...

GrahamCampbell commented 10 years ago

See cf0753d1410b3295c2d158130e6e2f39819c0288 and f4be76c8a2719eec6e9e4c3aebd68308e6c45d5c.

AntoineAugusti commented 10 years ago

Just tested it with my app, it is working: my views are compiled as minified, as expected!

GrahamCampbell commented 10 years ago

That pull was merged into laravel. As soon as Laravel 4.2.2 is released, I'll revert f4be76c8a2719eec6e9e4c3aebd68308e6c45d5c.

AntoineAugusti commented 10 years ago

Perfect! Looking forward to see a stable release of HTMLMin for Laravel 4.2.*

GrahamCampbell commented 10 years ago

Laravel 4.2.2 has been released now: laravel/framework/releases/v4.2.2. I have reverted that workaround now: d12424b711634a2436d6833eaa0cccadbaeca983. If the tests pass, I'll close this. See: builds/27332079.

GrahamCampbell commented 10 years ago

Everything looks good from where I'm standing. Feel free to ping me if I've messed something up!

andrewdworn commented 10 years ago

Hi Graham! After the unpleasant debate that my yesterday comment provoked, I checked your project and decided to give it a try. I seem to have a similar issue described here. The extension works in live mode, but doesn't work when only blade config option is enabled. I've just upgraded to Laravel 4.2.6, and followed the install instructions.

The HTMLMin::html($value) function also works! Although, my html code gets 'divided' in strange places, for example:

...<ul
class="pipe"><li>...
Any idea?
GrahamCampbell commented 10 years ago

@andrewdworn Don't worry about that "debate", it's not your fault at all.

So your issue is that you have enabled blade minification, but the blade files are not being minified?

I don't think this is a repeat of the same issue described here because I fixed that in this package, and in the Laravel core. I assume this issue is caused by the following scenario: 1) You have a Laravel 4.2 project and you've been using it before you installed my package. 2) You installed my package (~2.0@dev) and registered the service provider correctly. 3) You enabled blade minification, but for some reason you observe no change.

Let me explain why this happens. Before you installed this package, Laravel compiled your views and stored then in "app/storage/views". Laravel will only re-compile a view if it has been modified. You now enable blade minification, but the old compiled view files still exist, and Laravel sees that the views haven't changed, so it doesn't re-compile then.

What you need to do to get Laravel to re-compile the views is to open up "app/storage/views" and delete all compiled views. Now Laravel will re-compile the views as you need them, and they should be minified.

Let me know how you get on, and give me a buzz if you've got any further questions.

andrewdworn commented 10 years ago

Thanks for the quick answer! I have my "app/storage/views" dir open in mc all the time, and I clean it before expecting any change in my page, so it must be something else...

GrahamCampbell commented 10 years ago

I'll create a new issue for this so we can discuss it there...

GrahamCampbell commented 10 years ago

@andrewdworn This issue can be continued on #15.