Both these functions trim the incoming form value for the machine name.

But the machine name form elemtn won't let you put a space in a machine name anyway. If the user has JS turned off, then form_validate_machine_name() will produce a form error on the presence of the space also.

CommentFileSizeAuthor
#14 1876546-unnecessary_trim-d7.patch1.12 KBkevin morse
#7 1876546-Screenshot.png35.67 KBjoates
#6 [#1876546] Screenshot from 2013-01-29 15:48:44.png35.67 KBjoates
#4 unnecessary_trim-1876546-3.patch1.14 KBplopesc
#4 interdiff.txt672 bytesplopesc
#1 unnecessary_trim-1876546-1.patch1.14 KBplopesc

Comments

plopesc’s picture

plopesc
he/him
Primary language Spanish
commented
StatusFileSize
new unnecessary_trim-1876546-1.patch1.14 KB

Attaching patch that removes the unnecessary trimming.

Regards

plopesc’s picture

plopesc
he/him
Primary language Spanish
commented
Status: Active » Needs review

Changing status

joachim’s picture

joachim commented
Status: Needs review » Needs work 
+++ b/core/modules/node/content_types.inc
@@ -307,10 +307,10 @@ function _node_characters($length) {
-  $type->name = trim($form_state['values']['name']);
+  $type->name = $form_state['values']['name'];

I think we still need to trim the name, which is (I think!) the human-readable label. That should get a whitespace trim as a user convenience.

It's the 'type' value trimming that can be removed in both functions.

plopesc’s picture

plopesc
he/him
Primary language Spanish
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt672 bytes
new unnecessary_trim-1876546-3.patch1.14 KB

You're right.

Sorry for the mistake.

Re-rolling patch.

joachim’s picture

joachim commented

Thanks!

Looks good to me :)

joates’s picture

joates commented
StatusFileSize
new [#1876546] Screenshot from 2013-01-29 15:48:44.png35.67 KB

Note: attachment is broken (see #7)

joates’s picture

joates commented
StatusFileSize
new 1876546-Screenshot.png35.67 KB

confirm that the trim() works as required.

i added debug code to the form validation function..

<?php
dpm('[' . $type->type . '] [' , $type->name ']', 'validate');
?>

as you can see from the dpm() output i have also edited the machine-name field to add the "extra" space at the front to force it to fail validation.

There may be an additional issue that the machine-name starts and / or ends with underscores when whitespace exists in the name, do we need to address that side-effect or is it acceptable ? (since the machine-name still passes the validation test)

this screenshot does work..
Note: The page title of "Error" does not relate to this issue, it just showed up on my dev build after a git pull this morning.

joates’s picture

joates commented
Status: Needs review » Needs work
plopesc’s picture

plopesc
he/him
Primary language Spanish
commented
Status: Needs work » Needs review

Hello

In my opinion, the problem explained by joates is out of the scope of this issue.

The problem discovered not only affects to the node type form, because the machine name form is used in other pages such as image style creation.

In my opinion you should create a new issue related to machine_name form item validation to address this problem.

Regards.

joates’s picture

joates commented
Status: Needs review » Reviewed & tested by the community

agreed..

RTBC then, patch is good !!
(and side-effect of adding underscores to machine-name is irrelevant because the machine-name still passes validation).

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Yes this looks sensible. Committed/pushed to 8.x.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

David_Rothstein’s picture

David_Rothstein commented
Version: 8.x-dev » 7.x-dev
Status: Closed (fixed) » Patch (to be ported)

This isn't in Drupal 7 but has the "needs backport to D7" tag. I guess it's not that important to backport, but presumably it still could be.

kevin morse’s picture

kevin morse commented
Status: Patch (to be ported) » Needs review
StatusFileSize
new 1876546-unnecessary_trim-d7.patch1.12 KB

Here we go.

parthipanramesh’s picture

parthipanramesh commented
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

Latest patch does remove redundant trim command. Thank you!

David_Rothstein’s picture

David_Rothstein commented
Status: Reviewed & tested by the community » Fixed

Committed to 7.x - thanks! http://drupalcode.org/project/drupal.git/commit/9a0f281

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.