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

Laravel 5.4 Fixes with Tests #224

Merged
merged 7 commits into from
Jun 7, 2017
Merged

Conversation

CWDN
Copy link
Contributor

@CWDN CWDN commented Jan 30, 2017

I've updated the code and the tests to work with Laravel 5.4 and thus created breaking changes against previous versions. I wasn't sure what to do with your testing setup as there are multiple composer.json's for the different Laravel versions to be tested. But these don't seem to be needed anymore as the code changes I've done will break on previous Laravel versions.

I've had to create a proxy class for the ValidationRuleParser because its parse function is static and it didn't seem like you could mock static functions. I've done this to allow the DelegatedValidator to be tested. Annoyingly it will be the only class in your code base not being tested, so interested in what your thoughts are.

Thanks
Chris

@antonkomarev
Copy link
Contributor

@torrentalle Can you review this, please?

@antonkomarev
Copy link
Contributor

Not sure if it's working on older Laravel versions but it seems to work in 5.4

@austintoddj
Copy link

@torrentalle Would love to see this merged into the project soon! 🙏

@kyranb
Copy link

kyranb commented Jun 6, 2017

@torrentalle bumpity =)

@jampack
Copy link

jampack commented Jun 6, 2017

Been waiting since a week sadly 😞

@drjoju drjoju merged commit 092dad3 into proengsoft:master Jun 7, 2017
@drjoju
Copy link
Contributor

drjoju commented Jun 7, 2017

OK. The pull request is accepted.

@antonkomarev
Copy link
Contributor

Whoah! =) Thanks @drjoju
There is a requirement to make a release then.

@antonkomarev
Copy link
Contributor

antonkomarev commented Jun 7, 2017

@drjoju This PR breaks backward compatibility #242. So it should be released only in Major release or we should update the code to keep old Laravel versions support before release.

@kamrava
Copy link

kamrava commented Jun 7, 2017

This PR not working for me:

Type error: Argument 1 passed to Symfony\Component\HttpFoundation\Request::setSession() must be an instance of Symfony\Component\HttpFoundation\Session\SessionInterface, instance of Illuminate\Session\Store given, called in /var/www/vendor/proengsoft/laravel-jsvalidation/src/JsValidatorFactory.php on line 160 (View: /var/www/resources/views/user/drafts/new/sections/_foot.blade.php) (View: /var/www/resources/views/user/drafts/new/sections/_foot.blade.php)

I'm using Laravel 5.4.24

@jampack
Copy link

jampack commented Jun 7, 2017

@kamrava you need to update the package from stable version(1.5.0) to dev-master

@antonkomarev
Copy link
Contributor

antonkomarev commented Jun 7, 2017

@kamrava Release not published since it has breaking changes. Feel free to open PRs to return support of Laravel < 5.4 and then we will ready to publish stable release. #242 is show stopper issue.

@jampack
Copy link

jampack commented Jun 7, 2017

@a-komarev how come Laravel make such changes in a minor version that break everything backward?

@antonkomarev
Copy link
Contributor

antonkomarev commented Jun 7, 2017

@akkhan20 Laravel don't follow SemVer standard. Laravel 5.1 > 5.2 means something similar like 1.0 > 2.0 in SemVer.

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

7 participants