Fix for issue #5699 - Disable label position caching for Zend\View\Helper\FormRow::__invoke()

[Martin-P]

• Reverted Form\View\Helper\FormRow label position gets overwritten by __invoke() #4771 so __invoke() does not cache label position anymore
• Added new method setDefaultLabelPosition which is cached and reused by Zend\View\Helper\FormRow
• Added new method getDefaultLabelPosition for easy debugging purposes
• Added tests for the new functionality

[stefanotorresi]

hmmm, I see the catch. The thing is that the labelPosition property should already be the default. It's a class property; if changing it via its accessor is not meant to alter the behaviour of FormRow, and only be used internally (which is the case in your PR), then it shouldn't be accessible in the first place.

A solution could be to pass the parameter to from invoke() to render() as a second optional argument rather than using the accessors internally, so that invoke() doesn't make the change persistent. This way the setter can be used to alter the default behaviour (which would allow you to not change the test I added in #4771, and avoid a BC break).

What do you think?

/cc @weierophinney

[sparrowek]

I thought myself of sugesting such solution (passing extra param to render) when posting the issue but was not sure if it will not break the interface of view helpers... Anyway changing one property (labelPosition) in two contexts (permanent and temporary) causes the problem - so I thougth also of using a second property to store the temporary change in invoke() but that did not seem an elegant solution to me...

[stefanotorresi]

the render() method signature is not enforced by any contract so as long as the additional parameter stays optional it's not a problem.

[Martin-P]

which would allow you to not change the test I added in #4771, and avoid a BC break

I try not to debate on what exactly is the BC break, because that depends on the point of view. Both fixes (#3626 and #4771) are a BC break for the other group 😉 An additional parameter for render() sounds like a nice solution though.

[stefanotorresi]

Well, strictly speaking #4771 adds a test, letting existing ones pass,
while this PR needs to modify that very test to pass ;) (leaving aside the
fact that travis is failing the new tests anyway :P).
Admittedly, #4771 introduced a regression for which no regression test was
ever added!

That said, if you approve the approach I suggested, let's go with it; we'll
save the day and make everyone happy! :)

[Martin-P]

You are right, I see now that it is failing due to HTML vs. XHTML syntax. I did not see why it failed earlier. The tests for the initial fix (#3626) were indeed incomplete, testing label position cache for __invoke() was never added.

I am a bit new to GitHub, but I think it is ok to change this PR with your suggestion? Or do I need to create a new PR with the correct fix?

[stefanotorresi]

No need for a new PR, you can safely start from scratch and push --force to clean it up ;)

[weierophinney]

The original workflow we developed was:

$this->setLabelPosition('prepend');
// render a bunch of FormRow's, which are just elements wrapped with labels, help descriptions, and validation error messages

This workflow was in place because it assumed that every FormRow call you make in a given request will likely be the same. If you need to change between elements, you call setLabelPosition() again, or pass the position as part of the helper call.

The only improvement I think we'd need to make differently is to alter render() to accept the label position as an optional, second argument, and __invoke() to pass the argument directly. render() would pull the default value, if null, from the property, and use ::LABEL_PREPEND if still null. If you're willing to change your PR to do that, I can accept it, @Martin-P .

[Martin-P]

Note:
I removed one test that checked if the label position can be permanently set via __invoke(). Did I understand correctly that __invoke() should not permanently change the label position?

[Martin-P]

@weierophinney Is the fix ok this way or does it need some changes?

[Ocramius]

This PR needs a rebase

[Martin-P]

Is there an easier way for rebasing without a huge list of conflicts? It is a bit time consuming having to fix all those conflicts all the time.

[Ocramius]

@Martin-P not really - rebasing is really about fixing those conflicts. Either you fix them, or the merger has to, and honestly, if every PR had a huge set of conflicts to fix, reviewers would have a hard time getting stuff done :(

[Maks3w]

@Martin-P Merging the most recent version of master is another approach and only need fix conflicts for one commit. It's more cleaner rebase but if you feel it's a nightmare then merge.

Also you could fix up some of your commits which are not outstanding like "fix typo"

[Ocramius]

@Maks3w if the PR author merges master into his PR we have a nightmare when we need to git bisect looking for bugs... I always squash PRs when that happens...

[Martin-P]

Also you could fix up some of your commits which are not outstanding like "fix typo"

How can I change that? I need to do a commit to make the changes? I guess my knowledge of git is somewhat limited :)

[Ocramius]

@Martin-P I can squash the commits, no big deal.

[weierophinney]

Ping @Ocramius -- will you be able to merge this, or should I reassign? Also, I marked for 2.4.

[Ocramius]

@weierophinney I can merge in a couple of hours