Elhebert / laravel-sri

Subresource Integrity hash generator for laravel
MIT License
40 stars 16 forks source link

Feature/add attributes to blade directives #56

Closed juliomotol closed 4 years ago

juliomotol commented 4 years ago

This PR aims to improve the current blade directives available with the following notable changes:

This PR also adds tests for the blade directives which there wasn't any before.

juliomotol commented 4 years ago

I just saw PR https://github.com/Elhebert/laravel-sri/pull/37 and I think this is a cleaner solution since $attributes is more general than $execute but the idea is pretty much the same.

Elhebert commented 4 years ago

Nice PR, I'll have a deeper look later this week.

I think that I might remove the Blade directive soon to use Blade component that will provide a far better DX 🤔

juliomotol commented 4 years ago

Interesting! On v3 maybe?

I also noticed that you're setting an alias for Sri::class with sri in lowercase. Normally, aliases are pointed to the facade and is CamelCased.

https://github.com/Elhebert/laravel-sri/blob/6df0ce2b5f9f19f08b9bbd40e9b1015cbef842ca/src/SriServiceProvider.php#L18


$this->app->alias(SriFacade::class, 'Sri');
Elhebert commented 4 years ago

I think I will first do a new minor with the components, then remove them in a major version (so yeah 3.0). It's a bit hard for me to work on this package as I'm currently not working with Laravel anymore. But I do intent to do a full rewrite for a v3 at some point.

For the Alias I think we can remove the one in the service provider because I set one in the composer.json:

https://github.com/Elhebert/laravel-sri/blob/master/composer.json#L42-L44

juliomotol commented 4 years ago

I see. I also have a few concerns that could be implemented in v3 if you are open for an RFC.

Elhebert commented 4 years ago

I'll create an issue for v3 improvements.

See #57.