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

Fix blade directives #144

Merged
merged 3 commits into from
Apr 10, 2021
Merged

Conversation

mikemand
Copy link
Contributor

@mikemand mikemand commented Jan 26, 2021

#125 added the ability to add nonces to the scripts, but the implementation was not correct.

Blade passes all the arguments as a string (for example: "'en', 'nonce-ASDFGHJKL'") to the closure. After Blade compiles the view, the underlying function (app('captcha')->renderFooterJS(...)) receives the arguments as they were passed.

This can be further simplified by using array destructuring, which means dropping support for PHP 5.6 and 7.0.

Signed-off-by: Micheal Mand <micheal@kmdwebdesigns.com>

albertcht#125 added the ability to add nonces to the scripts, but the implementation was not correct
Signed-off-by: Micheal Mand <micheal@kmdwebdesigns.com>
@mikemand mikemand marked this pull request as draft January 26, 2021 17:54
@mikemand
Copy link
Contributor Author

I started writing tests and realized I was partially correct, and fatally incorrect at the same time. Fixing now.

Signed-off-by: Micheal Mand <micheal@kmdwebdesigns.com>
@mikemand mikemand marked this pull request as ready for review January 26, 2021 18:04
@mikemand
Copy link
Contributor Author

@albertcht How do you feel about dropping support for anything older than Laravel 7 (preferably Laravel 8, but I will settle for 7) and PHP 7.3?

@albertcht
Copy link
Owner

@albertcht How do you feel about dropping support for anything older than Laravel 7 (preferably Laravel 8, but I will settle for 7) and PHP 7.3?

I would personally prefer keeping support for older Laravel versions and PHP 5.6.

@mikemand
Copy link
Contributor Author

I would personally prefer keeping support for older Laravel versions and PHP 5.6.

Ok, that's fine. Have you had a chance to review this PR and the problem it fixes? The exception I was receiving is:
ArgumentCountError Too few arguments to function AlbertCht\InvisibleReCaptcha\InvisibleReCaptchaServiceProvider::AlbertCht\InvisibleReCaptcha\{closure}(), 1 passed and exactly 2 expected vendor/albertcht/invisible-recaptcha/src/InvisibleReCaptchaServiceProvider.php:84

@albertcht
Copy link
Owner

I would personally prefer keeping support for older Laravel versions and PHP 5.6.

Ok, that's fine. Have you had a chance to review this PR and the problem it fixes? The exception I was receiving is:
ArgumentCountError Too few arguments to function AlbertCht\InvisibleReCaptcha\InvisibleReCaptchaServiceProvider::AlbertCht\InvisibleReCaptcha\{closure}(), 1 passed and exactly 2 expected vendor/albertcht/invisible-recaptcha/src/InvisibleReCaptchaServiceProvider.php:84

Thanks for your pull request, I will review this PR as soon as possible.

@albertcht albertcht merged commit edb97f9 into albertcht:master Apr 10, 2021
@mikemand mikemand deleted the fix-blade-directives branch April 12, 2021 00:03
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.

2 participants