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

Remove illuminate/database dependency #59

Merged
merged 10 commits into from
Nov 23, 2021
Merged

Remove illuminate/database dependency #59

merged 10 commits into from
Nov 23, 2021

Conversation

leon0399
Copy link
Member

This MR closes #55

@leon0399
Copy link
Member Author

As you can see, there are some errors suporting lowest versions of Laravel 6.x on PHP 7.4. We need to decide, either we update minimal Laravel 6.x version, or we fix these bugs

@leon0399
Copy link
Member Author

This MR is now on pause, until #58 is merged and we decide what to do about my previous comment, after which I will rebase onto latest development branch and we will also test it on PHP 8.1

@leon0399
Copy link
Member Author

I've investigated, and found out, that these errors are occur, because Validated event was added only in Laravel 6.15.0, so I think it is possible to fix errors, without raising minimal Laravel version

@leon0399
Copy link
Member Author

I've fixed all errors. If you're not agreed to given solution, let me know, and I'll revert it

Copy link
Contributor

@mfn mfn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM and the changes make sense: keep L6 support but workaround older L6 releases not having the event yet => 👍🏼 from me

@eschricker
Copy link
Contributor

This MR is now on pause, until #58 is merged and we decide what to do about my previous comment, after which I will rebase onto latest development branch and we will also test it on PHP 8.1

#58 has been merged 😄

@leon0399 leon0399 marked this pull request as ready for review November 23, 2021 08:08
@leon0399
Copy link
Member Author

@eschricker done and dusted!

- name: Set Minimum Laravel ${{ matrix.laravel }} Versions
run: |
composer require "illuminate/contracts:${{ matrix.laravel }}" --no-interaction --no-progress --no-update
composer require "vlucas/phpdotenv:${{ matrix.dotenv }}" --no-interaction --no-progress --no-update
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you would add --dev here we would eliminate one warning (vlucas/phpdotenv is currently present in the require-dev key and you ran the command without the --dev flag, which would move it to the require key.)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eschricker I added it for us.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The --dev is just needed on phpdotenv not on illuminate/contracts

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The --dev is just needed on phpdotenv not on illuminate/contracts

Ok, I already remove it from contracts. You're welcome to close the PR if this is was missing :)

Copy link
Contributor

@eschricker eschricker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything fine. @leon0399 thanks for your work :)

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.

Unwanted and very strict dependency illuminate/database
5 participants