Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[5.6] Get Blade compiler from Engine Resolver #23710

Merged
merged 1 commit into from Mar 27, 2018
Merged

[5.6] Get Blade compiler from Engine Resolver #23710

merged 1 commit into from Mar 27, 2018

Conversation

rodrigopedra
Copy link
Contributor

If one registers a custom Blade compiler ( such as https://github.com/HTMLMin/Laravel-HTMLMin ) that replaces the built-in compiler, all Blade directives registration through the Blade Façade are registered with the custom compiler and not the built-in one.

As the view:cache command didn't retrieve the compiler from the EngineResolver but used the built-in one, when a user that uses a custom compiler runs this command, no custom directive is compiled.

This commit fixes this by retrieving the compiler from the EngineResolver, so there are no conflicts with packages that register a custom Blade compiler.

(For reference see the Blade Façade: https://github.com/laravel/framework/blob/5.6/src/Illuminate/Support/Facades/Blade.php#L34 )

If one registers a custom Blade compiler ( such as https://github.com/HTMLMin/Laravel-HTMLMin ) that replaces the built-in compiler, all Blade directives registration through the Blade Façade are registered with the custom compiler and not the built-in one.

As the `view:cache` command didn't retrieve the compiler from the EngineResolver but used the built-in one, when a user that uses a custom compiler runs this command, no custom directive is compiled.

This commit fixes this by retrieving the compiler from the EngineResolver, so there are no conflicts with packages that register a custom Blade compiler.

(For reference see the Blade Façade: https://github.com/laravel/framework/blob/5.6/src/Illuminate/Support/Facades/Blade.php#L34 )
@rodrigopedra rodrigopedra changed the title Get Blade compiler from Engine Resolver [5.6] Get Blade compiler from Engine Resolver Mar 27, 2018
@sisve
Copy link
Contributor

sisve commented Mar 27, 2018

Is this something that would need a test?

@rodrigopedra
Copy link
Contributor Author

rodrigopedra commented Mar 27, 2018

I thought about it but have no clue how to test it...

One idea I had for testing is registering a custom directive through the Blade Façade, run the view:cache command and check if the directive was compiled as expected. But I had doubts if this would suffice. I don't have a lot of experience with testing.

Do you think this idea this would work?

@deleugpn
Copy link
Contributor

That would make a pretty good feature test.

@rodrigopedra
Copy link
Contributor Author

That is what I thought, but I was afraid it would be a large feature test to test something that only go wrong if you extend the framework... if you use it as is, it won’t break.

If you think it is a must, I can try to write a test for this, but I wiil only be able to do so later at night.

But as I don’t have a lot of experience in testing I probably will need help reviewing my attempt, thanks in advance for any help.

@taylorotwell taylorotwell merged commit b1af528 into laravel:5.6 Mar 27, 2018
@rodrigopedra
Copy link
Contributor Author

Hi @taylorotwell , thanks for merging this, it took me a couple hours to figure out what was happening with my project.

Do you think a test testing the extend scenario is needed? I can try to figure out how to do it and send a pull request for comments.

@rodrigopedra rodrigopedra deleted the patch-1 branch March 28, 2018 00:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants