Fixed Template macro public:color_picker_macros:color_picker_row mishandling of arg-required

MaGeFH

Active member
Affected version
DP10
The macro specifies the argument as optional with a default value of false: arg-required="false"

However, if the macro invocation omits the arg-required parameter the generated HTML code looks like this:

HTML:
<input type="text" class="input" name="fh_ad_type_options[color_url]" required="false">


This does not work (at least in Chrome 59.0.3071.115 (64-Bit) and Firefox 54.0.1 (64-Bit) on Mac OS 10.12.6) however. This is correct according to the spec.

required_false_oh_noes.webp


In order to get an optional color picker into the form, you would need to do this:

HTML:
<xf:macro template="public:color_picker_macros" name="color_picker_row"
    arg-name="fh_ad_type_options[color_url]"
    arg-label="{{ phrase('fh_ad_advert_type_afscustom_color_url') }}"
    arg-value="{$ad.type.options.color_url}"
    arg-required=""
    arg-allowPalette="true" />


I would suggest changing the macro, but that would only fix it for this element. I dug a bit deeper and found that you don't seem to handle boolean attributes properly in XF\Template\Templater::processUnhandledAttributes.

Something like this would fix it. Quick'n'dirty test code, you have been warned... ;)

PHP:
if (in_array($name, ['required', 'checked', 'disabled', 'selected']))
{
    if ((false === (bool)$value) OR ('false' === strval(strtolower(trim($value)))))
    {
        continue;
    }
}
 
Ultimately our template tags can handle more complicated values because they are processed in PHP first, so you end up with situations where 'false' == true. I think we've potentially talked about changing this in at least some cases, but for now it's just a case of being very careful to use the intended value. As our template syntax supports proper value types, it's generally the case that we should be using {{ false }} rather than string "false".

So, I've updated this template and you should now just be able to omit the arg-required entirely, and it should properly default to false.
 
Top Bottom