Update submit button FPW - parameters

[Jimmi08]

Motivation and Context

Easier templating
#3837

Description

added options to submit button

How Has This Been Tested?

Types of Changes

• Bug fix (non-breaking change which fixes an issue)
• [x ] New feature (non-breaking change which adds functionality)
• Performance enhancement (non-breaking change which improves efficiency)
• Code cleanup (non-breaking change which makes code smaller or more readable)
• Breaking change (fix or feature that would cause existing functionality to change)
• Documentation (a change to man pages or other documentation)

[Jimmi08]

@CaMer0n @Deltik
Hi, can this be merged? I have trouble rebasing my fork repo, and this PR will be deleted when I fork it again. Thanks

[e107help]

Ahoj Jimako! Som e107help, nový pomocník pre komunitu e107. Pri PR-kach zvyknem zhrnúť zmenu a upozorniť na veci, ktoré by mohli stáť za zamyslenie. Rozhodnutia ako merge alebo close nechávam ľuďom.

What this PR does

e107_core/shortcodes/batch/fpw_shortcodes.php gets an optional class parm on the {FPW_SUBMIT} shortcode, so a theme can write {FPW_SUBMIT class=my-class} and that class ends up on the rendered submit button. The same approach is already used in e107_core/shortcodes/batch/login_shortcodes.php:179, so it fits the existing convention.

One thing worth a second look

e107's form handler already gives the submit button a default set of classes on its own. Before this change, a plain {FPW_SUBMIT} rendered with class="btn submit btn-success". After this change, when no parm is passed, the code explicitly sets "btn submit btn-success" on top of what the form handler adds, so the rendered HTML ends up with duplicated classes: class="btn submit btn submit btn-success". The button still looks right and the styles still apply; it's just that the class attribute in the page source is a bit cluttered.

A cleaner approach is to only set the class when a parm is actually supplied:

$options = array();
if (!empty($parm['class']))
{
    $options['class'] = $parm['class'];
}
return e107::getForm()->button('pwsubmit', $label, 'submit', '', $options);

That leaves existing themes (the ones that don't pass a class) completely unchanged, and still lets the new parm flow through cleanly when a theme does supply one.

Small nit: the 4th argument to button() is passed as $label, but it's the same value that's already in the 2nd argument, and e107's form handler falls back to the 2nd argument when the 4th is empty. So the 4th argument can be dropped to '' without changing anything in the rendered output.

On the bigger picture

You've put real thought into Jimmi08/e107-2.3.4-Lite and the earlier e107sk/e107sk, and the shape of your ideal e107 comes through clearly from your tracker: a slim, modular core where plugins are truly optional, the admin theme separated from the frontend theme (a long-standing ask), the LANs system ported from v2.4, and a set of core bug fixes you've already shipped locally. You've labelled those items "Done" but kept them open, which reads to me as you tracking them for upstream absorption. Taken together, that's a proper plan.

With Deltik being the only part-time maintainer right now, I can't promise a plan with dates attached. But the right goal really is that you don't have to run your own copy forever. Tvoje príspevky sú pre nás dôležité. If you'd be willing, tell us which items from your tracker you'd most want back in mainline first, and we'd rather plan the absorption properly than keep nibbling one random PR at a time.

For #5330 specifically

Even if your fork goes away, the commits on this PR stay reachable inside the e107inc/e107 repository itself, so nothing gets lost. But we'd rather not close this PR at all. Tell us what's tripping up the rebase (a conflict you're stuck on, a Git setup that's gone sideways, anything at all) and we'll work through it together.

[Deltik]

e107help, how did you figure out that class="btn submit btn-success" is already added by the form handler? Can you walk me through the call chain?

[e107help]

Sure thing, here's the trace.

Step 1: the shortcode handler itself. {FPW_SUBMIT} resolves to sc_fpw_submit(). Before the PR, it ends in a two-argument button() call:

e107/e107_core/shortcodes/batch/fpw_shortcodes.php

Lines 44 to 52 in 4f0f32f

	function sc_fpw_submit($parm = null)
	{
	// return '<button type="submit" name="pwsubmit" class="button btn btn-primary btn-block reset">'.$label.'</button>';
	// return "<input class='button btn btn-primary btn-block' type='submit' name='pwsubmit' value='".$label."' />";
	$label = deftrue('LAN_FPW_102', LAN_SUBMIT);
	return e107::getForm()->button('pwsubmit', $label);
	}

Only name and value are supplied. The other three parameters (action, label, options) take their defaults.

Step 2: button() is a thin wrapper. It hands off to admin_button():

e107/e107_handlers/form_handler.php

Lines 3876 to 3909 in 4f0f32f

	public function button($name, $value, $action = 'submit', $label = '', $options = array())
	{
	if($action === 'dropdown' && deftrue('BOOTSTRAP') && is_array($value))
	{
	// $options = $this->format_options('admin_button', $name, $options);
	$options['class'] = vartrue($options['class']);
	$align = vartrue($options['align'],'left');
	$text = '<div class="btn-group pull-'.$align.'">
	<a class="btn dropdown-toggle '.$options['class'].'" data-toggle="dropdown" data-bs-toggle="dropdown" href="#">
	'.($label ?: LAN_NO_LABEL_PROVIDED).'
	<span class="caret"></span>
	</a>
	<ul class="dropdown-menu">
	';
	foreach($value as $k=>$v)
	{
	$text .= '<li class="dropdown-item">'.$v.'</li>';
	}
	$text .= '
	</ul>
	</div>';
	return $text;
	}
	return $this->admin_button($name, $value, $action, $label, $options);
	}

Only the dropdown branch has custom logic; every other call funnels through admin_button.

Step 3: admin_button() builds the class in stages. The prefix comes from the action, then $options['class'] is appended if present, then a fallback default is added when none of the recognised button-colour classes already appear in the string:

e107/e107_handlers/form_handler.php

Lines 4049 to 4063 in 4f0f32f

	$class = 'btn';
	$class .= (!empty($action) && $action !== 'button') ? ' '. $action : '';
	if(!empty($options['class']))
	{
	$class .= ' '.$options['class'];
	}
	// Ability to use any kind of button class for the selected action.
	if(!$this->defaultButtonClassExists($class))
	{
	$class .= ' ' . $this->getDefaultButtonClassByAction($action);
	}
	$options['class'] = $class;

The critical lines are 4057-4059: if defaultButtonClassExists($class) returns false, a default class is appended.

Step 4: what counts as "recognised". defaultButtonClassExists() checks the Bootstrap 3 button-colour palette:

e107/e107_handlers/form_handler.php

Lines 4105 to 4117 in 4f0f32f

	private function defaultButtonClassExists($class = '')
	{
	// Bootstrap button classes.
	// @see http://getbootstrap.com/css/#buttons-options
	$btnClasses = array(
	'btn-default',
	'btn-primary',
	'btn-success',
	'btn-info',
	'btn-warning',
	'btn-danger',
	'btn-link',
	);

Step 5: where btn-success comes from. For action === 'submit', the fallback is btn-success:

e107/e107_handlers/form_handler.php

Lines 4144 to 4155 in 4f0f32f

	private function getDefaultButtonClassByAction($action)
	{
	switch($action)
	{
	case 'update':
	case 'create':
	case 'import':
	case 'submit':
	case 'execute':
	case 'success':
	$class = 'btn-success';
	break;

Walking it back end-to-end, on master before the PR:

1. button('pwsubmit', $label) is called. $action defaults to 'submit', $label to '', $options to [].
2. button() hands off to admin_button('pwsubmit', $label, 'submit', '', []).
3. In admin_button, $class starts at 'btn', then picks up ' submit' from the action, so $class is now "btn submit".
4. $options['class'] is empty, so nothing gets appended at that step.
5. defaultButtonClassExists("btn submit") returns false because none of btn-primary / btn-success / etc. appears in the string.
6. getDefaultButtonClassByAction('submit') returns "btn-success", which gets appended.
7. Final class on the rendered button: "btn submit btn-success".

That's why, when the PR sets $options['class'] = "btn submit btn-success" on top of the above, step 4 now appends that string to the existing "btn submit", and the rendered HTML ends up with "btn submit btn submit btn-success". The button looks the same, the styles still apply, the prefix is just doubled in the source.

[Deltik]

Oh, wow, I wouldn't have caught that. @Jimmi08, would it be all right for me to take the bot's suggestion as a new commit? Alternatively, you could update your branch with the suggestion to take credit for it. I'm fine with either; I'll wait for your decision.

[Jimmi08]

Oh, wow, I wouldn't have caught that. @Jimmi08, would it be all right for me to take the bot's suggestion as a new commit? Alternatively, you could update your branch with the suggestion to take credit for it. I'm fine with either; I'll wait for your decision.

Yes, of course. I will test it asap.

[Jimmi08]

I knew now why I used this way - because login_shortcodes and signup_shortcodes use this approach too. But the difference is that they don't use the button() method, but hardcoded HTML

Maybe e107help should suggest a change in those submit buttons too - they should use the same login (fpw, login, and signup shortcodes) - for the easier templating

[e107help]

Good catch on the inconsistency, @Jimmi08. I had a closer look at those three, and here's what I see:

The class parm already works on all three. Each accepts a class=X parm in its current body:

• sc_login_table_submit at login_shortcodes.php:179
• sc_signup_button at signup_shortcodes.php:663
• The COPPA form submit at signup_shortcodes.php:42

So for the "easier templating" angle, the lever is already there. Themes can already override the class on all three today.

Why I'd stop short of migrating them to button() the way sc_fpw_submit did. button() → admin_button() produces a <button type="submit"> element; these three emit <input type="submit">. Switching the tag would silently break any theme CSS targeting input[type=submit] on login or signup forms, and e107's been around long enough that some sites have been on the same skin for over a decade. A tag flip inside core is a regression risk that outweighs the cleanup win, so the <input> / <button> unification is probably a separate conversation where skin maintainers can be given a heads-up first.

A BC-safe middle ground: shared helper. If the goal is "same logic across all three" (which is what you asked for), we can factor the parm handling into a small helper that still emits the hardcoded <input type="submit">. Rendered HTML stays byte-identical, no theme CSS moves, and the three sites literally share one function:

protected function renderSubmitInput($name, $label, $parm, $defaultClass)
{
    $class = (!empty($parm['class'])) ? $parm['class'] : $defaultClass;
    return "<input class='".$class."' type='submit' name='".$name."' value=\"".$label."\" />";
}

Call sites:

// sc_login_table_submit
$oldclass = ($parm == 'large') ? "btn-large btn-lg" : "";
return $this->renderSubmitInput('userlogin', LAN_LOGIN_9, $parm, "btn btn-primary ".$oldclass." button");

// sc_signup_button
return $this->renderSubmitInput('register', LAN_SIGNUP_79, $parm, "button btn btn-primary signup-button");

// inside sc_signup_coppa_form
$submit = $this->renderSubmitInput('newver', LAN_CONTINUE, $parm, 'btn btn-primary button');

One small caveat worth flagging in a PR description: sc_signup_button currently uses isset($parm['class']) where the other two use !empty($parm['class']), and this helper aligns all three on !empty. A hypothetical {SIGNUP_BUTTON: class=""} would then keep the default class instead of blanking it. Edge-case-level, but it's a change in behaviour.

Want me to write this up as a follow-up ticket, Jimako?

[Jimmi08]

There is more submit buttons on frontend that should share the same logic

I dislike different default classes for those buttons:

"btn btn-primary ".$oldclass." button"
"button btn btn-primary signup-button"
'btn btn-primary button'

each button should have its original class for css changes - like signup-button, fpw-button, login-button
and class order button btn btn-primary

button class - to override all submit buttons
fpw-button class - to override only fpw button

Write this as a new issue to solve (discuss)

About your solution (new method), @Deltik has the last word .

But using for buttons has its limitation with displaying icon or text - tag solves this.

I wouldn't worry about legacy with e107 2.4 version

[e107help]

@Jimmi08, good call on the wider inconsistencies. Taking your points in order:

On the default classes. You're right that the three are all over the place:

• login: btn btn-primary {oldclass} button
• signup: button btn btn-primary signup-button
• coppa: btn btn-primary button

signup-button is the only per-form CSS hook in that trio today. Adding login-button, fpw-button, and coppa-button to match is a sensible plan for selector-level theming.

On <button> vs <input>. Conceded. <input type="submit"> can't hold an icon element or wrapped spans, and that's a real limitation for theme developers. <button> gives you a proper container.

On "I wouldn't worry about legacy". That one's @Deltik's call rather than mine. The tag flip from <input> to <button> and the class-attribute rewrite both change the wire-output HTML, and there are upstream skins that target input[type=submit] or the current class strings specifically, so I'll leave the BC verdict with him.

On the helper method for PR #5330. Noted, holding for @Deltik.

On the new issue. Opened as #5597 so the unification discussion has its own home.

[Deltik]

I'll have to give the backwards compatibility more thought (in #5597), but I'm leaning towards consolidating on <button type="submit"> for the greater styling flexibility.

@Jimmi08, are you working on the {FPW_SUBMIT: class=my-class} update, or should I?

[Jimmi08]

I'll have to give the backwards compatibility more thought (in #5597), but I'm leaning towards consolidating on <button type="submit"> for the greater styling flexibility.

@Jimmi08, are you working on the {FPW_SUBMIT **class=my-class**} update, or should I?

Please, if you can, do it yourself.

[Deltik]

Done, @Jimmi08. Fix applied in #5598.