Skip to content
This repository has been archived by the owner on Jan 30, 2020. It is now read-only.

BC break introduced in 2.8.4 #103

Merged

Conversation

davidwindell
Copy link
Contributor

@davidwindell davidwindell commented Jul 8, 2016

Tests to demonstrate the breaking change in behaviour introduced by 2.8.4 (this test will pass in 2.8.3)

Discussion in #102

@weierophinney
Copy link
Member

I've looked into this, and it's pretty much as simple as changing this block within bindValues():

if (!array_key_exists($name, $values)) {
    if (!($element instanceof Collection)) {
        continue;
    }

    $values[$name] = [];
}

to:

if (!array_key_exists($name, $values)) {
    continue;
}

However, that makes the test introduced in #19 fail (FormTest::testShouldHydrateEmptyCollection()).

The question is: which is the correct behavior?

The argument @davidwindell makes is that the change from #19 introduces a BC break at this point. What was argued on #19 was that the previous behavior was incorrect. As I noted when analyzing an iteration of that pull request:

Essentially, this patch has exposed a subtle bug that was masquerading as a feature. The test was checking to ensure that a value in a nested fieldset, when resubmitted, would not change. What's interesting is that, in stepping through the code, it appears that the reason it worked previously was because the nested fieldset was being silently ignored during binding. The test was passing, but because the scenario it described just happened to also be the result of an issue.

I'm really unsure how to proceed here. I'm dismayed that there is a change in behavior that is affecting users, but, on the other hand, reverting the patch also results in behavior that affects users. About the only thing I can think of at this point to accomplish both goals is to add a flag to allow opting into one or the other behaviors, and, frankly, flags are a maintenance nightmare.

Thoughts?

@weierophinney
Copy link
Member

Okay, looks like 106 solves it... via a "flag" passed to the collection and/or form when calling bindValues().. Will close this when I merge that.

@weierophinney weierophinney merged commit e3956c3 into zendframework:master Sep 14, 2016
weierophinney added a commit that referenced this pull request Sep 14, 2016
@davidwindell
Copy link
Contributor Author

Thank you very much for taking the time to look into this @weierophinney and @svycka for your patch.

Zf3 here we come!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants