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

Prevent Container::make() from type-hinting the parameters #509

Merged
merged 1 commit into from
Jan 8, 2018

Conversation

Nikita240
Copy link
Contributor

I was having a hell of a time getting jsonitems to return any legitimate output. I am using Laravel 5.4.

The issue happens in LfmPath::pretty() when it attempts to resolve a new LfmItem instance.

return Container::getInstance()->make(LfmItem::class, [$lfm_path, $this->helper]);

The problem is that in Laravel 5.4 the Container::make() method does not accept the parameters as an argument. You instead have to use Container::makeWith(). Container::make() was changed in Laravel 5.5 to behave exactly the same as Container::makeWith(), so changing the method to makeWith() ensures backwards compatibility.

image

The second problem is that the Container::makeWith() method does not accept a non-associate array for the parameters. You instead have to pass an associative array with keys as the dependency name. See laravel/framework#18474.

We end up with this:

return Container::getInstance()->makeWith(LfmItem::class, ['lfm' => $lfm_path, 'helper' => $this->helper]);

The code still actually ran before, because laravel simply type-hinted the LfmPath object instead of creating it from the parameters. This however, meant that the data was getting wiped as it was getting passed through this function.

Nikita240 added a commit to Nikita240/laravel-filemanager-1 that referenced this pull request Jan 6, 2018
@streamtw
Copy link
Member

streamtw commented Jan 8, 2018

Thank you @Nikita240 !

@streamtw streamtw merged commit d646d3f into UniSharp:master Jan 8, 2018
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.

2 participants