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.5] Allow chaining of the $request->merge() method #22479

Merged
merged 1 commit into from Dec 19, 2017

Conversation

Projects
None yet
9 participants
@Jono20201
Contributor

Jono20201 commented Dec 19, 2017

I found that it would be nice to be able to chain methods on the request. eg. $request->merge([])->only(), but it is not possible to do this currently and I cannot see a reason why this should not be allowed?

[5.6] Allow chaining of the $request->merge() method
I found that it would be nice to be able to chain methods on the request. eg. `$request->merge([])->only()`, but it is not possible to do this currently and I cannot see a reason why this should not be allowed?

@taylorotwell taylorotwell merged commit 4d3144e into laravel:5.5 Dec 19, 2017

1 of 2 checks passed

continuous-integration/styleci/pr The StyleCI analysis has failed - 1 file needs addressing
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@sisve

This comment has been minimized.

Show comment
Hide comment
@sisve

sisve Dec 19, 2017

Contributor

Come on, 43 minutes of feedback time before merging? How is this not an obvious breaking change where we're changing the public api of two methods?

Contributor

sisve commented Dec 19, 2017

Come on, 43 minutes of feedback time before merging? How is this not an obvious breaking change where we're changing the public api of two methods?

@ntzm

This comment has been minimized.

Show comment
Hide comment
@ntzm

ntzm Dec 19, 2017

Contributor

Yeah it's changing the return type, but who is going to be using the result when it's always going to be null?

Contributor

ntzm commented Dec 19, 2017

Yeah it's changing the return type, but who is going to be using the result when it's always going to be null?

@sisve

This comment has been minimized.

Show comment
Hide comment
@sisve

sisve Dec 19, 2017

Contributor

There will, from now on, be new packages developed that may be using the returned value. And there are already existing packages that provides a custom Request classes that have overridden merge() or replace(). It has suddenly became those existing packages fault that something breaks; even when their code has worked for 5 months (since the 5.5 release) already. All those existing packages now needs to issue fixes to their code to keep working, just because we decided it would be nice to chain methods...

Why do we treat the 5.5-lts branch as a playground for every committer? Why are we not committing every change into the 5.6-dev branch, and just merge back any bug fixes?

Contributor

sisve commented Dec 19, 2017

There will, from now on, be new packages developed that may be using the returned value. And there are already existing packages that provides a custom Request classes that have overridden merge() or replace(). It has suddenly became those existing packages fault that something breaks; even when their code has worked for 5 months (since the 5.5 release) already. All those existing packages now needs to issue fixes to their code to keep working, just because we decided it would be nice to chain methods...

Why do we treat the 5.5-lts branch as a playground for every committer? Why are we not committing every change into the 5.6-dev branch, and just merge back any bug fixes?

@jhoff

This comment has been minimized.

Show comment
Hide comment
@jhoff

jhoff Dec 19, 2017

Contributor

I'm curious to see an example of how this could possibly be a breaking change.

Contributor

jhoff commented Dec 19, 2017

I'm curious to see an example of how this could possibly be a breaking change.

@sisve

This comment has been minimized.

Show comment
Hide comment
@sisve

sisve Dec 19, 2017

Contributor

@jhoff Was there something in my example that was unclear? If so, what?

If we had used strict php7 typehints, would it have been clear then that we are modifying the method signatures of a public api? Even if we're not using typehints, we're still declaring signatures via docblocks and those signatures are expected to be followed by every derived class. That implies that we do not change the signature since every derived class needs to be updated; in this case to add return $this; to these two methods.

Contributor

sisve commented Dec 19, 2017

@jhoff Was there something in my example that was unclear? If so, what?

If we had used strict php7 typehints, would it have been clear then that we are modifying the method signatures of a public api? Even if we're not using typehints, we're still declaring signatures via docblocks and those signatures are expected to be followed by every derived class. That implies that we do not change the signature since every derived class needs to be updated; in this case to add return $this; to these two methods.

@36864

This comment has been minimized.

Show comment
Hide comment
@36864

36864 Dec 19, 2017

Contributor

Package Foo exposes ExtendedRequest, a class that extends Illuminate/Http/Request which overrides some methods to provide some extra functionality, including merge and replace.

Before this PR, anyone could just import the package and get the extra functionality by importing ExtendedRequest aliased as Request into any of their classes.

After this PR, anyone who is using these new return values might try to do the same, only to find their code no longer works because ExtendedRequest hasn't been updated yet, maybe because the dev hasn't had time to, or simply because they have no idea that they need to update due to a change in a point release.

Contributor

36864 commented Dec 19, 2017

Package Foo exposes ExtendedRequest, a class that extends Illuminate/Http/Request which overrides some methods to provide some extra functionality, including merge and replace.

Before this PR, anyone could just import the package and get the extra functionality by importing ExtendedRequest aliased as Request into any of their classes.

After this PR, anyone who is using these new return values might try to do the same, only to find their code no longer works because ExtendedRequest hasn't been updated yet, maybe because the dev hasn't had time to, or simply because they have no idea that they need to update due to a change in a point release.

@Jono20201

This comment has been minimized.

Show comment
Hide comment
@Jono20201

Jono20201 Dec 19, 2017

Contributor

@36864 That isn't a "breaking" change however? It wouldn't cause immediate errors/unexpected behavior to an existing application, as a consequence of someone running composer update.

What your describing would happen in a major release anyway, as small changes like this are unlikely to be included in the upgrade guide.

Either way, as you can see from my commit message I did intend on this been targeted at 5.6 -- but I must have selected the wrong branch when making the PR.... 🤭

Contributor

Jono20201 commented Dec 19, 2017

@36864 That isn't a "breaking" change however? It wouldn't cause immediate errors/unexpected behavior to an existing application, as a consequence of someone running composer update.

What your describing would happen in a major release anyway, as small changes like this are unlikely to be included in the upgrade guide.

Either way, as you can see from my commit message I did intend on this been targeted at 5.6 -- but I must have selected the wrong branch when making the PR.... 🤭

@36864

This comment has been minimized.

Show comment
Hide comment
@36864

36864 Dec 19, 2017

Contributor

What your describing would happen in a major release anyway

And that would be fine.

Either way, as you can see from my commit message I did intend on this been targeted at 5.6 -- but I must have selected the wrong branch when making the PR.... 🤭

I don't think anyone's blaming you for getting your PR accepted.

Contributor

36864 commented Dec 19, 2017

What your describing would happen in a major release anyway

And that would be fine.

Either way, as you can see from my commit message I did intend on this been targeted at 5.6 -- but I must have selected the wrong branch when making the PR.... 🤭

I don't think anyone's blaming you for getting your PR accepted.

@sisve

This comment has been minimized.

Show comment
Hide comment
@sisve

sisve Dec 19, 2017

Contributor

@Jono20201 Your quotes around "breaking" indicates that you have another definition of it that what we think it has.

A change in one part of a software system that potentially causes other components to fail; occurs most often in shared libraries of code used by multiple applications

Source: https://en.wiktionary.org/wiki/breaking_change

This change is truly something that "potentially causes other components to fail". This is far from a bugfix or attempt to fix an unexpected behavior that may incur a breaking change. This change was a nice-to-have change.

These types of changes are allowed in major releases. This change should have targeted the 5.6 branch and documented in the upgrade guide. It could have been a simple mistake from your end, or perhaps just that you didn't thinking of the consequences, or lacked some weird scenarios some old and bitter people (read: me) know about, but this isn't your fault. We have a huge problem with changes being merged into the current "stable" branch on a whim, and in this case with less than an hour of discussion time for anyone to comment how this was targeting the wrong branch.

Contributor

sisve commented Dec 19, 2017

@Jono20201 Your quotes around "breaking" indicates that you have another definition of it that what we think it has.

A change in one part of a software system that potentially causes other components to fail; occurs most often in shared libraries of code used by multiple applications

Source: https://en.wiktionary.org/wiki/breaking_change

This change is truly something that "potentially causes other components to fail". This is far from a bugfix or attempt to fix an unexpected behavior that may incur a breaking change. This change was a nice-to-have change.

These types of changes are allowed in major releases. This change should have targeted the 5.6 branch and documented in the upgrade guide. It could have been a simple mistake from your end, or perhaps just that you didn't thinking of the consequences, or lacked some weird scenarios some old and bitter people (read: me) know about, but this isn't your fault. We have a huge problem with changes being merged into the current "stable" branch on a whim, and in this case with less than an hour of discussion time for anyone to comment how this was targeting the wrong branch.

@GrahamCampbell GrahamCampbell changed the title from Allow chaining of the $request->merge() method to [5.5] Allow chaining of the $request->merge() method Dec 19, 2017

mpyw added a commit to mpyw-forks/framework that referenced this pull request Dec 20, 2017

[5.6] Allow chaining of the $request->merge() method (laravel#22479)
I found that it would be nice to be able to chain methods on the request. eg. `$request->merge([])->only()`, but it is not possible to do this currently and I cannot see a reason why this should not be allowed?
@jarektkaczyk

This comment has been minimized.

Show comment
Hide comment
@jarektkaczyk

jarektkaczyk Jan 9, 2018

Contributor

Just stumbled upon this one and I'm amazed by the comments here.

@36864 That isn't a "breaking" change however? It wouldn't cause immediate errors/unexpected behavior to an existing application, as a consequence of someone running composer update.

Can you estimate cost of a change that breaks during composer update ?

Now, can you estimate cost of a change that BREAKS the application (changes its behavior) in a way that doesn't break during composer update? No. You have no idea how it breaks things.
If you're lucky enough to have tests targeting this particular behavior (provided by the framework, so you'd rather not have it) they will tell you that. Otherwise the loss might be unlimited for the business.

Contributor

jarektkaczyk commented Jan 9, 2018

Just stumbled upon this one and I'm amazed by the comments here.

@36864 That isn't a "breaking" change however? It wouldn't cause immediate errors/unexpected behavior to an existing application, as a consequence of someone running composer update.

Can you estimate cost of a change that breaks during composer update ?

Now, can you estimate cost of a change that BREAKS the application (changes its behavior) in a way that doesn't break during composer update? No. You have no idea how it breaks things.
If you're lucky enough to have tests targeting this particular behavior (provided by the framework, so you'd rather not have it) they will tell you that. Otherwise the loss might be unlimited for the business.

@deleugpn

This comment has been minimized.

Show comment
Hide comment
@deleugpn

deleugpn Jan 9, 2018

Contributor

I see a lot of theoretical discussion but nobody actually hit the revert button on GH. On another note, I read this whole thread a few times and haven't seen a breaking change case in course.
A strongly typed language could treat this as breaking on compile time, but as nobody would be actually checking if return is null and php is not strongly typed, we assume this is not a breaking change.

Contributor

deleugpn commented Jan 9, 2018

I see a lot of theoretical discussion but nobody actually hit the revert button on GH. On another note, I read this whole thread a few times and haven't seen a breaking change case in course.
A strongly typed language could treat this as breaking on compile time, but as nobody would be actually checking if return is null and php is not strongly typed, we assume this is not a breaking change.

@devcircus

This comment has been minimized.

Show comment
Hide comment
@devcircus

devcircus Jan 10, 2018

Contributor

There's a cut and dry explanation above as to how this is breaking.

Contributor

devcircus commented Jan 10, 2018

There's a cut and dry explanation above as to how this is breaking.

@deleugpn

This comment has been minimized.

Show comment
Hide comment
@deleugpn

deleugpn Jan 10, 2018

Contributor

I don't see it.

Contributor

deleugpn commented Jan 10, 2018

I don't see it.

@sisve

This comment has been minimized.

Show comment
Hide comment
@sisve

sisve Jan 10, 2018

Contributor

Someone writes a package that targets 5.5 that calls $request->merge(...)->merge(...). It works for some 5.5 releases. That is the problem.

Contributor

sisve commented Jan 10, 2018

Someone writes a package that targets 5.5 that calls $request->merge(...)->merge(...). It works for some 5.5 releases. That is the problem.

@ntzm

This comment has been minimized.

Show comment
Hide comment
@ntzm

ntzm Jan 10, 2018

Contributor

So the package that uses that feature will have the specific version thats required? Laravel doesn't follow semver so new features are expected in patch releases

Contributor

ntzm commented Jan 10, 2018

So the package that uses that feature will have the specific version thats required? Laravel doesn't follow semver so new features are expected in patch releases

@36864

This comment has been minimized.

Show comment
Hide comment
@36864

36864 Jan 10, 2018

Contributor

I think the general expectation is that if a package works with 5,5.X it should also work with 5.5.X+1.

Or maybe that's just wishful thinking on my part and in the real world version numbers are made up and compatibility doesn't matter.

Contributor

36864 commented Jan 10, 2018

I think the general expectation is that if a package works with 5,5.X it should also work with 5.5.X+1.

Or maybe that's just wishful thinking on my part and in the real world version numbers are made up and compatibility doesn't matter.

@sisve

This comment has been minimized.

Show comment
Hide comment
@sisve

sisve Jan 10, 2018

Contributor

@ntzm I could accept that we say "new feature in 5.5.10; we now support 3d!" and then you could ask "do you have the version that supports 3d?". But now we have to ask if they have the version after the slight change, or the version before it. Can we even get people to report the patch number in issue reports?

@36864 I think the general expectation by now is that we just don't care. The newest shiniest shall always be merged, and we shall have no stability in the branches.

I guess we're moving in two different circles...

Contributor

sisve commented Jan 10, 2018

@ntzm I could accept that we say "new feature in 5.5.10; we now support 3d!" and then you could ask "do you have the version that supports 3d?". But now we have to ask if they have the version after the slight change, or the version before it. Can we even get people to report the patch number in issue reports?

@36864 I think the general expectation by now is that we just don't care. The newest shiniest shall always be merged, and we shall have no stability in the branches.

I guess we're moving in two different circles...

@Jono20201 Jono20201 deleted the Jono20201:patch-2 branch Feb 8, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment