Moshe's observations on D6 CCK

Update: I enabled comments in case people prefer that: Moshe

I applied yched's formatter patch and then wrote down my list of bugs, feature requests, and questions for D6 CCK. I have not looked at much code yet. I plan to submit patches for many of these, and thus learn the new design intimately. I'll get started tonight. I would appreciate if Karen/yched/all could add comments and hints to each item as needed. I don't want to head down rat holes that you've already travelled.

Documentation about planned and actual changes: http://drupal.org/node/191796, http://drupal.org/node/177892. (Note that the documentation may not have kept up with the code). (Karen)

admin/content/node-type/article/fields

1. menu settings default to above the body field

2. there are a lot of fieldsets not movable here (comment settings, authoring info, url path, revision, book outline, publishing options)
   ** There were lots of problems trying to get drag 'n drop working with all those other fields, so we simplified it, see http://drupal.org/node/203957 (Karen)
   ** In CCK for D5, only elments with a weight between -10 and 10 were listed. Kept out comment settings, author info et al...
   We now decide ourselves the list of core-defined elements that should make it in the list (contrib modules can use a hook to make their own additions available too - er, still to be documented), and I more or less kept the same elements (title, taxo...). We can definitely add stuff if we feel it's needed. But is it ? (Yched)
   ** MW: makes sense. I would add book outline, file attachments, and url path since those are somewhat end user.

admin/content/node-type/article/add_field

1. I much dislike that a field's machine name cannot be changed. We should offer a hook so other modules can react like core does for content type rename. We can keep a field like 'orig_type' which holds the first name. See core's content types.
   ** Changing the field name affects everything -- field tables, node tables, node table names, views fields, etc. etc. Very messy (Karen)
   ** Being able to change core types machine name raised a lot of cursing at various places in the issue queue... Not too hot on this either (Yched)
   ** MW: No argument that it is hard. It is worth it though. Unfortunately, it might have to be Karen or yched that does this to avoid even more breakage than needed.

2. We should recommend either decimal or float because few people know the difference.
   ** The float has to be retained because that is what was there before. The decimal field has been long requested and is only now available. I asked on the development list if anyone knew any reliable way to convert the existing float data to decimal with no risk of loss and no one had any answer, so I kept the existing floats and added decimals. (Karen)

3. UI looks a bit awkward
   ** ?? Not sure what that means (Karen)
   **MW: the descriptions look a bit odd but any other solution is a lot of work for little gain.

4. "notice: Array to string conversion in /Users/mw/htd/pr/includes/bootstrap.inc on line 706. warning: preg_match() expects parameter 2 to be string, array given in /Users/mw/htd/pr/includes/bootstrap.inc on line 709." when creating a decimal or integer with checkboxes widget.
   ** MW: Reported with analysis to http://drupal.org/node/219566

admin/content/node-type/article/display

1. ideally, a multiple list of node titles renders as a UL or OL and not as a series of [div]. This would make less sense for formatters like 'teaser' and 'full node'.

** Now that field theming is done through a tpl file, I'd be willing to provide more flavours. ul, ol, div, comma separated...
Our current field.tpl.php is quite ugly anyway, because it accounts for all display options in one file. Splitting it in separate straightforward templates wouls be nice. (Yched)

admin/content/node-type/article/fields/field_related

1. 'required' would ideally be a widget setting

2. Number of values causes data loss only on decrease? Does a user have to provide the required number or is this a maximum? or minimum? NEITHER! this appears to control how many fields are shown initially and upon Preview! Got NOTICE when used with text/checkboxes and unlimited.
   ** The number used to work -- exactly what you set unless you mark it unlimited. Did something break recently?? (Karen)
   ** The natural evolution for the 'number' of fields would be min values and max values. Not completely trivial though, this will wait for a minor release.

3. append (one row) and (multiple rows) to the description of Text Field and Text Area

4. I would make 'Allowed Values' into a collapsed fieldset and include the PHP in there. Current help text and UI is awkward.
   Done. (I kept the 'PHP code' collapsed fieldset, this mirrors the 'Default value' settings)

5. When a friendly name for a field is changed, we need to clear the menu cache since these appear in subtabs.
   ** See http://drupal.org/node/207499 (Karen)
   ** I removed the subtabs for now. Those were not intentional. (Yched)

node/add/article

1. node ref field has an empty choice at the top of select. text+checkboxes has one too 1. i provided a non php set of allowed values.

* MW: bug logged at http://drupal.org/node/219569

1. every time i preview, more textfield choices are added (is an unlimited field) even when i have empty ones available. i am also seeing "notice: Undefined index: #children in /Users/mw/contributions/modules/cck/content.module on line 1923.". i get that on view too with the 'test multiple' text formatter (which isn't outputting anything)

2. the help text appears below each instance of a multiple field. I would expect it at the end only.
   Fixed.

3. text field widget should do autocomplete when a set of allowed values has been provided. or provide yet another widget.

* MW: on second thought, most people will use select widget in this case. minor.

1. i noticed 'reverse link' on user ref fields (not not node ref fields). not sure this is a good road to travel. there are too many prefs to consider. should the link appear in teaser mode? should it be only title link or other formatters too? we are offerring title links only now it seems. we should consider recommending a viewfield instead of this feature. i am working on polishing that module.

node/x

1. " Fatal error: Function name must be a string in /Users/mw/contributions/modules/cck/nodereference.module on line 239" when i set a node ref field's display settings to anything but 'title - link'. formatter patch.
   Fixed in the formatters patch, which has since then been committed.

2. change "Illegal value for field_author." to "[value] is not an allowed value for [friendly field name]." This code appears in example_field.php as well.

3. when submitting a textfield, the 'Allowed Values' validator expects a key and not a 'display value' which is understandable but not ideal.

* MW: do we want to check for "aliases" as well and allow them if they map back to allowed value? Seems nice. I think the small risk of ambiguity is acceptable.

node/x/load (devel load tab)

1. the value of a node ref field is a string
* MW: this is amazingly how mysql_fetch_array() works. almost becomes a string.

DB

1. after removing a field, we are not removing columns from the content_type_[type] table
   ** There are several problems about removing data, see http://drupal.org/node/97861, http://drupal.org/node/209194, http://drupal.org/node/198508, (Karen)
   ** Moshe's talking about http://drupal.org/node/201329#comment-706701 - consequence of the content_field_instance_delete() fixes. (Yched)

examples/example_field.php

1. document more callbacks in text_field_info() and text_widget_info()?

README.txt

1. move JVD and JonBob to an Authors section and add Karen and yched to a new MAINTAINERS section.

theme_text_formatter_default()

1. should we really do allowed values check again in the formatter/theme layer?
   ** It's not really a check, it's displaying the 'alias' (if any) instead of the actual value
   (say you're using a number field to store a gender, you'd use 0|male - 1|female.
   0 or 1 gets stored, but 'male' or 'female' gets displayed.)