Create new boolean field "Cannot create references to/from string offsets nor overloaded objects"

Confirmed. The code in question was introduced in #763376: Not validated form values appear in $form_state.

Let's get some tests for Boolean fields, then...

Hm.

http://api.drupal.org/api/function/list_field_settings_form/7 is doing some black magic here and I'm not sure if we want to support that or not.

By default, the allowed values form is a textarea, where you can enter key|value combinations for that list.

In case this is a boolean field, it changes $form['allowed_values'] to a markup element (strange thing #1), sets #input = TRUE and a #value_callback and adds textfield sub-elements (yes, exactly, subelements on a markup form element. As I said, black magic).

Then in the value_callback, it takes the values from both textfields and assembles them into the same format that other lists use: 0|$off\n1|$on.

So far, this works.

However, now the form system tries to set the value for ['allowed_values']['on'] and ['allowed_values']['off']. And now, everything explodes because it tries to set $form_state['values']...['allowed_values']['on']. The problem is that $form_state['values']...['allowed_values'] is already set and it's a string. KABOOM.

I have two possible solutions and both are a hack, so better solutions are welcome. Maybe we should just special case the allowed_values handling for boolean fields and store on/off as an array.

The patch contains both solutions, each of them fixes the fatal error:

1. Change drupal_array_get_nested_value() to say that the element exists but not return anything in case we detect a string. Nasty, but works...
2. Add a #process function on allowed_values and set #input of the on/off fields to FALSE *only* during input processing. Because doing that always hides the default value.

We had the same problem with the form element example module, with the same error appearing only when an element is defined with the property '#tree' set to TRUE. (just enabling the form example module from examples project would reproduce the fail). The patch at #4 fixed the problem at all and the FAPI loves our created elements again..

The fix for the includes/common.inc in the patch was the only think we could test and that removed the problem in our case.

The textarea based UI here is awful. Would be nice to ditch that while fixing this bug. One of our UI guys gurus a mockup for this I think.

subscribe

Subscribe, and:

+++ includes/common.inc
@@ -5751,7 +5751,13 @@ function drupal_array_get_nested_value(array &$array, array $parents, &$key_exis
-    if (isset($ref[$parent]) || (is_array($ref) && array_key_exists($parent, $ref))) {
+    if ((isset($ref[$parent])) || (is_array($ref) && array_key_exists($parent, $ref))) {

What do the extra brackets around isset($ref[$parent]) do?

Powered by Dreditor.

The only potentially usable way I can come up with is to actually show what is meant:

But it will be hard to find the fine line between 'for demonstration purposes only' and 'WTF this checkbox won't click'. I suggest to focus on fixing the actual bug and leave the ui be for the scope of this issue.

$ref is a string is invalid FAPI. We do not want or need to support that. A FAPI child can not be a string.

#4: happy_hacking.patch queued for re-testing.

Needs work per #10.

Of the two hacks the one that keeps the weirdness confined to list.module seems the better of the two, just hacked the common.inc bit from Berdir's earlier patch out and reattaching so I can set this back to CNR.

If the UI here is in line for being completely refactored in D8, then just making it work without it spilling out into the base system seems enough to get rid of the critical. Tests still need adding.

I agree that the list.module hack is better than hacking form.inc but it's still a hack, so if someone can come up with a better solution to this... :)

Also, this seems to be a (small) regression in form.inc to me. Basically, something that did work until this point doesn't work anymore now. examples.module did this too and it is possible that other modules are affected too (I've seen an error in rules.module related to these new nested array functions and isn't date.module doing similiar stuff by converting a single element into multiple elements?).

Here's a little test. Not too sophisticated (in that it doesn't check that the correct labels for 'On' and 'Off' actually end up on the node edit form), but it definitely would have caught this bug and any other one that simply breaks the whole form.

Oh, and, for reference, the issue that introduced this "black magic": #456640: Clarify text descriptions in Field UI for booleans

I am at a complete loss of what happening here and why this hack helps. From the original hack one would think that we ended up with a $form['foo']['bar'] that is not an array. How? Why? Why we can't fix it? What does not input magery do?

Now I re-read the issue, the problem is that we have this weird markup element with input TRUE and a value_callback. Why can't we fix the value_callback?

Yes, exactly. First form api makes a string for the parent element in $form_state['values'] and then tries to insert something into that string because it assumes that it is an array, as it has child elements. The hack makes form api ignoring these child elements *only* during input processing. Fix the value callback how? :)

From what I understand it, the value_callback converts the on/off array into a string value so that it is consistent with the the other possible list types. (As boolean is just a special case of a list with exactly two possible options). So if we change the value_callback, that means we have handle it specially everywhere else.

That might not even be that complicated, but I haven't tried it.

Still needs more, more tests. But IMO this is a solid little patch.

Edit: #value_callback is setting up the value of one element and doing nothing else. This form needs a lot more hence we use #process.

Nice, just a small question....

+++ modules/field/modules/list/list.module	2010-09-30 17:37:11 +0000
@@ -156,13 +156,15 @@ function list_allowed_values_setting_val
+    $element['on']['#value'] = $on;
+    $element['off']['#value'] = $on;

I don't understand this part...

- Why do we still need to assign #value here?
- And why does this assign $on to both on and off?

Powered by Dreditor.

Just to mention that..

This last patch does not fix the issue in examples module, and we are still getting the Fatal error: Cannot create references to/from string offsets nor overloaded objects in C:\webdev\www\dev7\includes\common.inc on line 5756, when trying to combine several fields (an array) into a single string. Berdir already commented this in #14.

#870906: Explain #tree in element example (and rework Element Example!) the patch at #19, creates a custom form field element. Its value is a string, but for better understanding of the user the field split that string into three different parts, creating an array of textfields. DamZ and rfay wrote that part, I don't know if there is other way to get this example working with patch at #20 but doing the right way.

edited: the error in examples has been solved (for now) using a '#value_callback' function.

subscribe

Ok, here is a new approach.

The on/off value fields are not added below the allowed_values field but on the same level as that. Then we can keep the #value_callback function, the only difference is that we have to get the on/off values from $form_state.

This worked for me, but I only did very minimal manual testing. The previous patch always ended up in a wsod for me, not sure why the tests passed. Needs more tests anyway...

Updated the tests to use the new element names.

Hmmm my new issue seems in a way related if you check the error message...

#932648: Ajax warning when uploading to managed_file field

Subscribe -- let me add that the 6.x contrib modules are full of problems like this, PHP5.3 doesn't like it.

This version looks much better to me.

Now that we have drupal_array_get_nested_value -- can it be used to get get rid of $form_state['input']['field']['settings']['on']; please?

Edit: so the value should look its siblings instead of hardwiring.

No webchick, patch at #26 does not make fix examples module. I'm asking myself if this could also be an examples module issue, I mean, I'm not sure if that specific case (the element form) is done in the right way. Berdir's patch at #4 does fix our case, as I mentioned, because of the array issue he comments.

I'd someone with more experience then me in form API review the element in the examples module (it is patch #19 of #870906: Explain #tree in element example (and rework Element Example!)).

@31: That is on purpose. The new patch does not change form.inc but only fixes the crazy stuff that list.module is doing. So you need to change examples.module too.

Re-rolled for #30, tests pass locally.

Sorry for coming late to this (back from vacation) - and my bad for letting the weird $form structure in to begin with.

Agreed with the fix. Just added two comment blocks, for later maintenance sake.
- one in list_field_settings_form(), wording the overall logic (should have been done in the patch that introduced those two separate textfields, would probably have saved some head scratching in the early comments here).
- one in list_boolean_allowed_values_callback() to expand a little on the drupal_array_get_nested_value(#parents) call

As far as I'm concerned, this is RTBC.

Sorry, small problem with the current approach :
The values for the 'on' and 'off' FAPI textfields stay in $form_state['values'] along with the value computed for 'allowed_values'.
This means that the resulting $field has crufty $field['settings']['on'] and $field['settings']['off'] entries, along with the expected $field['settings']['allowed_values'].
Now that I notice this, I guess this was the reason for the 'in place replacement' approach in current HEAD...

Not only that but the drupal_get_array_nested_value() is returning null here, I have no idea why simpletest isn't logging the notice - but this means both that code and the test are wonky.

subscribing

I still need to read the issue, but I will have to sign off this patch.

I won't be able to work on this before a couple days, but :
What we want here is an input element that expands into sub elements (2 textfields), and gets one single final value in the form of a scalar (a string concatening the textfield values). That sounds pretty similar to what '#type' = 'checkboxes' does ? [edit: I rather meant 'radios']

Radios have the same name so that does not apply. I am trying to come up with something pretty.

Not only that but the drupal_get_array_nested_value() is returning null here, I have no idea why simpletest isn't logging the notice - but this means both that code and the test are wonky.

PHP WTF of the day, straight from #drupal-contribute

$a = NULL;
$b = $a['on'];

This doesn't generate a notice.

So, the test needs to be better in order to account for this.

This is the patch from #34 with tests to cover the issue.

It's good that this patch will not pass.

Let's see it failing.

The phpwtf has an entry in the PHP bug tracker http://bugs.php.net/bug.php?id=52184

Thx chx for working on this.

+ Can we also add to the test the fact that the created $field contains $field['settings']['allowed_values'] but neither $field['settings']['on'] or $field['settings']['off'] ?

Well, you are using the values of two form element to compute a third. That's not typically supported. To make it worse, the name of the third is locked. And to make it even worse, you dont want those two to be in field settings, eh? Well, I can do all that. Yay for the recursive array getter.

Well, now this can be simplified a little bit. But every bit helps.

Very nice work, chx.

+++ modules/field/modules/list/tests/list.test	2010-10-10 07:36:53 +0000
@@ -134,26 +134,40 @@ class ListFieldUITestCase extends FieldT
+    // Check that over long keys are rejected.

This is very minor (and shouldn't hold up this patch) but shouldnt this be "Check that overly long keys are rejected."

Powered by Dreditor.

Thx chx.

Sorry - minor request from #45:
Can we also add to the test the fact that the created $field contains $field['settings']['allowed_values'] but neither $field['settings']['on'] or $field['settings']['off'] ?

That sounds like a needs work. AWESOME to see this RTBC tho!!

#50, #51 done let's get this sucker in.

Great. RTBC indeed.

Other than that, I'm a bit worried at how convoluted it is to implement the pattern described in #45 : "an input element that expands into sub elements, but gets one single final value in the form of a scalar".
That's a frequent pattern notably in Field widgets, that need to rely on FAPI only to transform whatever their UI formulation is into the format expected by the field type 'schema'.
I'm thinking of date select widgets (3 selects for day, month, year --> a unix timestamp), or the 'field_example_3text' widget in field_example_field_widget_form() (3 textfields for r, g, b --> an hex color value).

Possibly not so bad for widgets, since the final forms values that are not in the schema are discarded anyway (unlike here), but it seems that some FAPI structures that worked until recently now break after #763376: Not validated form values appear in $form_state. I'm not sure the field_example_3text widget still works, for instance...

Committed to CVS HEAD. Thanks.

re @ my #54 : looks like date widgets are indeed broken - #929494-15: Everything is broken again

can others confirm this is still happening? updated from beta1 to the latest dev and still no luck when creating content that uses fields with allowed values set up. i tried deleting and recreating those fields to no avail.

I manually tested this and it was fine, the patch was also committed with automated tests. Please try a clean install of D7, try to reproduce there - if you can post exact steps to reproduce. But for now I'm closing this again.

#57 is talking about *creating content*, which I haven't tested and we don't have tests for it either.

Also, I'm still thinking that the patch that introduced these nested array functions is problematic as it is breaking things all over the place...

turns out the issue is (as suggested by #56) indeed with date fields/widgets. once i deleted any date fields, all my content types that have select list fields are working fine. thanks. i'll keep an eye on the date forums.

In my case, I'm getting the error when inputting the date selector expose filter in my views and then going to the view. Everything shuts down on that particular view and I get a white page:

"Fatal error: Cannot create references to/from string offsets nor overloaded objects in /Users/me/Sites/acquia-drupal/includes/common.inc on line 6392"

reference to a related thread in views: http://drupal.org/node/1485574

@calefilm, this issue is already closed. Please open a new issue if are having a particular problem, even if it seems related to this one.