There are two problems I have with the current implementation of the user update service:

  1. Since it's directly modeled after the user profile form, it can take multiple update calls to affect change on a single user. I don't really like the way core does this either. (Hopefully it's been remedied in later Drupal versions. Admittedly, I haven't looked beyond 6.)
  2. You always have to pass values for every field that would appear on the form, even though you may only want to update the value of a single field, otherwise existing values will get wiped out and required fields will give you validation errors.

Both of these make it pretty difficult for an outside entity to make use of the service.

My ultimate wish is that you could affect change on just the fields you send to the service, leaving any existing values as-is, and using only one call, no matter what category the fields might fall under.

CommentFileSizeAuthor
#1 refactor-user-update-1910012-1.patch5.03 KBkevin.dutra
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

kevin.dutra’s picture

kevin.dutra CreditAttribution: kevin.dutra commented
FileSize
refactor-user-update-1910012-1.patch5.03 KB

Here's a patch that basically does what I want it to. It's not perfect, but hopefully it will at least serve to start some discussion.

The basic idea is that edits submitted to the service are merged with the existing user object values. This should prevent the issue with data being wiped out or with required fields complaining because you didn't submit a value. (True, there'll still be the case where a field has just become required and you're editing a user that doesn't have a value there yet. That seems fine to me.)

Then the every profile form is submitted so that it only takes one call to the service.

kylebrowning’s picture

kylebrowning CreditAttribution: kylebrowning commented
Status: Active » Needs review
marcingy’s picture

marcingy CreditAttribution: marcingy commented
Version: 6.x-3.x-dev » 7.x-3.x-dev
Status: Needs review » Needs work

All these changes need to go into D7 first and then be backported

This hunk worries me as it has a db query why not call profile_categories() instead - yes it does pretty much the same but leaves us with less code to maintain.

+  // Assemble a list of categories
+  $categories = array('account');
+  if (module_exists('profile')) {
+    $result = db_query("SELECT DISTINCT(category) FROM {profile_fields}");
+    while ($category = db_result($result)) {
+      $categories[] = $category;
+    }
+  }

so effectively we become

$categories = module_exists('profile') ? profile_categories() : array();
$categories[] = 'account';

Another strange thing is

$form_state_copy = drupal_clone($form_state);

Can we not just assign.

kevin.dutra’s picture

kevin.dutra CreditAttribution: kevin.dutra commented

This hunk worries me as it has a db query why not call profile_categories() instead - yes it does pretty much the same but leaves us with less code to maintain.

That's definitely an option. Note that profile_categories() gives you an array of arrays

function profile_categories() {
  $result = db_query("SELECT DISTINCT(category) FROM {profile_fields}");
  $data = array();
  while ($category = db_fetch_object($result)) {
    $data[] = array(
      'name' => $category->category,
      'title' => $category->category,
      'weight' => 3,
      'access callback' => 'profile_category_access',
      'access arguments' => array(1, $category->category)
    );
  }
  return $data;
}

so it wouldn't be quite as simple as 

$categories = module_exists('profile') ? profile_categories() : array();
$categories[] = 'account';
Can we not just assign.

Yep, I believe you're correct. $form_state should be an array, not an object, so it shouldn't require cloning. Not sure why I did that...

kylebrowning’s picture

kylebrowning CreditAttribution: kylebrowning commented
Issue summary: View changes
Status: Needs work » Closed (won't fix)

Is this still wanted? Patch doesn't apply.