Please specify in the help text at users interface that form_id is regards to value and not id

Thanks

Comments

soxofaan’s picture

I assume you tried to enter a from ID as "foo-bar-form", while it should be "foo_bar_form".
I'm not sure we should put this information in the description text of the admin UI. It would take a fair amount of text to explain (view source, search for form, get id attribute, ...). Moreover, viewing in the markup is not the only way to get a form ID, so the description would only cover one use case.

I would take another approach. Now, when you enter "foo-bar-form", you get in that case a rather spartan message "Illegal form_id".
I would suggest to change this error message to make it more descriptive and helpfull, e.g.:

Illegal form ID. The form ID should consist of alphanumeric characters and underscores only (no spaces, no hyphens). You entered "foo-bar-form", but maybe you meant "foo_bar_form"?

any thoughts?

treksler’s picture

+1

cafuego’s picture

Version: 6.x-2.0 » 6.x-2.x-dev
StatusFileSize
new2.08 KB

I totally concur, it had me stumped for ages too, to the point of writing a blog about it when I finally got it working. Someone just pinged me after finding my blog and solving their issue and suggested I write a patch.

The patch in question is attached.

It adds a single line to the admin form, explaining that a form_id must be only alphanumeric characters and underscores. The validation function can now detects alphanumeric characters with hyphens and in that case will suggest a form_id with hyphens replaced with underscores.

The default error message is now less spartan and also re-iterates the need for only alphanumeric characters and underscores.

cafuego’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 633938.patch, failed testing.

soxofaan’s picture

Status: Needs work » Needs review
StatusFileSize
new2.2 KB

Great, thanks cafuego!

There seemed to be a syntax error in the path, which triggered to testbot to complain.
Fixed patch in attachment.

About the wording: English is not my mother tongue, but I would say "A form_id must only consist of ..." instead of "A form_id must consist of only ...". Any thoughts on this?

I think this feature definitely needs some test coverage too. E.g. things to test:
"foo_bar" -> accept
"foo-bar" -> suggest "foo_bar"
"foo_bar-baz" -> suggest "foo_bar_baz"
"FOO_BAR-BAZ" -> suggest "foo_bar_baz"
"foo$bar" -> deny

Status: Needs review » Needs work

The last submitted patch, 633938_wrong_form_id_suggestion_02.patch, failed testing.

cafuego’s picture

Status: Needs work » Needs review
StatusFileSize
new2.51 KB

I think the test bot is on crack.

About the wording: English is not my mother tongue, but I would say "A form_id must only consist of ..." instead of "A form_id must consist of only ...". Any thoughts on this?

It wasn't wrong per sé, but it wasn't very good english words ;-) Now reworded now as "A form_id may only contain lowercase alphanumeric characters and underscores."

I added support to test for uppercase characters and mixed hyphen/underscore use. The error message will show the entered form-id and the suggested form_id.

I've not looked at test coverage yet, though I did test the pregs used:

<?php
  $form_ids = array('I_should_FAIL', 'i_should_pass', 'I-SHOULD-FAIL', 'I_should-FAIL', 'i-should-fail', 'I_SHOULD_FAIL', 'rubbish=data?');

  foreach ($form_ids as $form_id) {
    $matches = array();
    if (preg_match('/^[a-z0-9_]*$/', $form_id)) {
      echo "PASS: $form_id\n";
    }
    else if (preg_match('/^[A-z0-9_\-]*$/', $form_id)) {
      echo "FAIL: $form_id\n";
      echo "TRY: " . strtolower(strtr($form_id, '-', '_')) ."\n";
    }
    else {
      echo "FAIL: $form_id\n";
    }
  }
cafuego’s picture

I think the test bot is on crack.

Actually, it's not. I was missing a } in my if / else block.

soxofaan’s picture

StatusFileSize
new4.72 KB

worked a bit on error handling code and added tests

Status: Needs review » Needs work

The last submitted patch, 10: 633938_wrong_form_id_suggestion_04.patch, failed testing.

wundo’s picture

Issue summary: View changes
wundo’s picture

Status: Needs work » Closed (outdated)