fields : do a pass of TODOs checking
yched - February 10, 2009 - 20:14
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | field system |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs work |
| Issue tags: | Fields in Core |
Description
From webchick in #369562: Field: Trivial cleanup
"it would be wonderful to have a fields in core sprint attendee do a "TODO" pass and just make sure the list matches our actual remaining todos"
#1
#2
I'll take this on and post what I find when I'm done.
#3
I updated my view of HEAD a few minutes ago and there are 89 occurrences of "TODO" in the Field module. Now I need to know where the current TODO list is located so I can compare. Anyone? Thanks in advance!
#4
bjaspan (over IRC) thought that this issues is more involved than a 2. He suggested I look through the TODO and fix what I can and post patches, so that will be my approach. As possible, I will look to comparing with the issue queue open issues at http://drupal.org/project/issues/drupal?status=Open&priorities=All&categ..., but as he suggested, that may be a bit much for me as a newcomer...
#5
I'm not getting to this in a timely fashion, and many of the TODO comments require advanced knowledge of the Field API, who is working on what, etc... So I am returning this one to the larger group.
#6
Not sure whether this is the proper issue, but here is one patch for one of the TODOs: the one about field name length.
#7
In addition, fixing this made me wonder: currently, nothing prevents field_create_field from creating fields with names starting with a number instead of [a-z_]. Shouldn't this be prevented ?
#8
The patch looks good. Can we just add a corresponding test in 'Field CRUD tests' ? I think there's a
// TODO: test other failuresline in there.You're perfectly right that we should prevent field names starting with a number. It would turn into invalid $object->1_foo properties and $1_foo variable names in templates.
Care to roll that in your patch ?
#9
Here is a revised version. It now checks the start character.
I've also added simpletest for illegal field names (non-alpha first char, and non-alnum other char).
I've also added a test for the field name length, however here I ran into a limitation of the simpletest mechanism (see patch for details) and had to remove that test. Not sure there is an actual solution, and it would likely have to be at the simpletest level anyway.
#10
Heh, true... well, I think we need to forget about that specific test, then. Not *that* bad. Let's just leave a comment on why we don't test this...
Other than that, looks ready to me.
#11
Rerolled with just a comment instead of the test.
#12
Great, only minor nitpicking now.
+++ modules/field/field.crud.inc 4 Aug 2009 20:00:56 -0000@@ -208,11 +208,17 @@
+ // We use drupal_strlen because the DB layer assumes that column widths
+ // are given in characters, not bytes.
+++ modules/field/field.test 4 Aug 2009 20:00:57 -0000
@@ -1296,6 +1296,33 @@
+ // the length of which exceeds MySQL max schema item size of 64 bytes, causing
Comments should wrap at 80 characters. The 1st line above is too short, the other too long ;-)
+++ modules/field/field.crud.inc 4 Aug 2009 20:00:56 -0000@@ -208,11 +208,17 @@
+ // Field name cannot be longer than 32 characters
+++ modules/field/field.test 4 Aug 2009 20:00:57 -0000
@@ -1296,6 +1296,33 @@
+ // Check that field name must start with a letter or _
...
+ // Check that field name must only contain lowercase alnum or _
Missing period at the end of the sentence. Also, I'm not sure 'alnum' abbreviation is good practice, we usually avoid abbreviations in comments.
This review is powered by Dreditor.
#13
Hm, I might have replied a little too fast in #10. Actually I don't understand why we couldn't test that a field name > 32 chars fails : it's supposed to raise an exception before it reaches the point where a table would be created, so we shouldn't care that MySQL would fail to create the table because of simpletest prefix ?
#14
It fails if we test it with a name that should be valid, because it is 31 characters long: in that case, the assertion succeeds and processing continues to create the field, triggering the mysql error because the table name is now 65 (minimum) characters long. This is not demonstrated by the test in its current state, though, only by the "positive" test which I wrote and removed before committing, which tests that creation succeeds below 32 and fails above, as it succeeds only below 30 or 29 depending on the number of previously created fields (number of digits in the field fid).
#15
Well, there's no need to test that field_name < 32 char works, the whole field.test demonstrates that ;-)
We just want to check that > 32 fails with a FieldException.
#16
Here it is with the test reinserted and commenting style fixed.
#17
Thanks. Just fixed a couple minor points (missing trailing periods, trailing whitespaces) + grouped all failure checks at the bottom of testCreateField().
Other than that, RTBC.
Thanks !
#18
Dries committed #17.
Back to active for the rest of the TODOs :-)
#19
Taken into account active/inactive field and instances.
#20
Adding Fields in Core tag.
#21
Looks fine by me. Thks fgm !
#22
Committed to CVS HEAD. Setting back to 'code needs work' for the remaining TODOs. Thanks fgm.
#23