Skip to content

Laravel 5#24

Closed
ghost wants to merge 18 commits intomasterfrom
unknown repository
Closed

Laravel 5#24
ghost wants to merge 18 commits intomasterfrom
unknown repository

Conversation

@ghost
Copy link
Copy Markdown

@ghost ghost commented Jun 18, 2015

Hi,

I had to use Trucker on a project and since I started fresh, I made the changes for Laravel 5. You might see a lot of useless commits since I was just confused about how Laravel 5 worked for config setting and getting ++ combined with the ConfigManager.php.

Everything seems to work fine for now. Since it's not a refactoring job, I left Guzzle to its original version in composer.json, but I guess someone who worked on the project could just verify if the latest version could make it.

Also, I changed only one line in the readme.md to reflect the new Laravel 5 way to publish vendor configs, but feel free to review if anything else is missing!

@andreasba andreasba mentioned this pull request Dec 31, 2015
return array_replace_recursive($items, $customItems);
});
}
$this->mergeConfigFrom(__DIR__ . '/../config/trucker.php', 'trucker');
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Have you verified that these config changes are functional?

@bkuhl
Copy link
Copy Markdown
Member

bkuhl commented Mar 8, 2016

Thanks for digging into this! Can you take a look at TravisCI and make the necessary corrections to get the build passing and then I'll re-review?

@davidwebca
Copy link
Copy Markdown

I did use it in production for a time until the project shifted to another solution and everything was working fine, but I don't have the setup to test things manually anymore.

@davidwebca
Copy link
Copy Markdown

I think the testes would have to be rewritten completely, but everything seemed to work on prod.

@bkuhl
Copy link
Copy Markdown
Member

bkuhl commented Mar 9, 2016

In my experience with L5 is that it's actually best to avoid writing tests for ServiceProviders since it usually relies on the IOC container. I'll fork your branch and give the tests some love.

@bkuhl
Copy link
Copy Markdown
Member

bkuhl commented Mar 9, 2016

@brianwebb01 Are you ok with me making changes to this TruckerServiceProvider that make this a Laravel-only package?

@brianwebb01
Copy link
Copy Markdown
Contributor

The world is your oyster. Do as you wish.

On Tuesday, March 8, 2016, Ben Kuhl notifications@github.com wrote:

@brianwebb01 https://github.com/brianwebb01 Are you ok with me making
changes to this TruckerServiceProvider that make this a Laravel-only
package?


Reply to this email directly or view it on GitHub
#24 (comment).

@bkuhl
Copy link
Copy Markdown
Member

bkuhl commented Mar 9, 2016

Digging deeper into this, there are a lot of dependencies that are no longer provided in some of Illuminate's package that this project relies on for the tests to pass. I'm not going to spend the time to update this project to make the tests pass because the work is significant. However I would like to make your branch available to others.

Are you able to change the MR for this branch so instead of going into master it goes into a laravel-5.2-dev branch?

@davidwebca
Copy link
Copy Markdown

I can't seem to figure out how to change it, but I'll try to initiate a new one instead.

@bkuhl
Copy link
Copy Markdown
Member

bkuhl commented Mar 9, 2016

Branch laravel-5.2-dev now exists

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.

3 participants