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

Added support for Laravel 4.2 #28

Merged
merged 5 commits into from
May 17, 2014
Merged

Added support for Laravel 4.2 #28

merged 5 commits into from
May 17, 2014

Conversation

boris-glumpler
Copy link
Contributor

I changed Illuminate\View\Environment to Illuminate\View\Factory, changed the variable name from $environment to $factory and bumped the Mockery dependency to ~0.9.0, cause PHPUnit 4.0 was throwing errors for 0.8. Because Laravel 4.2 is PHP 5.4 and up only I also removed the PHP 5.3 tests from .travis.yml.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling b34978b on ScubaClick:master into dac3c4b on davejamesmiller:master.

@boris-glumpler
Copy link
Contributor Author

Why would it change? The code is still the same. All I basically did was update things for Laravel 4.2. In this case that just meant changing Environment to Factory, cause it got renamed in 4.2.

@boris-glumpler
Copy link
Contributor Author

Just noticed that all my commit messages got truncated. No idea why...

@d13r
Copy link
Owner

d13r commented May 15, 2014

Hmm, I'm not sure about completely removing support for 4.1 (forcing people to use an old version of the package) unless it's absolutely necessary, nor about pinning it to L4.2, completely preventing it being installed on L4.3+. I've not had time to try L4.2 myself yet, but I thought the only required change was removing the type hint.

Why would it change? The code is still the same.

It's an automated message, don't worry about it!

@d13r
Copy link
Owner

d13r commented May 15, 2014

Sorry just noticed it says ~4.2 (meaning >=4.2,<5) - so ignore what I said about 4.3+.

@boris-glumpler
Copy link
Contributor Author

Well, 4.2 makes breaking changes, so I don't see how you can support both 4.1 and 4.2 without different branches (currently you only have master, right?), unless you're adding checks for the Laravel version. Personally, I'd create a 4.1 branch and then leave the master branch to go forward with 4.2.

@d13r
Copy link
Owner

d13r commented May 15, 2014

I don't see how you can support both 4.1 and 4.2

As far as the code goes, it looks like removing the type hint is all that's required to support both. In the unit tests it looks like the only thing that changed is the class name, so an if (class_exists(...)) should do it. The actual variable names (environment or factory) don't really matter - I would just change them to factory as you have.

I don't plan to support older versions of Laravel forever, but I'd rather avoid maintaining multiple branches in the meantime.

Thanks

@boris-glumpler
Copy link
Contributor Author

Yup, if that's the way you want to do it, then that's fine. Do you want me to submit another PR or are you gonna do that?

@d13r
Copy link
Owner

d13r commented May 15, 2014

If you could that would be great (you should be able to update this one) - I probably won't have a chance to get into it until the weekend, but if it's working OK I can at least merge it into master from GitHub. Cheers.

@boris-glumpler
Copy link
Contributor Author

Ok, will do it tomorrow. Already too late where I am...

@dwightwatson
Copy link
Contributor

Brilliant work, just ran into this one myself and was about to cook up my own PR. Looking forward to this making it's way into the package.

@boris-glumpler
Copy link
Contributor Author

Ok, have re-added support for Laravel 4.1. Unfortunately, we can't type hint the Factory/Environment class in the Manager constructor anymore, so I just added a check for those and it throws an InvalidArgumentException now if it's neither. Also adjusted the test classes.

d13r added a commit that referenced this pull request May 17, 2014
Added support for Laravel 4.2
@d13r d13r merged commit c790c09 into d13r:master May 17, 2014
@d13r
Copy link
Owner

d13r commented May 17, 2014

Thanks. 👍 I've merged it into master for testing. I'll try to make a new release with it soon, before L4.2 is released.

@boris-glumpler
Copy link
Contributor Author

Ok, cool! Still think that a separate branch would be better,,,

@d13r
Copy link
Owner

d13r commented May 17, 2014

I'm not convinced. This way I only have one branch to maintain - no messing around with merging and releasing multiple versions. The changes are so minor I really can't see any point in having a separate branch.

@d13r
Copy link
Owner

d13r commented May 19, 2014

Released as v2.2.1. Thanks again for your help.

@d13r d13r added the bug label May 19, 2014
@k1ng440 k1ng440 mentioned this pull request Dec 14, 2014
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.

4 participants