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

[5.4] Make with #18271

Merged
merged 10 commits into from
Mar 13, 2017
Merged

[5.4] Make with #18271

merged 10 commits into from
Mar 13, 2017

Conversation

taylorotwell
Copy link
Member

@taylorotwell taylorotwell commented Mar 8, 2017

This allows functionality similar to old $container->make(class, params) functionality.

@taylorotwell
Copy link
Member Author

All my tests pass now. If anyone sees any broken behavior it with it feel free to let me know so I can add a test.

@@ -710,7 +710,7 @@ public function build($concrete)
// hand back the results of the functions, which allows functions to be
// used as resolvers for more fine-tuned resolution of these objects.
if ($concrete instanceof Closure) {
return $concrete($this, last($this->with) ?: []);
return $concrete($this, last($this->with));
Copy link
Contributor

Choose a reason for hiding this comment

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

Using last here means that the composer.json needs to be updated to include illuminate/support

Copy link
Member Author

Choose a reason for hiding this comment

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

OK I will remove those calls.

@taylorotwell
Copy link
Member Author

I should note this makes a change to the old behavior of the make(abstract, params) method. In this version, it will never use a previously resolved singleton instance when calling makeWith. In my opinion it should make a new instance each time this method is called because the given parameter array is dynamic.

@GrahamCampbell GrahamCampbell changed the title Make with [5.4] Make with Mar 9, 2017
@Daveawb
Copy link

Daveawb commented Mar 9, 2017

Hey Taylor, thanks for this. I know you mentioned that back and forth on this is not helping and I do agree.

I have a small problem with this PR. The make method should have the same signature as makeWith (as it was), PSR-11 declares a get method, not a make method. Wouldn't it be more prudent to create a get method that takes a single parameter and just mark make as deprecated for removal in L6. At that point maybe alias make to get and finally implement a makeWith method at that point?

This would solve a lot of community issues up front without code changes (unless already running on 5.4), and implement the methods you need for PSR-11 compatibility in the future?

@taylorotwell
Copy link
Member Author

Make can not have same signature as makeWith on 5.4 because we don't make breaking changes on point releases. Adding that parameter would break the interface.

I personally am totally apathetic towards PSR-11 as I think it's fairly useless overall but since it is so simple to add I don't have a huge problem with adding compatibility for it in Laravel 5.5.

@Daveawb
Copy link

Daveawb commented Mar 9, 2017

Haha, and here I was, wrongly assuming that this was all gearing the container up for PSR-11. Thanks for clearing that up.

I'm sure it'd be BC if the signature became make($abstract, $parameters = null)?

Either way, nit picking and I'm grateful you've put this PR together.

@taylorotwell
Copy link
Member Author

No, it wouldn't be backwards compatible. Look at the container interface.

@RemiCollin
Copy link

It wouldn't break the interface if $parameters is optionnal. @Daveawb is right.

@devcircus
Copy link
Contributor

Changing an existing public signature to add an optional param is a break. Might not affect many, but still a break.

@RemiCollin
Copy link

RemiCollin commented Mar 9, 2017

How would it affect them ? Even in the hypothesis some users were extending the base container without the optional parameter, it would still pass :

interface ContainerInterface {

	public function make($abstract);
}

class Container implements ContainerInterface {

	public function make($abstract, $parameters = null) 
	{
		// works
	}
}

class ExtendedContainer extends Container {

	public function make($abstract)
	{
		// works as well
	}
}

@Daveawb
Copy link

Daveawb commented Mar 10, 2017

@RemiCollin Actually looking at it, Taylor is absolutely correct when he says look at the interface for the container. Adding the extra parameter to the interface would not play well with any extended container instances that do not currently add an optional parameter. Alas, this is what it is. Looking forward to this PR being merged.

@RemiCollin
Copy link

Yes, if you change the Container interface. But that would not be mandatory in that case, if you look at my example.

Mostly wanted to showcase how it would be possible to maintain pre-5.4 compatibility, while not introducing BC in current release. Not saying it would be a good pratice or not, it's totally debatable, but I think that any tool/language feature we have to provide users with smoother upgrades should be considered.

@taylorotwell
Copy link
Member Author

People, I'm not going to argue if it is a breaking change or not. It obviously is if you understand how PHP works. Please no more discussion on that topic.

@taylorotwell
Copy link
Member Author

taylorotwell commented Mar 10, 2017

Do people even want this feature or not? People were freaking out about wanting in the issue thread and now its just a couple of people arguing about BC. I'm not going to bother with it (because I think the feature is kinda silly) if this isn't actually a needed feature.

@RemiCollin
Copy link

Not arguing, just discussing implementation.

But you're right, I won't bother if you feel it's an inappropriate discussion to have on a PR.

@Daveawb
Copy link

Daveawb commented Mar 11, 2017

@taylorotwell Gosh the communitys wants and needs have really ground you down man. No arguments to be had here, just, I hope, constructive conversation. The community does absolutely want this, I'm not sure why no one else has chimed in and given a thumbs up but I speak for many people on my team who would very much like this functionality back.

@terion-name
Copy link

@taylorotwell With this it is impossible to pass a scalar in params :(

@saumya04
Copy link

saumya04 commented May 12, 2017

@taylorotwell Thanks for the feature, but I wasted my 2 hours for finding this change. I think you should mention this method makeWith() on laravel's website under the changelog section.

@jonnywilliamson
Copy link
Contributor

Do people even want this feature or not?

I didn't know I wanted this feature until about 1 hour ago. Has make it so much easier to pass different configs into generating Guzzle clients now depending on if I'm testing/debugging/in production.

So thank you!

@benjoox
Copy link

benjoox commented Jul 6, 2017

Do people even want this feature or not?

It saved my time too. Thanks!

@skovmand
Copy link

skovmand commented Sep 2, 2017

Thank you, thank you for this re-addition!

@antoniopaisfernandes
Copy link
Contributor

Hi all,

I'm having an issue with testing/mocking/replacing the bind in the container.

Let me try to explain.

$this->source->type is, for this test, LocalFolder
$this->source is the source of data

This is the implementation:

$handler = app()->makeWith(
    $this->source->type,
    ['source' => $this->source]
);

$fileContents = $handler->get($item);

Everything working nicely in other tests but when I try to replace that class, it doesn't hit the replacement:

$this->app->bind(LocalFolder::class, new class($source) extends LocalFolder {
    public function get($item)
    {
        dd("IT HIT");
        return 'MOCKED_CONTENT';
    }
});

It never hits "IT HIT" :(

Do you know if there is any problem/issue with binding and resolving with makeWith?

Thank you so much.
Best regards

@browner12
Copy link
Contributor

this is not the appropriate place to ask for debugging help. Please ask in the Slack or on a forum (stackoverflow or others). Thanks

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.