Namespacing for bundle names

Patches welcome. :-)

We do need to fix this before D7 ships. Thus marking as critical.

Prefixed bundle names is tricky, because nodes come in with $node->type = 'story', not 'node_story'. We cannot change this, it would cause an upgrade nightmare (variables for content type settings, node-story.tpl.php templates ...).
Having both an external, non prefixed name, and an internal bundle name where we force a prefix sounds awkward.

Another option is to state that the [bundle name, entity type] pair identifies a bundle. At first inspection, this means:
- adding an entity_type column to the field_config_instance table, and make [field_name, bundle, entity_type] a unique key (instead of currently [field_name, bundle])
- adding an $instance['object type'] property,
- changing the internal structure built in _field_info_collate_fields() : key $info['instances'] by object type then by bundle name, and store the [bundle, object type] pair in $info['fields'][$field_name]['bundles']
- adding a $obj_type param to _field_info_instances($bundle_name), and _field_info_instance($field_name, $bundle_name)
- keying the results of _field_info_bundles(NULL) by object type

Too late tonight to investigate the 'serial bundle_id' option. My first reaction on serials is to cringe for non-portability and staging nightmare.

Just some thoughts:

If the reason we can't solve the problem easily with prefixed bundle names is that we can't change $node->type (and I agree we can't change that), then we could also change the field_attach functions to take the bundle name as a parameter instead of implicitly in the object, e.g. field_attach_load('node', $node->type, $node). Of course, we're also about to add a $language argument to field_attach_view and _form, so maybe that would be getting towards too many arguments.

We could declare that an object's bundle property *must* be namespaced and leave it up to the field_attach caller to worry about it. Then node module, since it cannot change $node->type, could set $node->bundle = 'node_'.$node->type and return 'bundle' as the bundle property in hook_fieldable_info().

I don't think I like the idea of tying a bundle to a specific entity type. I have a vague sense that it might be useful to have a module that defines a collection of fields and a bundle to load them, without knowing what kind of fieldable entity they will be loaded into.

So, I went a little back and forth on this, and came to the conclusion that:

- This will keep popping at us. I think we won't see actual case of entities with 2, 3 or n (fixed) bundles. Either an entity type has no bundles (meaning only 1), like currently 'user', or if there's a case for several, then in the absolute they need (or at worst beg, if not actually implemented) to be user-extensible. Which implies user-entered names, as the 'fieldable terms' patch showed, which required the introduction of vocab machine names.
User-entered names means clashes. Uniqueness inside a given object type is all you can reasonably enforce.

- Asking fieldable entities to add a new, namespaced $object->bundle property, that concatenates two separate informations into one string ("[obj_type]_[obj_type_variant]"), is ultimately a hack that just defers onto client code the job Field API is too lazy to do.
$node->type = 'article' is what is natural to node.module. Requiring the additional $node->bundle = 'node_article' is redundant.
It also makes it extremely hacky or convoluted to find back the object type from the concatenated string we get as 'bundle name'.

- To try to reduce API changes, I considered an intermediate step where field_attach_extract_ids() could automatically prefix $obj_type to the bundle name it reads in the object. Abandoned that to bee a hack as well, where we just artificially manipulate and store two informations into one.

- At the end of the day, I do think an instance is the combination of a field_name, an object type and a bundle (an object 'subtype').

So I bit the bullet and went on the API change needed to reflect that: everywhere we had a $bundle function parameter or a ['bundle'] property, we also need an object type. Not a small patch, but I think the API actually ends up cleaner.

Fixed typo.

Er, previous patch simply had the updates to the db interaction with {field_config_instances} left as a TODO -> massively breaks.
This should make things a bit better.

Attached patch should fix the remaining test failures.

Hmn strange, patch still applies fine on my copy of HEAD. Test bot glitch ?
Rerolled, let's see.

OK, comments are now fieldable, their calls to field_attach_[create|rename|delete]_bundle() need to be updated too.
That was 2 exceptions per 'test drupal install' per node type present in the default profile...

Retest, just to see. I'd like slave #29, for instance :-p

Doh, you bet, parse error in comment.install. Should be better now.

Updated the tests for the 'taxo term field' that went in recently, and rerolled after #535034: Clean-up how fields and instances are prepared for runtime..

Sigh... Getting there.

This is now quite stale, but rerolls are painful for this, so I'd welcome feedback on the approach before.
Note that this is probably the most critical Field API task right now. Actually, retagging as a bug: creating a content type called 'user' and having all kinds of messy things happen qualifies as a bug...

In Paris we agreed with Barry that this is the good approach. Sadly, I couldn't find the time to reroll since then, and am now away from my coding env. for the next 4 weeks :-(.

yched is right, this patch is a serious PITA to re-roll. Here is my first attempt. I think all the Field API tests pass on my system, we'll see what the testbot thinks. The patch is not done yet, there are some doc TODOs remaining.

Fixed all reported failures, at least locally.

Ahem. Fixed the forum module (apparently I forgot to run those last time) and the doc TODOs. However, now I realize that field_ui.module is seriously broken, so the patch still isn't done.

Ooops.

Field UI at least seems to work now (create, edit, and delete). This patch is officially CNR. The patch is large-ish but essentially every change is the same: we just pass object_type everywhere we pass bundle. The patch touches EVERYTHING that uses Field API and thus is a real PITA to re-roll. Luckily, since 10/15 is coming, it will get committed soon. :-)

I have to say I'm not in love with it, but I think it is required. It appears that Mike Booth was right back in May when he said that bundles needed to be objects in their own right. At the time, bundles were just strings, so "bundle name" == "bundle" and there was no need for an object. Now, a bundle is a name and an entity type, which is two properties instead of one. Instead of creating a bundle object, this patch puts both values separately into function signatures, forms, etc. It works, but is inelegant... and if bundles ever get a third property, it will clearly be stupid. However, I also think it is late in the game even to be making this change, and introducing a new object type would be just one step more last-minute complexity.

Also, I'll go on record as saying that I think *I* was right back in February 2008 at DADS and December 2008 at the Fields in Core sprint: the whole concept of fields, instances, and bundles is somewhat wrong. We should have fields and entities. If I want to create a node with an extra image field, I should not have to define a new bundle for it; I should just be able to add an extra image field to that node. In this model, "bundle" is just a template for the default set of fields to be added to new entities when they are being created in the UI. This will of course require storing information about which fields are attached to which entities on a per-entity level... which would be perfect for hashtable-style databases like SimpleDB but perhaps not so efficient in SQL.

However, my previous paragraph of rambling will CLEARLY not happen for D7. I'm just rambling. :-)

Anyway, this patch is CNR, and I think it is should go in, modulo any last-minute bugs people find.

Re-rolled for #557292: TF #3: Convert node title to fields.

Tests pass for me. Re-rolled and trying again.

Ahem. What I meant was, "without the patch applied, all tests pass!"

Now they pass with the patch applied. Much better.

Rerolled.

This is much needed to prevent that users screw up their sites beyond repair. Committed to CVS HEAD! Thanks bjaspan and yched.

There was a minor mistake in this patch. The $obj_type argument to field_info_instances() is supposed to be optional. The function body is written to allow it to be optional, but the function signature is wrong. I'm going to be bold and mark this one-line fix RTBC. :-)

Committed. Thanks!

This causes image field to fail, patch at #606130: Field patch creating error for image field on node/add/*.

Gosh ! Thanks and kudos, Barry.

#609506: 'Undefined index' on admin/reports/fields and 'change widget' form fixes a few missing changes in field_ui