Resolved a bug with the form_hidden function when having multiple forms.

[jbrower]

Identical field names in the same page but diff forms would cause output to not be encoded properly.

[narfbg]

Isn't that fixed in the develop branch?

[jbrower]

That's quite possible. When I looked at the form helper, it looked like the static variable wasn't being cleared out on form close, which is the root cause of this problem. It's quite possible I missed it though.

[narfbg]

Yes, that function was indeed broken, but I'm pretty confident that this is already fixed in the development code. The following is its latest version:

    /**
     * Form Prep
     *
     * Formats text so that it can be safely placed in a form field in the event it has HTML tags.
     *
     * @param       string|string[] $str            Value to escape
     * @param       bool            $is_textarea    Whether we're escaping for a textarea element
     * @return      string|string[] Escaped values
     */
    function form_prep($str = '', $is_textarea = FALSE)
    {
            if (is_array($str))
            {
                    foreach (array_keys($str) as $key)
                    {
                            $str[$key] = form_prep($str[$key], $is_textarea);
                    }

                    return $str;
            }

            if ($is_textarea === TRUE)
            {
                    return str_replace(array('<', '>'), array('&lt;', '&gt;'), stripslashes($str));
            }

            return str_replace(array("'", '"'), array('&#39;', '&quot;'), stripslashes($str));
    }

[jbrower]

Looks like this wouldn't suffer from the bug as it isn't using a static variable at all. The old version had problems because the field name was being stored statically so that identical field names in the same form weren't sanitized again. I don't know if that behavior is desired or not. My changes preserve the static variable approach, bit this new function should work just as well, albeit potentially B2B slower since fields may be sanitized unnecessarily. The performance impact would likely be next to nothing however. That's how I see it through, and I could be wrong.

[narfbg]

Yeah, the old implementation was either seriously flawed or back in the day you needed to do a lot more than a simple str_replace() for some reason (and that supposedly was the reason to cache the data in a static var).

[jbrower]

Is there a particular reason why the new implementation isn't using the htmlspecialchars function?

[narfbg]

The code is pretty self-explaining - textarea elements only need less-than and greater-than to be escaped, while attributes (supposedly 'value' in our case) use quotes as the delimiter. Why would you need to use htmlspecialchars() for other characters?

[jbrower]

I have JSON in my value attributes. With those having quotes all scattered throughout, woudln't it be important to have the " in the JSON converted to the htmlsafe text?

[jbrower]

I looked at that again, yeah, that should work fine. (Sorry, it's early morning here.) It looks like my changes aren't needed anymore. I just wanted to make sure we weren't missing something somehow.

[narfbg]

OK, closing then.