Skip to content
This repository has been archived by the owner on Jul 17, 2024. It is now read-only.

Add support for Laravel 5.1 LTS #10

Merged
merged 4 commits into from
May 19, 2017
Merged

Add support for Laravel 5.1 LTS #10

merged 4 commits into from
May 19, 2017

Conversation

jbaron-mx
Copy link
Contributor

@matthewgoslett What do you think about creating a branch to support Laravel 5.1?

This PR allows the installation on 5.1 and also updates the readme.
I just tested out on my app and everything seems to work well, except that the url() method will not work because it was introduced until L5.2+

The reason to this is that there are many companies that cannot rely their backend on versions that are not LTS. Currently my case.

So the idea is to create a new branch and merge this PR on top of that.
Also it would be nice to link/mention the new branch on the current readme so everyone can switch to it if they need to.

Removed Public URLs section because url() function was introduced on
L5.2+
@matthewgoslett
Copy link
Contributor

I'm quite keen to keep it separate branch which can support both versions, assuming that we can just bump down the framework requirement and everything works as expected.
I haven't looked to deep but not sure if theres a way to not allow the url method to be called for older versions (by version checking in the code), and then just documenting in the readme re the url method support.

@jbaron-mx
Copy link
Contributor Author

Personally, I would prefer to just add a statement in the readme clarifying the url method will not work on older laravel instead of version checking to not allow it.

I'm sorry, I don't think I got what you mean by keep a separate branch supporting both version, which both? 🤔

Copy link

@etiennemarais etiennemarais left a comment

Choose a reason for hiding this comment

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

Otherwise looks good to me. Please update the readme to indicate which version breaks on the url changes? Then I think we can merge this. Thanks for contributing 🚀

README.md Outdated
@@ -54,34 +54,6 @@ The Google Client uses a few methods to determine how it should authenticate wit
4. If running in **Google App Engine**, the built-in service account associated with the application will be used.
5. If running in **Google Compute Engine**, the built-in service account associated with the virtual machine instance will be used.

### Public URLs

Choose a reason for hiding this comment

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

I would just put this back and clarify that older versions will not work. Having everything in a single branch is easier to maintain imo.

README.md Outdated
$url = $disk->url('folder/my_file.txt');

// see https://laravel.com/docs/5.3/filesystem for full list of available functionality
// see https://laravel.com/docs/5.1/filesystem for full list of available functionality

Choose a reason for hiding this comment

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

Some small change but please upcase the first character in this comment?

@jbaron-mx
Copy link
Contributor Author

@etiennemarais Done :) If you find any grammar mistake please feel free to suggest changes, as english is not my mother language. 😊

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

Successfully merging this pull request may close these issues.

3 participants