[5.8] Remove the container makeWith method

[X-Coder264]

The makeWith method was deprecated and just aliased to the make method since Laravel 5.5 -> #19201

This PR aims to finally delete it from the framework as Laravel 5.5 was released quite some time ago.

[deleugpn]

Totally opposed to this change as there is no good reason for it whatsoever.
I understand it's just an alias, but it's much more expressive and makes the consumer code better.
I always prefer makeWith over make when I'm defining some parameters.

[X-Coder264]

I'd argue the complete opposite. There is no good reason whatsoever for keeping it. By removing it there is less maintenance burden as there will be less code to maintain. Plus stuff like this (laravel/telescope#410) wouldn't happen (currently if somebody wrote their implementation of the contract Telescope wouldn't work with it). Besides, using a method that is not on the contract is never a good idea, especially when that method is just a deprecated alias for a method that is on the contract.

[brunogaspar]

While i agree, it's just an alias, do keep in mind that the method is not marked as deprecated.

I'm fine with removing it, however, it should be marked as deprecated first, then on 5.9 or something, remove the method.

[X-Coder264]

@brunogaspar AFAIK Laravel doesn't explicitly mark methods as "deprecated", making an alias is Laravel's way of implicitly doing that. The same thing happened with the Event dispatcher fire method in Laravel 5.4 which was now removed for 5.8 (#26392)

[paras-malhotra]

I agree with @X-Coder264. I think it isn't going to be a major breaking change to remove it as it's pretty much an alias. The upgrade guide would cover this so I think we should be fine removing it in 5.8.

I think Taylor's target has always been that a minor upgrade should be possible within 10-15 minutes, and since it's an alias, I think it should be fine and pretty quick to change.

[brunogaspar]

@X-Coder264 Laravel might not mark them as deprecated, religiously, but i've seen it being done on Laravel before.

Now, if i think that Laravel should have more strict guidelines for this kind of stuff? Yes, it should, we shouldn't go blindly remove stuff without first marking such methods as deprecated, even if it takes 2 minutes to update after.

But personally i don't care much if it's removed or not, as i usually use what the contract offers, but i believe it should be done right.

[X-Coder264]

@brunogaspar I don't see the benefit of marking something explicitly as deprecated as Laravel isn't semantically versioned. Laravel allows for breaking changes in major releases (5.8 being the next one) and it's going to be documented in the upgrade guide so what is the benefit of marking it as deprecated and then waiting until 5.9 to remove it? You are just delaying the removal for six months for no benefit/reason (the user will need to do a quick find and replace in the project no matter if it's gonna be removed in 5.8 or 5.9).

[brunogaspar]

You say there's no benefit/reason, i totally respect that and i can totally see why you think that, for such small change, however, i see it the other way around though, which is to mainly give time for developers to know that such method is going to be removed.

I'm probably saying all of this because i've been affected by such changes before, many times, where such changes were not documented, at all or breaking changes being done in minor releases for example.

As you say, it's going to be on the Upgrade Guide, so to me, it should be marked on the Upgrade Guide as deprecated first, as a warning basically, and then it can be removed later. People should be hopefully ready by the time 5.9 is released.
That would be the proper way to do it of course, and you don't even need to follow SemVer to adhere to this, it's pretty much common sense.

Even adding it on the Upgrade Guide or Changelog, doesn't mean people will read it or adhere to it, i've seen many many many people come here after an upgrade to a latest version and complain something was not working or totally broken, then they realise they didn't follow the Upgrade Guide.

---

But either way and to end the discussion here, at least for me, as i said on my previous reply, i don't care much if it get's removed right away or marked as deprecated, i'm fine with it as i usually follow what the contract offers, but that doesn't mean i'll not have to perform upgrades later down the road, since method signatures for example, are constantly updated, for the better of course.

[deleugpn]

It's a better change to bring it to the contract than to remove it from the code, if you really care about the contract here.
I don't write code that depends on Laravel contracts unless I'm writing a package and in that case I would stick with make. However, as a developer working fully with laravel/laravel I feel free to type-hint the concrete implementation and rely on the makeWith for expressiveness. The argument of being less code to maintain is useless at best as we all agreed that this is just an alias.
I'm also fine with not having it on the contract and still having it on the concrete.

[X-Coder264]

It's a better change to bring it to the contract than to remove it from the code, if you really care about the contract here.

Sorry, but I don't see how would that be a better change. Each method on the contract presents an unique feature that the concrete implementation needs to have. Forcing all implementations to have an alias method doesn't make any sense. And for what? Some subjective "expressiveness"?

I don't write code that depends on Laravel contracts unless I'm writing a package and in that case I would stick with make. However, as a developer working fully with laravel/laravel I feel free to type-hint the concrete implementation

I don't see the benefit of type-hinting the concrete implementation (be it a package or not). That just seems like a bad practice, but that is completely off-topic and not relevant to this PR.

[deleugpn]

How can it be off-topic if you're affecting the set of users that type-hint the concrete implementation just for the sake of the contract? If you want to use only contract, by all means go ahead and do whatever you want, but don't change the framework in such a way that affect negatively the people that don't do what you do.

[X-Coder264]

"Just for the sake of the contract"? I hope you are kidding - I don't know why are contracts treated in such a way in the Laravel community. We had the same exact case with the Event dispatcher fire method (which I mentioned in a previous comment) and nobody objected anything. Here you are the only one against the removal as far as I can see (the others just commented on the way it should be removed) and your entire argument is based on "this is more expressive" which is IMHO absurd at best. This breaking change can literally be fixed by doing a search&replace in your project which should take a couple of minutes at most so talking about a "negative effect" this is gonna have on people is also weird.

[devcircus]

The comparison to "fire" method is not quite the same. The fire method was intentionally changed with plans to remove in the future. "makeWith" was written when making with parameters was reintroduced after being removed previously. Then the make method was changed to do the same. makeWith maintains a more intentional meaning compared to make and I believe should remain, or at least go through proper deprecation. As for the contract, a contract doesn't have to contain every method used in a concrete, only those which are required for the concrete to work properly. It doesn't really need to be there, and would just give implementors another method to define.

[sisve]

I do not think that @deleugpn is alone in wanting deprecation and api stability, but perhaps he was the only one answering during sunday hours (assuming an European timezone), or perhaps he is the only one not watching the current CS:GO minor (semifinals and finals going on for the last 6 hours). Perhaps it's a correlation thing; those that want api stability could be those that doesn't monitor github issues during weekends.

There's some history about make and makeWith. The makeWith method was added in 5.4.16 to bring back extra parameters to the make method, which was once removed. The make method itself couldn't be changed because it would be a breaking change. The 5.4.0-5.4.15 releases did not have a method that allowed passing in parameters. This means that every package that needed this functionality, and target 5.4, had to wait for 5.4.16 and call that makeWith method. These packages has worked with that method call since then (Mar 15, 2017), for over 20 months..

Why should we introduce changes so that packages that has worked for this long (2 year when 5.8 will be released) would need to be changed?

Every change has a cost associated. What you assume is just an easy change to the application developer could just as well break third party packages. Suddenly tutorials and documentations will be incorrect. These takes much longer than a few minutes to correct.

I also believe a search-and-replace is more complex than you make it out to be; perhaps it's a tooling thing, or a language thing, but you would need to look at every invocation of a makeWith method to figure out if it's the correct one, or another identically named method in another class.

I believe there's one very important comment in github that we've totally failed to communicate widely, or even follow at all. However, it's still very relevant.

We will attempt to deprecate removals for 1 release in the future.

Source: laravel/ideas#383 (comment)