Skip to content
This repository has been archived by the owner on Oct 18, 2020. It is now read-only.

laravel 5.7 support #192

Closed
wants to merge 1 commit into from
Closed

laravel 5.7 support #192

wants to merge 1 commit into from

Conversation

PheRum
Copy link

@PheRum PheRum commented Sep 4, 2018

No description provided.

@d13r
Copy link
Owner

d13r commented Sep 4, 2018

I'll leave this open so other people can use it if they want a temporary workaround:

{
    "repositories": [
        {
            "type": "vcs",
            "url": "https://github.com/PheRum/laravel-breadcrumbs.git"
        }
    ],
    "require": {
        "davejamesmiller/laravel-breadcrumbs": "dev-master"
    }
}

However, I probably won't be merging it because I don't generally support multiple Laravel versions in a single Breadcrumbs version - mainly because I haven't figured out a way to run the unit tests against both/all versions. i.e. at the moment this will only run the tests against Laravel 5.6 not 5.7, while if you upgraded testbench it would only run them against 5.7 but not 5.6.

I will aim to release a new version with official 5.7 support in the next couple of weeks. There are some architectural changes I'm thinking of making at the same time...

Cheers
Dave

@d13r d13r mentioned this pull request Sep 4, 2018
@freekmurze
Copy link

freekmurze commented Sep 4, 2018

Here's how I run tests against both L5.6 and L5.7: https://github.com/spatie/laravel-view-components/blob/master/.travis.yml#L7-L14 . Works great!

There are no changes needed to support L5.7 in your package, only the constraints need to be a bit more flexible. For 99% of my packages I only had to tag on extra version for illuminate stuff and testbench https://github.com/spatie/laravel-view-components/blob/master/composer.json#L18-L24

If you want I could send a PR for all these changes.

In my opinion you'd have no risk for adding these changes to laravel-breadcrumbs. Many users of your package could just upgrade normally to L5.7. (No need to keep in the back of your mind that you'll need to remove that vcsrepository you're suggesting at some point).

Everybody runs their packages in the way they want, so if you stick with not tagging the current version for L5.7, I'd respect that.

Thanks for all your excellent work with this package. I'm using it in Oh Dear!.

@d13r
Copy link
Owner

d13r commented Sep 4, 2018

Thanks Freek, I'll experiment with that. 8-)

@freekmurze
Copy link

Let me know if you have any questions around this!

@d13r d13r mentioned this pull request Sep 5, 2018
d13r added a commit that referenced this pull request Sep 5, 2018
@d13r
Copy link
Owner

d13r commented Sep 5, 2018

OK, that's done and tagged as 5.1.1.

Thanks again for the suggestion @freekmurze. I used the same idea but with explicit version numbers instead of --prefer-lowest.

https://github.com/davejamesmiller/laravel-breadcrumbs/blob/1013335a528d4ec07c9a294f9f0b794b9440d4e2/.travis.yml#L13-L19

(In case anyone wonders why I named the variable $Laravel instead of e.g. $LARAVEL_VERSION, it's because it looks nicer in Travis CI.)

@d13r d13r closed this Sep 5, 2018
@freekmurze
Copy link

Job well done 👍

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants