There are a few problems with eck__entity__form_submit() function.
...
foreach ($properties as $property => $info) {
$form_value = _eck_form_property_value($state, $property);
if (isset($form_value)) {
...
$vars = array('data' => $form_value);
$data = eck_property_behavior_invoke_plugin($entity_type, 'pre_set', $vars);
if (array_key_exists($property, $data)) {
$form_value = $data[$property];
}
...
$wrapper->{$property}->set($form_value);
}
}
The "pre_set" is called too many times. If we take a look of how eck_property_behavior_invoke_plugin was written, it loops trough all properties. However, in the submit function we are already in a loop for submitted property/value pair.
The submitted value is passed to all "pre_set" callback functions of all properties implementing it. We should only invoke the pre_set for the current property with it's value.
I think we need another invoke funcion for this particular callback, to allow calling a function for a property with it's value, and not for all properties.
Also, this issue fixed some cases where a submitted value could be empty.
https://www.drupal.org/node/2209587
However, NULL values are not processed. If NULL can't be assigned to a property as value, I don't see how existing int column values could be set from the existing value to NULL.
Thanks,
Mihai
Comments
Comment #1
mihai_brb commentedI've started working on this and did the following.
First, the eck_property_behavior_invoke_plugin was split and code was moved to a sub-function called eck_property_behavior_invoke_property_function.
The functions then do:
eck_property_behavior_invoke_plugin - invokes a function for all entity properties and uses
eck_property_behavior_invoke_property_function - invokes a function for one property.
Second, I've replaced the condition that checks the property value in eck__entity__form_submit. Here, we should allow passing a NULL value, a case that did not passed the previous condition.
Then, in the properties loop, I used the new eck_property_behavior_invoke_property_function to invoke the pre_set for the current property.
It is still a subject of discussing how should be passed the property value. I used an array keyed 'value', but we might as well use the property name as key.
Mihai
Comment #2
mihai_brb commentedComment #3
fmizzell commented@mihai_brb Thank you for finding this. A couple of things: First, the function eck_property_behavior_invoke_plugin takes an extra argument called $specific. The purpose of that argument is to give the function data that needs to be passed to our behavior implementors that is specific to a property. I didn't even remember that argument, and I don't think is particularly pretty, but that arg could be used to pass the value for a specific property without having to refactor eck_property_behavior_invoke_plugin.
So the idea would be to take the call to eck_property_behavior_invoke_plugin outside of the look, and use the look simply to build this $specific array to pass the appropriate values to the appropriate behaviors.
I have nothing against refactoring, but as none of the ECK behaviors are using this specific pre_set hook, I imagine that it was requested by a behavior in another module, so it would be great if we can be as careful as possible and not break their stuff.
The second issues mentioned here is the need to have NULL as an acceptable value. When is this an issue? I do believe that NULL should be an appropriate value for a property, but I am wondering if NULL ever comes about when dealing with the UI. When you have an int property with a widget, and you delete its value, does $state['value'][
] get set to NULL?
Let me know what you think.
Comment #4
mihai_brb commentedHi @fmizzell,
Thanks for your feedback.
The "pre_set".
I understand the usage of $specific in plugin invoke function. But why would I want to invoke the callback for an out of context property?
The callback is called anyway even with $specific properly used. The specific only helps passing the right data to the callback.
And why would I want to call a pre_set for all properties that many times? Every behavior implementing a pre_set will be called as many times as the number of properties present in form_state.
To conclude, I doubt that someone can use "pre_set" the way it is now. Not only that one property value is called too many times, even for properties that are not in form_state, but in the pre_set callback you don't have the relevant context(for what property the value is).
I don't necessary intend to use pre_set. I only wanted to use it as a work-around to be able to set empty property values as NULL. I then realized the above problems.
The NULL value.
In my case, NULL gets in form_state trough default_value or from a validate function.
For example, for a date (timestamp) property stored as int, not required. Deleting the existing property value requires to set its value in form_state as NULL. If you pass an empty string you get EntityMetadataWrapperException: Invalid data value given triggered from the eck submit function.
Another example is for a property referencing a user, again int, not required. Deleting the existing value means to set it as NULL.
The way I temporary "fixed" it is to have set it to 0. This is not ideal because in both cases zero can mean something else.
Thanks,
Mihai
Comment #5
fmizzell commented@mihai_brb Thank you for clarifying the context of the NULL usecase. Let me write a little code later today to show you what I am thinking. It will be true that the pre_set hook will be called for properties implementing the hook even when no value was given in the $state['values'] array, but It should only be called once. I will post the patch and maybe we can discuss further from there.
Comment #6
fmizzell commentedComment #7
fmizzell commented@mihai_brb Sorry, It took so long, this patch does not address the case in which a value needs to be set to NULL, just the problem with calling the pre_set behavior too many times.
Let me know what you think.
Comment #8
mihai_brb commentedHello. I've tested and this fixes the pre set problem. Is there anything we can do with null values ? That was my main and initial problem :) And with this patch pre set won't help any more ...
Thanks.
Comment #9
fmizzell commented@mihai_brb, Ok, I will look at your patch and incorporate your work with mine, to solve the NULL problem.
Comment #10
fmizzell commented@mihai_brb Finally!!! sorry for the delay, but this should do it.
Comment #11
fmizzell commentedComment #12
fmizzell commentedComment #13
mihai_brb commented@fmizzell thanks, but unfortunately this still does not solve the problem.
Mainly because you are using isset() on form values, and that returns FALSE for NULL. I'm using array_key_exists() instead.
Attached another patch that works for me, with some comments that should make the process readable. Please let me know in case of any questions.
ps: _eck_form_property_value is not used any more, it could be removed, but not included in this patch.
Comment #15
fmizzell commented