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

Move directive initialisation to the boot method #24

Merged
merged 1 commit into from
Mar 6, 2019

Conversation

owenvoke
Copy link
Contributor

@owenvoke owenvoke commented Mar 6, 2019

As per the conversation in #23 it seems that the Blade::directive() usage in the register() method isn't initialised. In Laravel 5.8, the facadeAccessor() for the Blade facade now returns the accessor string instead of an instance like in previous versions (see laravel/framework#25497).

According to the documentation, using facades in the register method isn't recommended as they may not have been defined yet, so they should be used in the boot method, which occurs after all register methods have finished.

I've tested this in Laravel 5.5, 5.6 and 5.8. Also, @HDVinnie has tested it on dev servers using Laravel 5.7 and 5.8 as per #23 (comment).

The PHPUnit tests seem to work as expected on Travis and locally, but @Elhebert, please feel free to test this.

Fixes #23

@Elhebert
Copy link
Owner

Elhebert commented Mar 6, 2019

Nice catch!

Thx for the fix.

@Elhebert Elhebert merged commit f2c9ee7 into Elhebert:master Mar 6, 2019
@owenvoke owenvoke deleted the bugfix/5.8-blade-directives branch March 6, 2019 15:15
@owenvoke
Copy link
Contributor Author

owenvoke commented Mar 6, 2019

Cheers! I guess this can be a v1.5.1 whenever. 👍

@Elhebert
Copy link
Owner

Elhebert commented Mar 6, 2019

I'm on it as soon as I'm on my laptop.

@Elhebert
Copy link
Owner

Elhebert commented Mar 6, 2019

Done ✅

@HDVinnie
Copy link
Contributor

HDVinnie commented Mar 6, 2019

Thanks @Elhebert @pxgamer Great Package! 💯

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.

3 participants