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

L10 compatibility #12

Closed
wants to merge 7 commits into from
Closed

L10 compatibility #12

wants to merge 7 commits into from

Conversation

saade
Copy link

@saade saade commented Feb 15, 2023

Hi!

This PR adds L10 compatibility for Tenancy to work.

Related archtechx/tenancy#1065

lukinovec added a commit to lukinovec/virtualcolumn that referenced this pull request Feb 15, 2023
.github/workflows/ci.yml Outdated Show resolved Hide resolved
Comment on lines +12 to +19
include:
- laravel: "^9.0"
testbench: "^7.0"
- laravel: "^10.0"
testbench: "^8.0"
exclude:
- laravel: "^10.0"
php: "8.0"
Copy link
Member

Choose a reason for hiding this comment

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

Why does include have testbench and exclude has php? Where is testbench used?

Copy link
Member

Choose a reason for hiding this comment

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

Also, at this point it might be better to just use something like:

- laravel: 9
   php: 8.0
- laravel: 10
   php: 8.1

That way the code will run on both PHP versions and both Laravel versions, we don't really care about L9 + 8.0 I think. And the syntax will be much simpler than having PHP versions listed, Laravel versions listed, an include section, and an exclude section.

@@ -2,7 +2,7 @@

## Installation

Supports Laravel 6, 7, 8, and 9.
Supports Laravel 6, 7, 8, 9, and 10.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Supports Laravel 6, 7, 8, 9, and 10.
Supports Laravel 9 and 10.


- name: Install dependencies
run: composer require "laravel/framework:^${{matrix.laravel}}.0"
run: composer require "laravel/framework:${{matrix.laravel}}"
Copy link
Member

@stancl stancl Feb 16, 2023

Choose a reason for hiding this comment

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

I'd keep the original syntax, the workflow name is more readable then (L10 instead of L^10). With Laravel we don't care about specifying the minor version, so there's no need to specify versions as ^x in the matrix.

Edit: Seems like we were using a different pattern in the tenancy repo, I'll see if it's worth changing.

@stancl
Copy link
Member

stancl commented Feb 16, 2023

I'll close this PR in favor of the other one here that we finished today.

For the preferred workflow syntax, see this and my reviews above: https://github.com/archtechx/virtualcolumn/pull/11/files#diff-b803fcb7f17ed9235f1e5cb1fcd2f5d3b2838429d4368ae4c57ce4436577f03f

If you could make those changes in your other PRs, I can merge them today and hopefully tag a tenancy release.

@stancl stancl closed this Feb 16, 2023
stancl added a commit that referenced this pull request Feb 16, 2023
* Add L10 support

* Update PHP version in ci.yml

* Update ci.yml

* Revert ci.yml changes

* Migrate PHPUnit XML config using "--migrate-configuration"

* Update phpunit.xml (delete cacheDirectory, use backupStaticAttributes instead of backupStaticProperties)

* Delete backupStaticAttributes

* Add PHP version matrix

* Use ci.yml from #12

* Correct Laravel matrix versions

* Revert ci.yml changes

* Use multiple PHP versions

* Update ci.yml

* Don't use PHP 8 with Laravel 10 in CI

* Update ci.yml

* Remove L6 and L8 support

* Use single PHP version in CI

* Update ci.yml

* Change matrix in ci.yml

* swap laravel 9 & 10

* Update README.md

---------

Co-authored-by: Samuel Štancl <samuel@archte.ch>
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

2 participants