Bugfix for issue #35 #322

Closed
wants to merge 1 commit into
from

Projects

None yet

6 participants

@basv

Hi guys. This issue has been open for a long time. I provided a fix in bitbucket but got no response. I really hope this fix will be pulled into CI. I hope to receive some feedback if you decide not to. Cheers! Bas

Bas Vermeulen Fixes issue #35 a4da3bc
@philsturgeon

Seems like a valid fix, but not sure how well it goes with the way set_value() was patched. That used array_pop() to remove the first item from the post array, which meant no extra key was required but it could only be used once for each key item.

This is a different approach and may be better, but we have to be careful about making this work in a similar way.

@basv

Yeah, with that patch I started my quest for the holy fix for the set_select issues. That patch indeed works fine for set_value, where we deal with one value and would also work properly for select boxes with just one option :)

I tried hard to come up with something where we wouldn't need to set a fourth parameter but failed to do so, which is a con , I know. The pro of this implementation is that in most situations you don't have to set this parameter and it's backwards compatible with all the current set_select() implementations. So only those who do need support for arrays as field names will have to bother with this fourth parameter.

If we can find a way to do this without a fourth parameter that would be great ofc!

@basv

Any new thoughts on this?

@royduin

Is this one still open? It's from 2 years ago! Just experienced the same problem. I'm going to try your fix!

UPDATE:
Just tested it and it works perfectly!

@cryode

There are so many discussions and pull requests revolving around array notation form items and how to retrieve / deal with them properly. Stuff has even been merged already. This PR is so old, there's no way it can be merged without conflicts, and clearly the submitter does not have enough interest to maintain it. Can only do as much as the community is willing to put in.

@royduin

I've extended the framework with this solution. So just the set_select function in MY_ files and it works perfectly. I don't see why this couldn't merged. Never mind, it solved my problem so I'm satisfied.

@ckdarby

@royduin It can't be merged because it is 2 years old. All of those changes will be conflicts with this & we only deal with fast forward merges.

@basv

LOL @ how I get the blame for not showing any interest to maintain this. This fix was submitted years ago, even before CI moved to github. Nobody seemed to care enough to merge this into the core. Why the *** should I "maintain" a fix if nobody is accepting it! Pretty arrogant how you put this on me @cryode . Think before you write this crap.... If you had any plans to implement any solution fixing the array repopulation, a simple email with a request to get this updated would have been enough...

@royduin Yeah it works perfect for me, even with the latest version. I gave up getting this in the core. Glad to see it helped someone else :D

@cryode
@basv

Well, why would I have any interest in keeping it up to date after spending hours to get it in a proper fix with bug report and pull request without any action from the CI developers? Sorry if my reaction was harsh but the way you put it and the frustration I already had about this just pissed me of. No hard feelings though.

@basv

@ckdarby Can you point out why this can't be merged, yes it's 2 years old but I checked the current form_helper and form_validation files from the develop branch and I don't see any changes that makes this update not compatible. I am wondering which differences you found?

@cryode

I don't blame you in the least for having no interest. The fact that that is still an open issue in the first place is beyond amazing. Why it's still an open issue, though, could be for a number of reasons, the fault of your own, CI's team, or other extenuating circumstances. I have to guess when I see old PR's like this still collecting dust, and most often it's because the original submitter stopped paying attention.

@basv

Okidoki, thanks for your clarification, and again sorry for my pissed of reaction ;p. Would you mind to help me check out why this won't work to get merged in the current code?

@royduin

Maybe there are some changes made beside the set_select function in form_helper.php and Form_validation.php. So merging now isn't a good idea I think. Get the latest versions of these files and copy the set_select functions in there. Then it would be no problem.

In my case I've just extended these files and overwritten only the set_select function. Again, thanks for this great fix!

@narfbg

Here's a reason of why it can't be merged - it's based on CI 2.0.3 and we're already trying to release a major new version. Nobody is really sure if the bug really exists anymore, we just don't close issues that we're not sure about. It's nobody's fault really - don't fight over that. :)

If you want to get this one going though, the least you'd need to do is update the docs part as we moved to a Sphinx-based one and this PR inserts raw HTML in files that no longer exist in the develop branch.

@basv

Ok will do.

@basv basv referenced this pull request May 25, 2013
Closed

Fixes issue #35 #2461

@basv

Wasn't sure how to get the updated files in this pull request so I added a new one:

#2461

@narfbg narfbg closed this Sep 23, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment