nliautaud/formsimple

Simply create forms

formsimple's Issues

Better handling of missing language files or strings

The code that retrieves translations assumes that at least the en.php file exists. If the file doesn't exist, the function does not return any value. It should display a warning or something (eventually, this check / warning for basic assumptions should be made upon instantiation of the main FormSimple class)

If the translation isn't found in that language, the requested key gets sanitized and gives no evident clue to the end user that a string is actually missing - instead it would appear as if it picked the English string.

Depending on the requested key, the displayed text could be misleading or imprecise. Asking for the translation of form_error_token, for instance, it would return Form error token, standing for Already sent message.

Personally, I would return {FormSimple/form_error_token} instead (or even better, {FormSimple/xx_YY/form_error_token}, where xx_YY is the language code, as sometimes end users sloppily communicate errors without providing enough context).

I also think that the function in here:
https://github.com/nliautaud/FormSimple/blob/master/actions/action.php#L81
should simply make use of this one:
https://github.com/nliautaud/FormSimple/blob/master/FormSimple.php#L373
assuming the language files get merged as suggested in issue #5.

Consistency & conventions

First things that should be applied.
See branch pear-formatting.

• trailing spaces
• indentation
• camelCase
• lowercase true, false & null
• control structures format
• remove ?> at eof
• ?

Ambiguous constant prefix

In order to avoid any conflict with other GetSimple code, constants should be prefixed with a longer string. I suggest changing FS to FORMSIMPLE_ everywhere in the code.

https://github.com/nliautaud/FormSimple/blob/master/FormSimple.php#L63-L78
https://github.com/nliautaud/FormSimple/blob/master/handles/FormSimple_gs.php#L12-L19

Enabled / disabled behavior

I am following the path of testing locally the various combinations after the fixes I've made on my copy of the pear-formatting branch, hunting for more problems before implementing new stuff.

Now testing the form after disabling the action execution from the settings panel I get a notice about an undefined variable in this line:
https://github.com/nliautaud/FormSimple/blob/master/FormSimple.php#L255

I would simply fix it by adding a "d" at the end as it obviously should be $this->enabled and not $this->enable, as for the declaration here:
https://github.com/nliautaud/FormSimple/blob/master/FormSimple.php#L25
but I'm not sure I understand the intention behind that chunk of code.

Assuming I fix it, $this->enabled normally returns true, being its default value:
https://github.com/nliautaud/FormSimple/blob/master/FormSimple.php#L50

That means that $this->message('form_error_disabled'); gets raised if and only if the panels' setting returns false and if the actual class instance is enabled, as for the following chunk:

if (!FormSimple::setting('enable') && $this->enabled) {
    // FormSimple is disabled
    $this->message('form_error_disabled');
}

If I change the test to: !FormSimple::setting('enable') || !$this->enabled then the error would be raised if either of the settings return false; would that reflect the original intention?

GetSimple i18n guidelines compliance

In order to ease the life of our translators and to allow end users to easily change whatever string they'd like to, the code should comply with:
http://get-simple.info/wiki/plugins:i18n
this would let the plugin strings get translated with this plugin:
http://get-simple.info/extend/plugin/translate/112/
or any other compliant tool.

Necessary action:

• rename $FormSimple_lang to $i18n

Strongly suggested action:

• delete all language files into FormSimple/actions/sendmail/lang and merge them with those into FormSimple/lang
  (I don't think we should expect the translation tools to scan the whole plugin folder looking for additional lang folders, and also, this would ensure that, at least at in the context of this plugin, there are no duplicate strings, something which I have fought with using a well known CRM system "out there", and which almost drove me crazy trying to fix its translations)

Optional action:

• rename all the files to use the en_US form instead of just en, to allow for differentiation of British / American / Australian, Portuguese / Brazilian and the alike.

Internal variables polluting global namespace

$FSCOUNT:
https://github.com/nliautaud/FormSimple/blob/master/FormSimple.php#L87

$FormSimple_lang:
https://github.com/nliautaud/FormSimple/blob/master/FormSimple.php#L395
https://github.com/nliautaud/FormSimple/blob/master/actions/action.php#L84

$FormSimple_settings:
https://github.com/nliautaud/FormSimple/blob/master/FormSimple.php#L655

$$name:
https://github.com/nliautaud/FormSimple/blob/master/FormSimple.php#L735

$$settings:
https://github.com/nliautaud/FormSimple/blob/master/actions/action.php#L67

They should be all refactored to static variables, class members or the alike to avoid overwriting or conflict, and to enforce data isolation.

FormSimple::setting() missing return value (and proper error handling) for non-existing settings

https://github.com/nliautaud/FormSimple/blob/master/FormSimple.php#L650

Hard-coded value for message field length check

https://github.com/nliautaud/FormSimple/blob/master/fields/message.php#L12

FormSimple::config_edit() misplaced return value

https://github.com/nliautaud/FormSimple/blob/master/FormSimple.php#L874
The return False; statement should be moved outside of the IF block to always return an actual boolean.

FormSimple::last_version() missing return value and assuming successful get & decode

https://github.com/nliautaud/FormSimple/blob/master/FormSimple.php#L612
The function makes assumptions about the URL getting the correct data, properly formatted, and omits to handle the unsuccessful status case (the function exits without returning any data).

The failure case should also be handled here:
https://github.com/nliautaud/FormSimple/blob/master/FormSimple.php#L626

Helper function with common name polluting global namespace

The unset_r() helper function has a too common name and pollutes the global namespace allowing for name clashes:
https://github.com/nliautaud/FormSimple/blob/pear-formatting/FormSimple.php#L919