xoops_confirm should return html

[ggoffy]

if I use a template to show the content, I have a problem with xoops_confirm: xoops_confirm put the text with echo directly to the content area of my side, and this is normally at the end of my template, and if I use an own template footer, it is after this.

If we adopt function xoops_confirm like this:

function xoops_confirm($hiddens, $action, $msg, $submit = '', $addtoken = true, $return_html = false)
{
    $submit = ($submit != '') ? trim($submit) : _SUBMIT;
    $confirm = '<div class="confirmMsg">' . $msg . '<br>
          <form method="post" action="' . $action . '">';
    foreach ($hiddens as $name => $value) {
        if (is_array($value)) {
            foreach ($value as $caption => $newvalue) {
                $confirm .= '<input type="radio" name="' . $name . '" value="' . htmlspecialchars($newvalue) . '" /> ' . $caption;
            }
            $confirm .= '<br>';
        } else {
            $confirm .= '<input type="hidden" name="' . $name . '" value="' . htmlspecialchars($value) . '" />';
        }
    }
    if ($addtoken != false) {
        $confirm .= $GLOBALS['xoopsSecurity']->getTokenHTML();
    }
    $confirm .= '<input type="submit" class="btn btn-default" name="confirm_submit" value="' . $submit . '" title="' . $submit . '"/>
          <input type="button" class="btn btn-default" name="confirm_back" value="' . _CANCEL . '" onclick="history.go(-1);" title="' . _CANCEL . '" />
          </form>
          </div>';
    if ($return_html) {
        return $confirm;
    } else {
        echo $confirm;
        return void;
    }         
}

With

$confirm_delete = xoops_confirm(array('ok' => 1, 'id' => $Id, 'op' => 'delete'), $_SERVER['REQUEST_URI'], _FORM_SURE_DELETE, '', true, true);

now I can use $GLOBALS['xoopsTpl']->assign('confirm_delete', $confirm_delete) to put it anywhere in my template

any opinions?

[geekwright]

Well, I can understand the problem, but I've got some different thoughts.

First, the global functions such as xoops_confirm() need to disappear. They are a lot of similar functions, that also cause problems. They ultimately limit XOOPS and what it can become. They will go away.

Second, the fact that it returns HTML is in itself a problem. There is no xoops_confirm.tpl to control how it is expressed. It has been hacked to be a little better visually, but it still is an eyesore and a pain to the designers. Random functions in core shouldn't burp out HTML. This final expression should move as close to the presentation layer as possible.

Lastly, the trailing boolean parameter is usually a sign that things could be better. I've used them, we've all used them, but eventually they come back to bite you. Most QA tools will flag them as a code smell. Here is an article that talks about the issues in some detail.

I know, none of that solves your problem :smile: That is all just background on why I think we can do much better. Building the future on something broken just isn't a good strategy.

I would suggest we should create a real XoopsForm, built with real XoopsFormElements, to take the place of xoops_confirm().

The form code is right at the edge of presentation and domain logic. It already has the ability to be either displayed or assigned to template variables. Its presentation can already be modified by the theme in several ways. As a form, it could be further extended if needed. Logically, this is where it should reside -- a special purpose form.

[ggoffy]

I have solve this already by a separate class (see e.g. https://github.com/XoopsModules25x/modulebuilder/blob/master/class/Common/XoopsConfirm.php)

I think we can close this issue