This draft patch introduces a common set of methods for saving, loading, and deleting Drupal objects (nodes, users, taxonomy terms, etc.) and also a new hook, hook_drupal(), to be used with all object types, supplementing (but not replacing) the existing set of hook_user(), hook_nodeapi(), etc.

Background

On the dev list recently we've discussed a longstanding desire: to bring consistency to the various Drupal object types (node, term, user, etc.) and at the same time make it possible to attach behaviours to all object types at once (rather than having to do each as a separate hook).

Several creative approaches have been suggested. They all have at their core the creation of a common set of methods that all object types can extend. From there several different proposal emerge. Some want to introduce a new global object table, so all objects have a single unique identifier. Some ideas involve relatively minor changes, while others propose radical restructuring of our existing code.

Approach

We could and should continue discussing possible approaches. But it seems most practical to start with implementing the minimum we've agreed on so far: a common, consistent set of methods for all object types. With that implemented, we'll be in a better position to see what further restructuring we need. It will also put some powerful immediate benefits of a common API into fairly easy reach, e.g., converting node_access to be a general object access system.

Specifics:

  • Introduce a set of object handling functions in common.inc:
    • drupal_load(), calls the appropriate $type's method, e.g., node_load().
    • drupal_save().
    • drupal_delete().
    • drupal_invoke(), used to invoke a type-specific method, e.g., node_load(). This is called only by the drupal_load(), drupal_save(), and drupal_delete() methods. In other words, we retain our existing _load, _save, and _delete functions (for nodes, users, etc.), but they're now called not directly but via the general drupal_ calls.
    • drupal_invoke_drupal(). This calls the general hook_drupal(), which can be implemented for all object types. Our old friends hook_user() still exist, and can still be implemented separately. In fact, doing so is more efficient if the desired action applies to only one object type, e.g., node. They are not, however, invoked directly. Rather, calling drupal_invoke_drupal() invokes hook_drupal() and the type-specific hook, e.g., hook_user().
  • Details:
    • hook_nodeapi() is renamed to hook_node() for consistency.
    • Order of arguments is reordered for consistency and for passing by reference. Specifically, the $op argument comes before the $object being passed in hook_drupal() and therefore in hook_node() etc.
    • Existing object functions are renamed for consistency, e.g., _comment_load() becomes comment_load().
    • The existing _save and _delete return values SAVE_NEW, SAVE_UPDATED, and SAVE_DELETED are returned for all object types (e.g., they were used previously for terms but not for nodes).
    • Taxonomy vocabulary and term object handling is pulled into two include files, vocabulary.inc and term.inc, which have function names without the 'taxonomy', e.g., vocabulary_save().
    • hook_taxonomy() is dropped. hook_vocabulary() and hook_term() implementations are possible, but in most cases it will be simpler to change hook_taxonomy() implementations into hook_drupal() implementations.
    • The existing function drupal_load() is renamed to drupal_load_file(), since we introduce a new drupal_load().

Remaining issues

Many, no doubt!

This is a roughed-in and untested patch. I haven't yet upgraded my test environent to MySQL 4.1 so I haven't been able to test at all yet. I'll do so soon and post revisions. It's certainly broken in places.

One I'm aware of is passing by reference. In this draft I've used only a single argument passed by reference in drupal_invoke_drupal(), where hook_drupal() and the other object hooks (hook_user(), etc.) are invoked. However, at least one existing hook, hook_user(), has two arguments passed by reference in at least some hook implementations. I'll need to look more closely at these to see if it's possible to get this down to a single argument passed by reference. If not, it'd be messy, since arguments passed by reference can't have defaults. We'd need to pass in arguments for all calls to drupal_invoke_drupal(), even though in most cases they're not needed.

Also, a number of inconsistencies are not addressed here. For example, though

I'd appreciate feedback on the general approach and the specific implementation. Is this where we want to be going as a first step to the aims outlined above?

Comments

forngren’s picture

Subscribe.

pwolanin’s picture

see: http://drupal.org/node/79684#comment-184791

I think that hook_drupal() is really non-obvious (-1). hook_objectapi_X, hook_objects_X, hook_entity_X, something!

If we're going to do this, hook_nodeapi, hook_user, etc. should be abolished- a make the API consistent, and as simple as possible.

For this API hook, I'd also rather see a sparate function for each op (per the link above), which mean that the only switch statement in each function is a switch on the object class (need to standardize terminology here).

nedjo’s picture

hook_drupal() is really non-obvious (-1)

Agreed, it's probably not critical at this point to agree on final terminology but 'entity' might be better until we do.

Thanks for linking back to the CRUD issue, I hadn't seen the promising contributions there in the last few days.

I've updated that issue with a reference to this one, and a possible strategy of completing this patch as a step towards a larger API reworking.

webchick’s picture

I think this type of approach will definitely work as an interim step to a totally abstracted hook_everything. Though if we can do it in one step... :)

-1 to arguments like $a3 and $a4. I was stunned and shocked that that got into hook_nodeapi. These are totally non-intuitive, and worse, aren't PHPDoced in the actual functions. Why do we need $a3 and $a4 when we have $args as an array of N depth? Is this for compatibility with nodeapi? If so, nodeapi should be changed to take the more generic $args parameter, imo.

+function drupal_load($type, $param = array(), $a3 = NULL, $a4 = NULL) {
+  $object = module_invoke($type, 'load', $a3, $a4);
+  drupal_invoke_drupal($type, 'load', $object);
+  return $object;
+}

$param is never used in the function body, so this isn't adding any flexibility... it's also PHPdoced as $args, not $param. One of these is a typo...

General question: would it reduce the logic required if we passed $arrays rather than $objects around? Everything comes in and out of forms as arrays.

Sorry for the brevity; will try and review more thoroughly later...

nedjo’s picture

Here's a refreshed patch with at least some of the more glaring errors fixed.

Changes in this patch version:

  • drupal_invoke_drupal() changed to the more intuitive drupal_invoke_entity().
  • Documentation for the arguments of drupal_load().
  • Entity/object handling methods are added as a new include file, entity.inc. This can be overridden by a contrib module defining the variable drupal_entity_include. This allows custom object handling methods, comparable to the approach with drupal_mail() which allows the registration of a custom mail handler.

Since this patch changes loading, saving, updating, and deleting, as well as all object API hooks (hook_user(), hook_nodeapi(), hook_taxonomy(), etc.), there's a lot of room for problems.

I've installed this on a new site and most things seem to work--create, update, and delete nodes, terms, users. I added a node with a path and that was correctly saved, indicating that the new hook_entity() is working as a wrapper for existing hook implementations (in this case, path.module's path_nodeapi(), renamed to path_node()).

One error I found was that the taxonomy term select was broken on node edit--no terms were displayed as options. There will be more such problems.

Why do we need $a3 and $a4 when we have $args as an array of N depth?

Well, sure, we could use func_get_args() to read in these arguments, then array_shift() off the first ones as needed (when we need to pass some arguments by reference, or when there are indeed consistent arguments used across all object types).

The main issue here is that in this patch we're not - yet - making the larger changes that would be needed to get all object types to have the same set of arguments.

The biggest problem looks to be user, which passes around that extra argument--the array of user properties that's used in addition to the user object.

But, again, in this patch so far I'm not wading into reworking everything. Yes, we could and should make user handling more consistent with node and comment. Yes, we could and should allow term loading to feed in an array of parameters to match like we do with users and nodes. Yes, we could and should be consistent in our choice of array vs. object data type.

I'm hoping we can tackle those one by one in follow up work after making the minimal changes to achieve the primary aim here: a single set of object handling methods and a single object hook.

Any testing, comments, or reviews would be greatly appreaciated.

webchick’s picture

No patch, nedjo. :(

nedjo’s picture

FileSize
113.42 KB

Thanks, with attachment this time :)

pwolanin’s picture

I still feel that we'd be btter served by breaking out the object hook into a set of hooks: hook_entity_X, where X = load, insert, update, prepare, submit, view, etc- essentially the full list from hook_nodeapi().

this can also help to avoid the $a3, 4a4 problem, since you can more clearly define what the parameters means for each X!

nedjo’s picture

I still feel that we'd be btter served by breaking out the object hook into a set of hooks:

I can see the point, but I don't see the strong need that would justify introducing a plethora of new hooks (most of them very similar to each other). I think our best way forward is to make these changes in steps, doing the minimum at a time. Keep in mind that there are many $op arguments used at present for the object hooks, much more than just the basic load, update, insert, delete, so we might end up with 8 or 10 or more new hooks. It would be better to incrementally reduce the need for all these $op arguments, and then consider whether we can/should separate out to separate hooks.

In general this patch would need a lot of community review and energy. I'm not confident enough in the details of the approach to press on without some informed feedback and critical review, so I'm going to leave it where it is until if and when it gets the quality of review that would allow us to move forward--or abandon this approach for a better one.

I've opened a new issue, http://drupal.org/node/118345, focusing on user methods, that could be tackled separately to make this job much easier.

pwolanin’s picture

I turned my sandbox code (mentioned above) into a working module: http://drupal.org/project/object_driver

Some of the features implicit in the original (non-working) code have been cut, but perhaps this will at least be useful and/or provoke more discussion.

ray007’s picture

subscribing

nedjo’s picture

Title: General _load, _save, and _delete methods for all objects, plus a single object hook » Data API: first steps
Assigned: Unassigned » nedjo
Status: Needs work » Needs review
FileSize
79 KB

Here's a refreshed patch. It needs testing and work, but it's ready for review.

With a schema API nearly ready, http://drupal.org/node/144765, we're opening the door to a much improved Data API.

Most of that improvement, likely, will happen after Drupal 6. This patch is intended only to take some preliminary steps.

Notably:
* Introduce a single set of methods for saving/updating, deleting, and loading data objects.
* Introduce a hook_dataapi that can be used to respond to these operations for all data objects.
* Enable the substitution of a custom set of object handlers via an override of the dataapi.inc file. This is designed to enable the development of contributed solutions.

Changes from the previous patch:
* changed 'entity' to 'dataapi', more descriptive.
* removed user handling. we'd need to write http://drupal.org/node/118345 before being able to integrate users.

(I'm having diff challenges, please report if the patch doesn't apply for you.)

kika’s picture

Just reading the patch:

1. _selete looks like typo

 function taxonomy_vocabulary_confirm_delete_submit($form_values, $form, &$form_state) {
-  $status = taxonomy_del_vocabulary($form_values['vid']);
+  $status = drupal_selete('vocabulary', $form_values['vid']);

2. why term.inc and vocabulary.inc are created? It is a good thing, yes, but why part of this patch - can not it be handled independently, making this particular patch slimmer?

moshe weitzman’s picture

subscribe

Wim Leers’s picture

Subscribing.

bonobo’s picture

subscribe

nedjo’s picture

@kika, thanks for the typo note, I'll roll a new patch soon.

The term and vocabulary methods are needed for consistency--all object methods are in the form e.g. objectname_load(). They could be included directly in taxonomy.module, they just seemed to me to make more sense as includes.

These changes are half-steps; they increase consistency, but leave most of the variability of the current methods unaddressed. In http://drupal.org/node/145684 I've tried to sketch out some next steps. It only makes sense to take the first steps now if have an idea of where we want to go next.

webchick’s picture

nedjo, while you're re-rolling, the vocabulary.inc hunk fails for me:

patch: **** malformed patch at line 2063: --- modules/taxonomy/vocabulary.inc

(and subscribing :))

bjaspan’s picture

subscribe

criznach’s picture

This is killer stuff guys and gals. I'll be watching this API for sure.

nedjo’s picture

FileSize
80.72 KB

Rerolling the patch. Includes a few small error fixes.

drewish’s picture

in common.inc:

+  // Allow for custom dataapi handling methods.
+  // Must come after file.inc.
+  if (variable_get('drupal_dataapi_include', '') && file_exists(variable_get('drupal_dataapi_include', ''))) {
+    require_once './' . variable_get('drupal_dataapi_include', '');
+  }
+  else {
+    require_once './includes/dataapi.inc';
+  }

it seems a little wonky to be calling variable_get('drupal_dataapi_include', '') three times. why not put the value into a variable? trying to keep it out of the global scope?

otherwise i'm pretty excited by what this will allow, and it'll take care of one of my pet peeves: the lack of a hook_delete_revision

webchick’s picture

Functionality test:
1) I can create vocabularies, but I cannot edit them, add terms to them, or list their terms... I always get redirected to the overview page. Probably something isn't quite wired correctly after the vocabulary.inc/term.inc split, because adding/editing forums and forum containers works fine.
2) LOTS of errors/warnings when previewing a comment. The trouble seems to start with:

warning: Missing argument 3 for drupal_invoke_dataapi(), called in /Applications/MAMP/htdocs/head/modules/comment/comment.module on line 1763 and defined in /Applications/MAMP/htdocs/head/includes/dataapi.inc on line 121.

Sorry, gotta run to supper for now. :(

nedjo’s picture

Thanks, I'll get working on these issues, and more testing.

nedjo’s picture

FileSize
84.44 KB

Here's an update with a few fixes.

The vocabulary and term issue has to do with our new menu system. We feed in menu items with e.g. %taxonomy_vocabulary as an argument, which is interpreted as a load function, taxonomy_vocabulary_load(). I've replaced the occurrences of %taxonomy_vocabulary with %vocabulary in the vague hope that that might trigger vocabulary_load(), but no go in my testing. Hints anyone?

Some of the errors in comment preview came from a usage of $status that I missed in comment_save(), now fixed. But there are some remaining, suggesting that certain form values are not available in validation. I haven't determined why.

This patch certainly needs plenty of further work and even basic testing. Leaving the status at 'needs review' therefore seems inappropriate. But in some ways the remaining issues are details--important ones, yes, but solvable. What's equally important at this point is, is this a general path we want to be taking? So I'm leaving it as needs review because that question remains open.

pwolanin’s picture

@nejo - try activating or de-activating a module to force a menu_rebuild()

nedjo’s picture

FileSize
83.54 KB

Refreshed patch with some fixes and improvements.

The vocabulary paths are now working (vocabulary editing, term adding).

I went through the various dataapi.inc methods and eliminated the hated $a3, $a4, $a5 arguments. Turned out most of them were unneeded. The only change I had to make to this was that, for node validation, hook_nodeapi() used to pass the $form array. This wasn't used anywhere in core. I've dropped it, enabling us to have clean, unambiguous arguments in hook_dataapi().

I'm still getting some undefined index errors on testing. I'll try to test on a clean install and compare.

nedjo’s picture

Status: Needs review » Postponed

Not for Drupal 6.

The approaches taken in this patch were interim and designed for Drupal 6. A fresh start after code freeze, based on schema information, might learn something from this patch but should start at a much more basic rewrite.

KarenS’s picture

This is a very interesting concept. I think maybe some of this could be a part of our next CCK in core effort -- standardizing the data object itself might be a nice starting point for creating a standardized method to add fields to any kind of data.

Anyway, just subscribing so I don't lose track of the idea.

wuf31’s picture

subscribing

Frando’s picture

Version: 6.x-dev » 7.x-dev
Status: Postponed » Needs work
the greenman’s picture

subscribing

beginner’s picture

subscribe

Pancho’s picture

subscribe

yojoe’s picture

subscribe

traxer’s picture

subscribe

hswong3i’s picture

subscribe

dropcube’s picture

Subscribing.

RobLoach’s picture

Planning on using parts of this in Services for Drupal 7, so subscribing.

alex_b’s picture

subscribing.

RobLoach’s picture

Will there be a Block Data API? drupal_load('block')

Owen Barton’s picture

Subscribe

amitaibu’s picture

subscribe

cfennell’s picture

subscribing

BioALIEN’s picture

Subscribing. What's the update on this? Is some of this moving to CCK as per KarenS's post in #29?

RobLoach’s picture

What happened to this? #225450: Database Layer: The Next Generation is now in, which will likely make the Data API much easier to implement....

litwol’s picture

With 2 major changes in core (dbtng and testing), i think this needs to be moved to HEAD. i see little hope of this getting into core before drupal 8.

p.s. 'VERSION' select box does not offer 'HEAD' as a value. it would have been nice to have something to symbolize a version past the current development.

webchick’s picture

I don't see why we couldn't get this in 7.x. Code freeze is an indefinite period of time away. Of course, the sooner people start the better the chances of this are. :)

RobLoach’s picture

What I don't like about this patch is how $revisions is a parameter in drupal_load, as it's only unique to loading nodes. What if we used:

/**
 * Load an object.
 * @param $type
 *   The type of object to load.
 * @param ...
 *   Arguments to pass to the load function (like an ID or an associative array).
 * @return
 *   A fully-populated object.
 */
function drupal_load() {
  $args = func_get_args();
  $type = $args[0];
  unset($args[0]);
  $object = module_invoke($type, 'load', $args);
  module_invoke_all($type, &$object, 'load', $args);
  return $object;
}

Then we could still pass the revisions argument, while keeping it abstracted for any other data type.

RobLoach’s picture

FileSize
71.53 KB

Had a big hack at it from scratch using nedjo's earlier patch as a model. I started to slow down as I got into the taxonomy. Comments and nodes work..... Help appreciated. Lots of tests failing ;-) .

BioALIEN’s picture

Status: Needs work » Needs review
RobLoach’s picture

Status: Needs review » Needs work

This needs a lot of work...

recidive’s picture

Now that hook_nodeapi() and hook_user() are gone, we are a step forward in enabling this.

One possible battle plan for making this happen:

Focus this first patch on node objects and make a consistent data api implementing the drupal_load(), drupal_delete(), etc wrappers.

File separated issues for:

Renaming drupal_load() to drupal_load_file().
Renaming hook_nodeapi_* to hook_node_* for consistency.
A patch for each other objects (user, vocabularies, etc) refactoring their APIs integrating them with the new generic one.

I've filed an issue for renaming hook_nodeapi_* to hook_node_* and to make node APIs more consistent.

RobLoach’s picture

Status: Needs work » Active

This is pretty pwniful for standard loading of multiple data content types: #460320: Standarized, pluggable entity loading (nodes, users, taxonomy, files, comments).

RobLoach’s picture

Status: Active » Closed (duplicate)