Well this seems to be a pretty deep, and potentially ugly bug, so I'll take some time here to try and describe it first at least. I spent the day in xdebug and have a reasonably good idea where trouble is happening in the FAPI flow now, but I am hoping someone with more knowledge of cck internals can help me with the next steps.

Here is a scenario first:

With CCK Content Permissions enabled, take and create node type with a multi-value integer field using checkboxes. (Although this seems to effect a variety of other fields like date field also) Let's say this field has possible values of 1,2,3,4,5.

Now for this field we'll give user "A" edit permission, and user "B" NO edit permission.

User "A" comes and creates a node titled "Test" and chooses values of 1,2 & 4.
User "B" comes and edits that node changing the title to "My Test" and submits it.

Out multi-value field in that node now has a values of 1,1 & 1.

If either user edits it again it collapses to a single value of 1.

If this would have been a CCK date field with an initial user "A" value of "2008-08-21 00:00:00", after user "B" would have edited the title, the date would be set to "0008-08-21 00:00:00".

In other words it's a bizzare mangling of data because of an incorrect array value assignment for users without edit permission which ultimately get set during form_set_value() runs.

All this processing takes place in drupal_process_form(). To see the interesting differences, take a look at the difference in $form "values" for a user with edit permissions and a user without. Do so after the following line is run in drupal_process_form():

$form = form_builder($form_id, $form, $form_state);

You will see that value data is different. For example using the scenario above...

User "A" (with field edit permission) would have this in is $form after form_builder is run:

...[#value] => Array
        (
            [value] => Array
                (
                    [1] => 1
                    [2] => 2
                    [4] => 4
                )

        )

...[value][#default_value] => Array
                (
                    [1] => 1
                    [2] => 2
                    [4] => 4
                )

And ultimately

...[value][#value] => Array
                (
                    [1] => 1
                    [2] => 2
                    [4] => 4
                )

Where User "B" (without edit permission) would have:

...[#value] => Array
        (
            [value] => Array
                (
                    [0] => 1
                    [1] => 2
                    [2] => 4
                )

        )

...[value][#default_value] => Array
                (
                    [0] => 1
                    [1] => 2
                    [2] => 4
                )

And ultimately

...[value][#value] => Array
                (
                    [1] => 1
                    [2] => 1
                    [4] => 1
                )

Using date fields it's even more obvious as the key structure is more dramatic.

User "A" (with field edit permission) would have this in is $form after form_builder is run:

...[#value] => Array
                (
                    [value] => Array
                        (
                            [date] => 08/21/2008
                        )
                )

...[value][#default_value] => Array
                        (
                            [date] => 08/21/2008
                        )

...[value][#value] => Array
                        (
                            [date] => 08/21/2008
                        )

Where User "B" (without edit permission) would have:

...[#value] => Array
                (
                    [value] => 2008-08-21 00:00:00
                )

...[value][#default_value] => 2008-08-21 00:00:00

...[value][#value] => 2008-08-21 00:00:00

So how the form is getting built, with existing values for users without edit permission, during form_builder() phase is creating differences in keys and array structure, which ultimately make the calls to form_set_value() set bogus data.

I'm sorry this is so long winded, but I literally spent the entire day trying to track this down. (As I really need permissions to work in my current project.) I'm happy to help here as much as possible, but would be eternally grateful if someone with more cck internals experience could join in.

Any input is appreciated!

As a bizarre final side note on the date mangling, this is literally what's happening to the dates during the ultimate _form_set_value() iteration:

// What's in $form_values incorrectly because of incorrect array structure
$value = "2008-08-21 00:00:00";
// What gets set via _form_set_value()
$value['date'] = "08/21/2008";

Results in $value = "0008-08-21 00:00:00"

Really wierd huh ? :D

CommentFileSizeAuthor
#17 cck.patch3.39 KBKarenS
#16 optionwidgets.module.patch2.64 KBKarenS
#5 optionwidgets-access-defaults.patch1.01 KBMoonshine
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

KarenS’s picture

KarenS CreditAttribution: KarenS commented

The common thread between optionwidgets and date is that both of them do some pre- and post-processing of the values that may be getting skipped, so I think that's the key.

I'll try to find time to dig into this to see what happens to these values for users without permissions, but I'm pretty sure that's where we need to be looking.

Moonshine’s picture

Moonshine CreditAttribution: Moonshine commented

Thank you!

I've started looking looking deeper with options widgets using checkboxes to start. It does appear that on *submit* when processing optionswidgets_buttons_process(), users without edit permission have values that get run through optionwidgets_data2form(), while users with permissions are not.

What ends up in $element['#value'] is then different after that point:

User without edit permissions

Array
(
    [value] => Array
        (
            [0] => 1
            [1] => 2
            [2] => 3
            [3] => 4
            [4] => 5
        )

)

User with edit permissions

Array
(
    [value] => Array
        (
            [1] => 1
            [2] => 2
            [3] => 3
            [4] => 4
            [5] => 5
        )

)

====

EDIT: Well there's more to it then just getting $elements constructed properly in optionwidgets_buttons_process(). I added the following in there:

  // Preprocess value data if form was submitted by someone without edit permissions
  if ($form_state['submitted'] && !isset($element['#value'][$field_key])) {
    if (is_array($element['#value'])) {
      foreach ($element['#value'] as $item) {
        $values[$item[$field_key]] = $item[$field_key];
      }
    }
    $element['#value'] = array ($field_key => $values);
  }

Which brought the value array in line with a user with edit permissions, but it still doesn't apply values properly.

If the following code is added to the above code:

    $element ['#access'] = TRUE;
    $element['#post'][$field_name] = array ($field_key => $values);

It does get saved properly, but obviously that's just a dangerous hack. So I'm continuing my search, past this function...

KarenS’s picture

KarenS CreditAttribution: KarenS commented

The difference between users with edit permissions and those without is that the form element gets #access set. So we need to figure out exactly what steps happen or don't happen in that case.

Fields that should not be edited should be treated like hidden fields -- not processed at all, just carry the values around unaltered. Either that or keep them out of the update array so they don't get updated when the node is saved.

We may be in core patch territory here, because if data protected by #access doesn't retain its original values when the form is saved, it is a bug.

I don't have time to dig into this right now, unfortunately, but I think there may be some deep deep FAPI analysis needed to see if this is really a core FAPI bug.

Moonshine’s picture

Moonshine CreditAttribution: Moonshine commented

Thats fine, I'm going to keep trucking on this today though as it's sort of a deal breaker here.. :) Deep is right though... I've believe I've narrowed it down to the following FAPI code, where value processing diverges based on #access.

The user *without* edit rights ends eventually goes through the "// Load defaults." section where #value is created as a keyed array of 1's:

Array
(
    [value] => Array
        (
            [1] => 1
            [2] => 1
            [3] => 1
            [4] => 1
            [5] => 1
        )

)

Whereas the user with edit access retains his "cck style" (?) value from the #post.

Array
(
    [value] => Array
        (
            [1] => 1
            [2] => 2
            [3] => 3
            [4] => 4
            [5] => 5
        )

)

This happens in form.inc : function _form_builder_handle_input_element()


  if (!isset($form['#value']) && !array_key_exists('#value', $form)) {
    $function = !empty($form['#value_callback']) ? $form['#value_callback'] : 'form_type_'. $form['#type'] .'_value';
    if (($form['#programmed']) || ((!isset($form['#access']) || $form['#access']) && isset($form['#post']) && (isset($form['#post']['form_id']) && $form['#post']['form_id'] == $form_id))) {
      $edit = $form['#post'];
      foreach ($form['#parents'] as $parent) {
        $edit = isset($edit[$parent]) ? $edit[$parent] : NULL;
      }
      if (!$form['#programmed'] || isset($edit)) {
        // Call #type_value to set the form value;
        if (function_exists($function)) {
          $form['#value'] = $function($form, $edit);
        }
        if (!isset($form['#value']) && isset($edit)) {
          $form['#value'] = $edit;
        }
      }
      // Mark all posted values for validation.
      if (isset($form['#value']) || (isset($form['#required']) && $form['#required'])) {
        $form['#needs_validation'] = TRUE;
      }
    }
    // Load defaults.
    if (!isset($form['#value'])) {
      // Call #type_value without a second argument to request default_value handling.
      if (function_exists($function)) {
        $form['#value'] = $function($form);
      }
      // Final catch. If we haven't set a value yet, use the explicit default value.
      // Avoid image buttons (which come with garbage value), so we only get value
      // for the button actually clicked.
      if (!isset($form['#value']) && empty($form['#has_garbage_value'])) {
        $form['#value'] = isset($form['#default_value']) ? $form['#default_value'] : '';
      }
    }
  }

I'm still hunting here, but it almost seems like #afterbuild may be a way around this. Although I've heard chx say that should dissapear and has no possible valid use. :(

But it still seems curious why CCK checkboxes seemingly use a different value structure then what FAPI does internally when it builds out defaults via form_type_checkboxes_value().

Still on the hunt here..

Moonshine’s picture

Moonshine CreditAttribution: Moonshine commented
Assigned: Unassigned » Moonshine
Status: Active » Needs review
FileSize
optionwidgets-access-defaults.patch1.01 KB

Well, after further digging it seems pretty clear that how FAPI handles default value creation for checkboxes vs. the format that cck uses is the disconnect here.

During my traces, I came to the optionwidgets_validate() function where there is already some form value processing taking place. At first glace this seems like the proper spot to add a fix for #access conditions. So here is an intial patch to take a look at.

Feel free to let me know if I'm way off base... :)

moshe weitzman’s picture

moshe weitzman CreditAttribution: moshe weitzman commented

@moonshine's approach looks about right, though karenS seems to indicate that she wants core patched, not optionwidgets. The core patch thats needed might be a lot like #227966: Use default values to #disabled form fields.

yched’s picture

yched CreditAttribution: yched commented 
+      $updated[$key]['_error_element'] = $element['_error_element']['#value'];

This looks strange. $element['_error_element'] is only here to help pointing validation errors in hook_field('validate') to the right form delta. Building anything in it feels like a hack.

Moonshine’s picture

Moonshine CreditAttribution: Moonshine commented

Actually I just put that in there based on what was happening via the normal optionwidgets_form2data() call:

....
// Reinject the '_error_element' so that hook_field('validate') knows
  // where to flag an error.
  if (isset($element['_error_element'])) {
    foreach ($result as $key => $value) {
      $result[$key]['_error_element'] = $element['_error_element']['#value'];
    }
  }

Although I would think you're right, default values shouldn't have an occasion to error normally(?).

Moonshine’s picture

Moonshine CreditAttribution: Moonshine commented

@moshe, that's an interesting read.

One comment really struck a cord with me though:

the Form API has a #value_callback for a reason: not having to deal with specifics of each field type in generic functions like _form_builder_handle_input_element().

So really I'm not entirely convinced this is a core bug. To me it feels like CCK is making use of checkboxes (defined in core) in a special way regarding values. It currently seems to do so via some special processing in it's custom submit and validation functions. This works fine when values are present in #post and the user has #access.

However, when #post values aren't present and the user doesn't have #access, cck's getting tripped up by what core's returning via it's default checkboxes value function. Which makes me wonder about CCK having it's own value function to handle the defaults as it expects.

But honestly I'm way too new to Drupal to feel like I know anything definitive about anything in FAPI yet. This is literally my first trip though it here, and the debugger is still smoking. :)

yched’s picture

yched CreditAttribution: yched commented

OK, got it. This is a censored version of what's done in optionwidgets_form2data().
Patch is not too clear, then. I'd suggest adding an optional param to optionwidgets_form2data() so that it runs in 'censored' mode.

Side note : I'm only talking aesthetics and readability here, not about whether this is the right fix or not.

Moonshine’s picture

Moonshine CreditAttribution: Moonshine commented

No problem.. I'll re-work later tonight. Just two things:

1) Do you really want an extra param to optionwidgets_form2data()? It's already being feed $element so it could apply the #access logic itself.

2) What is your final feeling about the _error_element's when it is running in "censored" mode?

thanks... and I understand this may not be "the" fix... I'm just happy it solved my problem for now. I need to move on to date field next... :)

KarenS’s picture

KarenS CreditAttribution: KarenS commented

First, I'm not saying this is a core bug, just that it potentially might be, depending on what we uncover about the way #access works.

Second, I dug into form.inc to see exactly what is different about the processing when you use #access and when you don't, and there are exactly two places that are different:

1) #value_callback gets skipped

2) The form is marked that it doesn't need validation, which should mean #element_validate gets skipped

That should mean that the patch at #5 won't have any effect at all, because why would the validation function get called on a form that needs no validation, so I'm puzzled about that.

So one workaround that would be within the realm of the way #access looks like it is intended to work would be to do the initial transformation of the array in #value_callback instead of #process. Then you won't need to unwind it in #element_validate because both those steps will be skipped. But this won't work if #element_validate is *not* getting skipped.

If that doesn't work, another alternative is to check for #access in #process and skip the transformation there. If it's never transformed, it won't need to be unwound.

I think the goal should be to one way or another leave database values completely untouched if #access applies.

KarenS’s picture

KarenS CreditAttribution: KarenS commented

Oops, I got that wrong. #value_callback *is* called in all cases. Here's what's different if there is no #access:

If no #access and #value isn't set already,
- create $edit from #post
- set #value to the #post values
- mark the form to need validation

Now that I corrected that, I'm going to try to follow what's happening in optionwidgets and see what we could do differently.

KarenS’s picture

KarenS CreditAttribution: KarenS commented

More digging, if #access is set, #needs_validation is not set. The result of that is that core validation gets skipped, and the core validation that gets skipped is the processing needed to clean up checkboxes and radio values. This does *not* skip #element_validate, as I assumed it would, which is why we still have a value coming to #element_validate, but the incoming value won't match the value that got passed through the core checkboxes processing, which is why the array values look odd.

Not sure yet what is the best solution, I'm still digging through this.

KarenS’s picture

KarenS CreditAttribution: KarenS commented

Well I can make things work correctly by checking the state of #access in both #process and #element_validate and then just set the element['#value'] to the database value and skip all other processing. But that means every field module that might be affected by this needs to understand how #access works and where their modules might get broken and go through and clean all these things up.

I think we need to back all the way back to the content module, where it creates the form in content_field_form(). If #access will be turned off, we should not create a widget at all, we should just set #value at that point and skip all further processing. This would fix things for all modules automatically, since the only time content_widget_invoke() would get involved would be when we're dealing with a user who has access to the field and we actually need to add a widget to the form.

I just need to tweak content_permissions to intervene in content_field_form() instead of form_alter(), since content_permissions' form_alter() happens too late to do any good.

KarenS’s picture

KarenS CreditAttribution: KarenS commented
FileSize
optionwidgets.module.patch2.64 KB

I made two patches for the two ways this can be fixed. Both work. The question is which is the better route.

The first is to alter optionwidgets to pass the value on unchanged. If we do it this way, widget_invoke and widget processing will still happen (for better or worse) and widget modules will have to figure out how to protect their values.

KarenS’s picture

KarenS CreditAttribution: KarenS commented
FileSize
cck.patch3.39 KB

The second is to fix this at the content_form level. If fixed this way, widget_invoke() will not be called if the user has no access.

This seems better in many ways because individual field and widget modules don't need to do anything, but if there is some reason widgets *need* content_widget_invoke() called in that case, it won't happen.

KarenS’s picture

KarenS CreditAttribution: KarenS commented

Also note that content_field_form() could be used as an API to create field forms in various places (not necessarily just the node form), a module could use it to move a field form into a panel or block or whatever, and the content module uses it in the AHAH add more button.

The current hook_form_alter() method used by content_permissions will miss any of these that don't end up placing the field in a node form, so fixing access control in the content_field_form() function instead of using hook_form_alter() on the node form has another benefit.

Except that if the function is called via ajax we may not have the right permission info. What's the right way to control permissions properly on ajax-provided data?

Moonshine’s picture

Moonshine CreditAttribution: Moonshine commented

Wow.. Thanks KarenS. I really like #17 a lot. Working well here with checkboxes, date field, ... everything so far. Seems to me like a very clean approach. I even understand it. :)

It certainly changes value behavior for users without #access. The first thing I noticed with the patch in place was related to computed fields, where you may now have a previous value set for users without #access before the "computed" code has been run. Really just a minor change in thinking needed there, and ultimately there is lots of flexibility with that field and permission settings.

I wish I could help you brainstorm ajax data but I wouldn't know where to begin.. :/ I owe for your work though. I know how busy you guys are! :D

yched’s picture

yched CreditAttribution: yched commented

Karen : agreed that fixing this in content_field_form() looks like the best way.

but if there is some reason widgets *need* content_widget_invoke() called in that case, it won't happen.

We probably need to check filefield / imagefield stiil work with #17. IIRC, they do funky stuff in hook_widget().

I'm not sure I see how AJAX is a problem ?
Why wouldn't we have the right info ?

Side note :

$addition[$field_name] = array(
  '#access' => $access,
  '#type' => 'value',
  '#value' => $node->$field_name,
);

We probably don't need to set '#access' to FALSE for a 'value' FAPI element, right ?

KarenS’s picture

KarenS CreditAttribution: KarenS commented
Component: content.module » Content Permissions module

If we use ajax to get the value, there is no info about the user in that request, or is there? If the ajax session knows who the user is, that's fine, no problem.

I set the #access in case other modules are doing anything with hook_form_alter so it retains the access control, or at least so an intelligent module could have a way to check if access is allowed. Maybe not needed, but won't hurt anything.

I added a new component for Content Permissions.

yched’s picture

yched CreditAttribution: yched commented

No problem, a request that comes from AHAH comes with the right session info, we have a $user while processing it, so permission check works as usual.

KarenS’s picture

KarenS CreditAttribution: KarenS commented

I looked through the filefield and imagefield code and don't see anything that looks like it would be a problem if hook_widget() was skipped completely for users who don't have edit permission. The only thing it appears to do is some validation, which won't be needed if the value isn't getting edited.

But that's some complex code, so there could be issues I'm missing. I'll try doing some tests and hope I can at least tell if it would be hopelessly broken by this patch.

KarenS’s picture

KarenS CreditAttribution: KarenS commented
Status: Needs review » Fixed

Well, it seems that imagefield and filefield still need a lot of work, so it's hard to be sure about whether bugs are introduced, but I could create 'protected' image fields and they are not visible to the user who should not see them on either the node or in the form, and when the user without access saved the form, it did not destroy any data, so I think it's probably OK.

CCK is broken now for *all* fields touched by users without permission, worst case I am fixing all other fields and (maybe) creating some problems for imagefield and filefield. And since imagefield and filefield are not considered production-ready yet, there is time to fix whatever comes up.

So I'm going to go ahead and commit the patch from #17.

Moonshine’s picture

Moonshine CreditAttribution: Moonshine commented

Excellent! Definitely don't let Imagefield/Filefield hold anything up. They have their own problems. :) At this point they aren't even doing file cleanup when files are updated or removed.

Thanks again.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for two weeks with no activity.