[drupal-devel] [bug] Brackets should not be used in IDs (primary offender: form_ functions)

[Steven]

Issue status update for http://drupal.org/node/19986

 Project:      Drupal
 Version:      cvs
 Component:    base system
 Category:     bug reports
 Priority:     critical
 Assigned to:  killes at www.drop.org
 Reported by:  factoryjoe
 Updated by:   Steven
 Status:       patch

JonBob: while there can be a problem with file uploading, the id
attribute is used purely for theming and CSS/JS.


I didn't see this patch and already applied a more recent one [1]. It
is similar to this one, except that it only replaces ][ in the id. Feel
free to submit a patch there with a regexp if you think it is better.


Note that you should replace a string of /consecutive/ disalowed
characters with a single dash. This is better for readability.
[1] http://drupal.org/node/23516




Steven



Previous comments:
------------------------------------------------------------------------

April 5, 2005 - 00:41 : factoryjoe

Attachment: http://drupal.org/files/issues/blah.html (1.13 KB)

Square brackets in IDs are not valid XHTML and cause a host of problems
when trying to style individual elements. I have attached a page that
proves this.


Additionally, when you try to style an element by an ID that contains
brackets, *all other styling from that rule will be ignored*. Brackets
are simply not valid characters for IDs [2] (/emphasis added/):



There's also a difference between CLASS and ID attributes as regards to
the allowable format of values. By HTML rules, CLASS is CDATA, which
basically means 'anything goes', but CSS rules impose some
restrictions. /And by HTML rules, ID is ID, which means that only
letters of the English alphabet ([A-Za-z]), digits ([0-9]), hyphens
("-"), underscores ("_"), colons (":"), and periods (".") are
permitted, and the first character must be a letter (and underscores
don't work reliably)/


"
If you validate the attached document, you will get these two errors:


XHTML validation error

Line 25, column 19: character "]" is not allowed in the value of
attribute "id"
<div id="edit-16188][weight">

CSS validation errors

Line: 17 Context : #edit-16188
Parse Error - #edit-16188][weight {
	    border: 2px solid blue;
	  }



Line: 20 Context : #edit-16188
Parse Error - #edit-16188][weight, #test-2 {
	    border: 2px solid green;
	  }




[2] http://css.nu/faq/ciwas-aFAQ.html#QA14




------------------------------------------------------------------------

April 5, 2005 - 01:50 : killes at www.drop.org

Attachment: http://drupal.org/files/issues/valid-forms.patch (5.95 KB)

Ok, here is an initial patch. The patch needs testing.


I introduced a new function _form_get_id that is supposed to replace
any non-allowed characters by a dash. This is not really nice, as we
will get CSS IDs that have double dashes, but it is valid. We cannot
simply reduce the double dashes to single dashes because there already
might be another ID with only a single dash.




------------------------------------------------------------------------

April 5, 2005 - 02:29 : gordon

This will tidy up the id's of the fields. I have seen this esp. in
writing htmlarea as I need to manipulate the ids to remove the [] and
other chars.


When this is applies please inform Me, and the Authors of FckEditor,
and TinyEd as this patch will break the gui editors in some places.
This is not a minus, but I myself will like to avoid having the
htmlarea module broken for no longer than it needs to.




------------------------------------------------------------------------

April 5, 2005 - 10:39 : Bèr Kessels

can we not expand this into a general ID checker. So that we can be
assured of unique ids on any page?


Bèr




------------------------------------------------------------------------

April 5, 2005 - 15:52 : killes at www.drop.org

It would be possible to do this, yes. We'd need to ensure that all IDs
are generated by this function and save the IDs in a static var and do
something if the ID is already taken. However, this coul dlead to
different IDs for the same tag on different pages. So it isn't a really
good idea. it is mandatory that Drupal ets fixed before the 4.6 release.
So please somebody test the patch: :)




------------------------------------------------------------------------

April 5, 2005 - 16:01 : JonBob

The problem does go a little deeper than this. There are several places
in the code where it is assumed that form field names are scalars
rather than arrays. Typically, we get around this with the ugly
'array][field' syntax, and that causes problems for IDs to be sure. But
it also confuses other APIs that take form element names, such as
file_check_upload(). You can't pass an 'array][field' name to this
function and have things work.


I think we need a more global solution that gets away from the
confusing bracket sequence entirely. Perhaps allow an array of strings
to be passed to the form_ functions, so we could do
form_textfield('foo', array('foo', 'bar')) instead of
form_textfield('foo', 'foo][bar').