Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
It would be best for everyone not to keep this strange "feature" into core.
Comment | File | Size | Author |
---|---|---|---|
#56 | field-autoload-4450440-56.patch | 42.22 KB | bjaspan |
#46 | field-autoload-4450440-44.patch | 41.07 KB | bjaspan |
#37 | field-autoload-4450440-37.patch | 54.72 KB | bjaspan |
#34 | field-autoload-4450440-34.patch | 41.52 KB | bjaspan |
#25 | module_invoke_by_ref2.patch | 14.43 KB | Berdir |
Comments
Comment #1
Damien Tournoud CreditAttribution: Damien Tournoud commentedExperimental patch (removes autoload for field_attach).
Comment #3
Damien Tournoud CreditAttribution: Damien Tournoud commentedSlight reroll.
Comment #4
catchsubscribe.
Comment #5
catchcrossposted.
Comment #6
Damien Tournoud CreditAttribution: Damien Tournoud commentedReformatted the exact same patch.
Comment #7
Damien Tournoud CreditAttribution: Damien Tournoud commentedAs a summary: this patch aims to remove the autoloading "feature", that came bundled with the fields API megapatch.
Here is how this "feature" works:
* Some functions (like
_field_attach_load()
) are marked with a special @autoload doxygen tag* An external, unreleased "generate-autoload.pl" file parses the files and generate a corresponding
field_attach_load()
function infield.autoload.inc
* The generated functions are simple wrappers that (1) call
drupal_function_exists()
to load the target function and (2) call the target functionThis allowed the fine team in Boston to advance, but it is far from being future proof, and its time has gone.
I suggest making most of the functions in
field.attach.inc
andfield.info.inc
static methods of three classes: FieldAttach, FieldBehavior and FieldInfo. Classes are automatically discovered and loaded by the registry, thus removing the need for inelegant wrapper functions in core.Comment #8
webchickWell that certainly sounds like a DrupalWTF. :)
We need to get Barry's two cents on this patch, since autoload was his baby.
Comment #9
bjaspan CreditAttribution: bjaspan commentedWe should mark either this issue or the older http://drupal.org/node/363967 as a duplicate.
Offhand I'm not opposed to using an effectively dummy class as an autoloading wrapper. Field API should be class-based anyway, so the dummy class may help lead to a not-dummy class. And in my original implementation (mid 2008) the basic Field and FieldInstance data structures were actually classes, solving the problem.
But i haven't reviewed this patch yet.
Comment #10
Crell CreditAttribution: Crell commentedIMO there are plenty of reasons for Fields to be full-on objects anyway, which solves the autoload issue. My objection to Barry's earlier implementation (we discussed it over dinner in Szeged) is that it wasn't a full-on object, but really just a struct to provide a different way of specifying defaults. My statement at the time was "make it a real object with proper methods and encapsulation and dependency injection and stuff or don't, but don't make it half-arsed OO." I'd still prefer "full on object" to procedural here. :-)
Comment #11
Damien Tournoud CreditAttribution: Damien Tournoud commentedArguments from Crell on the IRC:
True, but we are *not* rewriting anything. Just putting a wrapping class. The patch is huge, but most of it is changing the indentation level.
True, but this *is* a procedural implementation. We are just putting a wrapping class around it, using this wrapping class as an autoloading namespace.
Comment #12
yched CreditAttribution: yched commentedBy "OO Field API", we generally mean something like "$field structure as object of class TextField, hook_text_load() as TextField::load()", etc.
This is not at all what this patch does. It just tuns field_attach_X() API functions into methods of a static class. Those functions wouldn't be methods of any Field class anyway, so it's not even 'putting a wrapping class around the procedural field API". So I don't think this makes a later OO Field API any easier or more difficult.
As I said to DamZ on IRC, my only minor gripe with this approach is that is makes the 'field_attach_[op]() goes with hook_field_attach_[op]" pattern a little less clear. We get "FieldAttach::load() goes with hook_field_attach_[op]". No better proposal for now, though.
Comment #14
bjaspan CreditAttribution: bjaspan commentedThere is no good solution here. We're just trying to find the least objectionable solution. I enumerated the options I was aware of in #363967: Resolve auto-loading strategy, and this issue adds a new option (kudos to Damien for thinking of it). Our list so far:
1. Load all of Field API's API functions on every page request. This is what we are trying to avoid.
2. Require every module that wants to use Field API functions to wrap their calls in drupal_function_exists().
3. Use module_invoke() for Field API, which makes pass-by-reference impossible.
4. Provide explicit autoloaders; this is what is currently committed. I can of course contribute the script that generates them as well (and I know a way to eliminate the @autoload tags).
5. Use a dummy class with static methods to provide implicit autoloaders.
I think #2 and #3 are out of the question. Personally, I think #4 is the least objectionable, which I why I went with it (mea culpa for not including the script and fixing @autoload) but could happily live with #1, #4, or #5.
Of course, we also have:
6. Rewrite the whole API in an OO style.
That's overkill to solve this particular problem. I'd be happy with it, thought, but do not have time to do it for D7.
Comment #15
catchAt least in the short term, could we move everything in field.autoload.inc into field.module? Would save field_init() (or the direct require_once() which I saw in another unrelated patch due to field_init() breaking).
Comment #16
bjaspan CreditAttribution: bjaspan commentedThe unrelated patch you saw loads field.autoload.inc into field.module outside of hook_init() because hook_init() is not run during update. Moving everything from field.autoload.inc directly into field.module is not different in any way I can think of from simply loading field.autoload.inc at the top of field.module as that other patch does.
Comment #17
catchIt's one less require_once() - and having require_once() outside a hook at the top of a .module just looks nasty frankly.
I don't know if require_once() actually has a measurable performance impact though (and only on non-APC sites) - but if we move to a larger number of .inc for registry optimisation, we should also try to centralise code which really is going to be needed on every request.
Having said all that, I don't see the problems with the current patch - it's not a rewrite, it's just a lot of moving stuff around.
Comment #18
Dries CreditAttribution: Dries commentedTagging.
Comment #19
chx CreditAttribution: chx commented3. Use module_invoke() for Field API, which makes pass-by-reference impossible.
This is not true -- PHP5 passes around objects by handler which fixes this. If you want to change arrays, that's a problem, yes but I am of opinion that this also can be fixed. The attached patch shows how. PHP5 only, but this is D7.
Comment #21
yched CreditAttribution: yched commentedre #19 : Yes, our use cases include passing arrays by ref.
The proposed patch only takes care of the first argument, right ?
Comment #22
chx CreditAttribution: chx commentedYes but that's enough isnt it? I wonder why I get a failed to install HEAD -- I tested installation, that's why user.module is patched.
Comment #23
yched CreditAttribution: yched commented"- The proposed patch (#19) only takes care of the first argument, right ?
- Yes but that's enough isnt it? "
Not quite :
Comment #24
chx CreditAttribution: chx commentedWell you need to move by-ref to the first argument. That cant be a problem.
Comment #25
Berdirchx's patch in #19 did not work because there are quite a few module_invoke($module, $hook, 'some string') calls.
I changed those that I could find...
Comment #26
bjaspan CreditAttribution: bjaspan commentedI just want to clarify that I consider "Use module_invoke() for Field API" to be out of the question even if the pass-by-reference issue is resolved. Field API is a core Drupal API. Do we really want to declare that *every* Drupal API function needs to use module_invoke(), e.g.
I think the answer is clearly no, and if we don't want to do it for node_load() and l(), then we should not do it for Field API, Form API, DB API, or anything else. It would be a horrible mistake to allow the code registry's autoloading capability to force us to making Drupal so annoying and ugly to program.
Comment #27
chx CreditAttribution: chx commentedThe difference here is that these field API functions are relatively rarely called compared to say node_load or l.
Comment #28
Damien Tournoud CreditAttribution: Damien Tournoud commentedI tend to agree with Barry here. There is no added value in replacing the stub autoloading functions with calls to module_invoke(). Plus, module_invoke() documentation clearly states:
So at the very minimum, we need to change the documentation.
I believe that enclosing the big FAPI features (attach, info, behavior, etc.) by autoloading wrappers is what makes the most sense.
Comment #29
yched CreditAttribution: yched commented"Well you need to move by-ref to the first argument. That cant be a problem."
Er, that's a pretty big constraint on the shape and logic of the APIs. + agreed on Barry's arguments in #26. Using module_invoke() here just for its autoload feature is a hack.
Damien's static class approach is fine by me, if we don't want to keep the current autoloaders. As I wrote a few comments above, it doesn't make a later move to an OO Field API any easier or harder. The functions involved wouldn't be methods of Field or Instance classes anyway, which is what people mean when they say 'OO Field API'. If anything, field_attach_* functions would be methods of a FieldableEntity superclass which Node, User, etc would extend, and Drupal is frankly not there yet.
Comment #30
chx CreditAttribution: chx commentedIf FieldAttach::update() is what the gang wants, sure. I was just showing module_invoke is quite possible...
Comment #31
bjaspan CreditAttribution: bjaspan commentedLarry previously objected to Field API using classes just for the sake of data structures. I'm going to object to using classes just for the sake of autoloading, though less strenuously than I object to module_invoke().
Honestly, the more I think about it, the more I think autoloaders are the right solution. This is exactly the sort of thing every other system in the world uses autoloaders for, and no one else has proposed a better solution. I do need to improve the autoloader approach as previously mentioned and contribute the generation script.
I'll assign this to myself at least until I deliver the best autoloader patch I can.
Comment #32
bjaspan CreditAttribution: bjaspan commentedComment #33
Damien Tournoud CreditAttribution: Damien Tournoud commentedAs previously mentioned, the "autoloader" approach defeats the purpose of the registry, adds an unneeded PHP overhead to an already slow system and introduces a pattern I personally don't want to see in core.
In a nutshell: we already have an autoloader, the registry, that can dynamically load classes. I see no reason not to use that.
Comment #34
bjaspan CreditAttribution: bjaspan commentedThe attached patch contains:
1. the generate-autoload.pl script, reworked not to require the @autoload tags presently in Field API code,
2. updates to Field API code to work with the new generate script (removing @autoload tags and renaming functions to be autoloaded from _funcname to al_funcname), and
3. a new field.autoload.inc file generated from the changes above.
My response to #33 is: (1) I think this approach is *faster* than the code registry because there is much less overhead involved. (2) The purpose of the registry may be to autoload code, but it fails for this use case, so we either need an approach like mine or an approach like yours of introducing fake classes to get static methods.
Except for small improvements/bug fixes, I'll call this patch my best shot for automatically generated autoloader files as the solution to this problem. If the community goes for static methods, I'll live with it.
Comment #35
bjaspan CreditAttribution: bjaspan commentedTo clarify: The registry fails to autoload code for the use case of global Drupal APIs that are defined in a module, should always be available, and are procedural. It's great for menu callbacks.
Comment #37
bjaspan CreditAttribution: bjaspan commentedHere is the same patch, but with generate-autoload written as a PHP script.
The generate script would need to be a bit more clever: it should scan the entire Drupal docroot and generate an autoload include in each module directory that has autoloaded functions. Otherwise, the committers will have to remember which modules to run it for manually which clearly is no good. Another option would be to scan the Drupal docroot and create one big autoloader for everything in includes/autoload.inc, but that would actually break drupal_function_exists() since it would return TRUE for functions in modules that are not actually enabled.
I recognize that this whole approach has flaws. Maybe fake classes with statics really is better. <shudder>
I'm marking this CNR so we can get a ruling from Dries and/or webchick.
Comment #38
tstoecklerComment #39
Crell CreditAttribution: Crell commentedNot to be snarky, but I really have to wonder why we're going to all this trouble to build complicated alternate-autoload systems for the Fields API when the issue to leverage the registry properly to reduce code weight is still languishing with "well do we *really* need it?" comments. (#345118: Performance: Split .module files by moving hooks) Methinks we're chasing the wrong cat.
Comment #41
bjaspan CreditAttribution: bjaspan commented@crell: I think #345118: Performance: Split .module files by moving hooks is irrelevant to this discussion. The Field API code in question is not hooks, it is public API functions. My understanding is that you think public API functions should be in .module files to be loaded on every page request. We could certainly do that, and it would resolve this issue. But we tried to arrange it so Field API code is only loaded when needed.
Thinking about it, though, the Field Attach API is/will be called on most content pages: nodes, comments, taxonomy terms, users, etc. all can or will have fields attached. So maybe Field Attach API functions *should* be loaded on every page load. Maybe we should even just move it from field.attach.inc to field.module.
The non-hook Field API functions that are called rarely are the Field CRUD functions, and there are not that many of those. I'd still like to avoid loading them on every page, though. Perhaps we should just hard-code manual autoloaders for those functions into field.module and be done with it.
Snarky comment in return: If we had stuck with my original idea that you shot down of class based data structures for Field and FieldInstance, this would be a non-issue. :-)
Comment #42
bjaspan CreditAttribution: bjaspan commentedCorrection to snarky comment in #41: using classes for Field data structures would not have helped, it would have made the situation worse. Class Field would have logically been defined to contain the CRUD functions, which are the ones that are used rarely, but then used as arguments to FieldAttach functions, which are used often. So we would have loaded field.crud.inc just to get the type definitions when we did not need the code that came with it.
Comment #43
yched CreditAttribution: yched commented#345118: Performance: Split .module files by moving hooks in indeed a different issue. The way I understood Crell's comment was "if #345118 is still getting skeptical looks, why should Field API bother with its own loading cost". Esp. given given that field-free pages won't be that frequent : admin pages, Views pages (if no block trigggers a node / user load of its own...)...
In this case, manual loaders in field.module for CRUD functions seem reasonable.
Comment #44
chx CreditAttribution: chx commentedCRUD could use module_invoke, hm?
Comment #45
bjaspan CreditAttribution: bjaspan commentedre #44: Sure. If we move node_type_save() from node.module into node.types.inc and require module_invoke() to call it, and do the same for all other Drupal APIs, then we can do the same for Field CRUD functions.
Comment #46
bjaspan CreditAttribution: bjaspan commentedIt is time to start over on this issue. Here's the problem: Field API adds a lot of new public, always-present APIs to Drupal, and Drupal currently has no way to handle autoloading them. The FiC team tried to improve on this situation by creating an autoload mechanism for Field API that maybe could also be used by the rest of Drupal, but no one (including me) really likes what we came up with nor any of the alternative solutions suggested so far.
It seems clear to me now that since we do not know how to handle autoloading these functions in an acceptable way, we should do what every other module providing a public API does: load them on every page request. When we find a solution to the problem as a whole, that solution will also work for Field API. Until then, there is no reason for Field API to be different than anything else.
So, attached is a patch that removes autoloading from Field API completely. I think we should commit this patch and then possibly open a separate issue for addressing autoloading rarely-used public APIs (which will be the sister issue to but different from #345118: Performance: Split .module files by moving hooks).
Comment #47
bjaspan CreditAttribution: bjaspan commentedIncidentally, #46 even achieves the goal stated in the issue title: Remove the field autoload "feature". :-)
Comment #48
catchLooks good, lets us deal with the issue more centrally instead of it being field API's problem. Being able to look up the actual function on api.drupal.org will be a lot less clicks.
Comment #49
chx CreditAttribution: chx commentedFresh start++
Comment #50
Crell CreditAttribution: Crell commentedI am +1 on not making Field API special and working on a better global solution later. This is not a Field API-specific problem so the solution should not be Field API specific either.
Comment #51
Crell CreditAttribution: Crell commentedOops, crossposted.
Comment #52
yched CreditAttribution: yched commentedWorks for me too.
Comment #53
webchickI'd like to get Damien's thoughts on this before committing, but this approach definitely looks much more Drupalish, and if it results in much higher RAM consumption, then that's just extra fire under our behinds to solve this in a generic way for all .inc files.
Comment #54
Damien Tournoud CreditAttribution: Damien Tournoud commentedWorks for me.
Comment #55
yched CreditAttribution: yched commentedDouble space issue in the field.module comment, though.
Comment #56
bjaspan CreditAttribution: bjaspan commentedFixed the double-space issue in the field.module comment and, since I had my search-and-replace handy, in field_help() as well. No other changes to the patch, I swear; in fact I diffed the two patches to be sure.
Comment #57
Dries CreditAttribution: Dries commentedKilled! Committed to CVS HEAD. Thanks.
Comment #58
yched CreditAttribution: yched commentedYay. Now let's reroll broken patches ;-)
Comment #59
yched CreditAttribution: yched commentedDries, it seems field.autoload.inc file hasn't been nuked as it should. Back to RTBC.
Comment #60
webchickRemoved. Thanks!