-
Notifications
You must be signed in to change notification settings - Fork 73
⬆️ draft support for laravel 6.* #53
base: master
Are you sure you want to change the base?
Conversation
fec06d3
to
b7e4ae7
Compare
Signed-off-by: Wim <wim@join.marketing>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing really has changed. Are there any breaking changes so far?
@@ -63,7 +63,7 @@ directory. The most logical place to start are the [docs for the `HasEncryptedAt | |||
|
|||
## Requirements | |||
|
|||
* Laravel: 5.5, 5.6, 5.7, or 5.8 | |||
* Laravel: 5.5, 5.6, 5.7, 5.8, or 6.x | |||
* PHP: 7.1, 7.2, or 7.3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since 7.1 is already deprecated and 7.2 is in security-only, maybe also look into PHP 7.4 support (add allow-failure
CI tests for example)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, That's good idea. For now I was just looking into my own case where i needed Laravel 6 and php 7.3 support.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that should be a separate PR. There will probably be some issues.
Co-Authored-By: Roelof <github@roelof.io>
Thats right, everything seems to work fine so far, it's just the CI tests that i didn't get to succeed, because of the above dependency issues |
Co-Authored-By: Roelof <github@roelof.io>
- Don't double-install packages. Add new packages first and pull them all in at the same time. - Removed uopz. It's not being used. - Use `composer test`, which allows us to add additional tests there.
I had some suggestions, so I opened these in a Pull Request on your fork. |
Improved Laravel 6 Support
When will it be merged? I would like to use this lib on Laravel 6 without using my own repo. |
* @param int $value | ||
* @return string|null | ||
*/ | ||
function str_random(int $value = 16): ?string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why? Just replace it with Illuminate\Support\Str::random()
if it's even being used anywhere.
Seconding this, I hope @austinheap has some time to look at this. |
Thanks @roelofr. Looking forward for next release. |
Any idea when it will be merged? |
A merge would be awesome :) |
@emielmolenaar Yeah, especially since Laravel 7 is already live. |
Well, a major release every half year is something entirely different compared to the minor releases we had before. |
Could you please merge this? This is the only thing keeping me back on Laravel 5.8 |
Any more news on this one already? In need of the 6 version as well. |
Signed-off-by: Wim wim@join.marketing