field_attach_*() calls
Taken from email discussion - Quick summary :
chx :
drupal_function_exists('field_attach_*');
field_attach_*(...);is ugly, we should do :
module_invoke('field', 'attach_*', ...);yched : This doesn't work because for this forbids reference-altering the arguments
chx: Then let's use the 1st form when by-ref args are needed and the 2nd one for the rest.
(end of summary)
I think mixing those two forms make poor DX. The beauty of this API is 'hey, look how simple it is to make your entity fieldable, just add these one-liners at the places that make sense for you'.
Plus the module_invoke() form gives the false impression that there is such a thing as hook_attach_*(), of which field.modules provides an implementation (current phpdoc for module_invoke reads 'Invoke a hook in a particular module'.
Should we move those functions into field.module so that they're always available (or maybe unconditionally load field.attach.inc in field_init() ?).
After all, field attach API is our main API, will be called much more often than field CRUD ?
I was thinking about exactly
I was thinking about exactly this issue just this morning. I agree that the drupal_function_exists() and module_invoke() approaches are both horrible. I guess I like module_invoke() more, at least it is only one line of code, but of course it can't always work. Grump.
Note that the Field module is going to be/should be required, just like node is, so we can assume it is always enabled.
This is a limitation/flaw in the code registry model, so "it isn't our fault." Not that that helps. Here are the options that occur to me:
My first choice is #4, but that isn't going to fly. My second choice is #2, with our hand-coded auto-loaders to be removed when #3 actually gets implemented.
For #3, my idea is that the code registry can generate a single .inc file full of all necessary auto-loaders, store it in the files/ directory (since we know it can write there), and always load that one file during bootstrap. Then we get auto-loading for all functions in every module, automatically. chx?
uh huh
the code registry helps with our memory footprint which is horrible. that is not helped by opcode caches.
generating autoloaders, maybe but how will you know which fucntions you want to call directly? A ton of functions are not called directly, hook implementations and form definition/validation/submit callbacks come to my mind.
Option 2 also makes code
Option 2 also makes code navigation in IDEs by CTRL-clicking function names more painful, but, right, that's probably our best bet for now...
Added to plan.
I added this task to the planning spreadsheet.
I created autoloaders for
I created autoloaders for all field_attach_*() functions. Here's a sample:
<?php/**
* Accounts for d-n-d reordering of field values.
* TODO D7 : rename to 'field_attach_submit' ?
*/
function field_attach_presave($obj_type, &$object) {
if (drupal_function_exists('_field_attach_presave')) {
return _field_attach_presave($obj_type, $object);
}
throw new FieldException('cannot autoload function _field_attach_presave');
}
?>
The Perl script scripts/generate-autoload.pl creates them, including the PHPdoc, etc., so they are very easy to re-generate. All the Field Attach tests still pass.
NOTE: You will probably have to rebuild your site after importing this change as the code registry will be all screwed up and user_load() won't work until it is fixed so you cannot really get to the "Clear the cache" function.
So, what other functions
So, what other functions should we have auto-loaders for? All of Field CRUD? Probably; we do not want module install files to have to use drupal_function_exists() explicitly.
I actually wonder if field_attach_*() should just always be loaded outright. Essentially every non-cached page load calls user_load(), and so will load field.attach.inc anyway, and we're just imposing an extra unnecessary function call. For Field CRUD, on the other hand, most page loads will not use those functions so autoloading makes sense.
thoughts?
Agreed that CRUD API could
Agreed that CRUD API could use autoloaders : it will be rarely called but we want the functions available without callers having to explicitly include stuff themselves.
Unconditionnallty including field.attach.inc was my initial sugestion, but thinking about it more, I'm not sure it's a good idea right now. Until we move, say, node body or taxonomy to fields, we can imagine some simple Drupal sites that would not need no fields at all.
field.module being 'required' is one thing, but we should stick to making it as lightweight as possible
Every page load for a
Every page load for a logged-in user (which is to say, every non-cache page load) calls user_load (right?). user_load() calls field_attach_load(), which loads field.attach.inc. So, I think field.attach.inc is always going to get loaded on any non-cache page load. If so, we should just always load it and save the indirect function call.
Nope
user_load is not automatic.
Well, then my argument for
Well, then my argument for always including field.attach.inc falls apart. :-)