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

[Form][2.3] Fixes empty file-inputs getting treated as extra field. #10251

Closed
wants to merge 2 commits into from

Conversation

Projects
None yet
7 participants
@jenkoian
Copy link
Contributor

commented Feb 12, 2014

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #8575 (#8575 (comment))
License MIT

Re-applies 968fe23 (PR #8575).

The test for this already exists, it was just this line that got overwritten by eb9f76d#diff-ca5e25b47f3ecc94cd557946aeb486c6L542

To clarify, this is a PR into 2.3 branch - this already exists in 2.4 (and later from this PR: #9146)

@Bendihossan

This comment has been minimized.

Copy link

commented Feb 14, 2014

+1

@jakzal jakzal added the Form label Feb 15, 2014

@jenkoian

This comment has been minimized.

Copy link
Contributor Author

commented Feb 20, 2014

Anyone? Should just be fixing a simple regression... @webmozart @fabpot

@craigmarvelley

This comment has been minimized.

Copy link

commented Feb 26, 2014

I'd really appreciate someone taking a look at this - we aren't able to work around it and upgrading to 2.4 is not feasible at the moment.

@bendavies

This comment has been minimized.

Copy link
Contributor

commented Feb 26, 2014

@cordoval

This comment has been minimized.

Copy link
Contributor

commented Feb 27, 2014

We have to come up with a tool that notifies or verifies the authors of the PRs merged when these type of ripple forward merging of 2.2 or 2.3 is done into the upstream branches. So that perhaps we can verify that @fabpot has not skipped anything since usually is the case that the PR author can say yes or no if things have been rightly merged also. But that is just an idea.

In any case, I believe the problem is twofold. We merged a test which is not a regression test because it does not fail when this change was not imported. This is our current situation. The test did not tell us that this was missing when it should have. This is a critical requirement to add even perhaps a note on the notes for contributors, especially for bugfixes. @jenkoian could you please fix the regression test?

The second part, I forgot 👶

@jenkoian

This comment has been minimized.

Copy link
Contributor Author

commented Feb 27, 2014

Thanks for the reply @cordoval!

I've updated the test so that it fails without my change. Let me know if there's anything else you need.

@cordoval

This comment has been minimized.

Copy link
Contributor

commented Feb 27, 2014

this should be green, what it would be beautiful is if you add two commits, one with the failing test and travis fails and the next one making it green 👶, that way we give it a second chance to the stopwatch and signal failing tests which are unrelated.

@jenkoian

This comment has been minimized.

Copy link
Contributor Author

commented Feb 27, 2014

OK @cordoval have pushed just the failing test for now - when travis reports back I'll commit the fix - hope that's what you meant...

@jenkoian

This comment has been minimized.

Copy link
Contributor Author

commented Mar 3, 2014

ping @cordoval

@fabpot

This comment has been minimized.

Copy link
Member

commented Mar 3, 2014

Thank you @jenkoian for taking care of this regression.

@fabpot fabpot closed this in 485efad Mar 3, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.