Just seen while doing a module review that the field trigger_values became trigger_value and the content also changed. This makes an automatic upgrade (I'm currently writing) impossible or at least difficult. Is there any technical reason for changing this? I guess I could add a condition for every value, but is this really the plan? It would also required an OR for the field values and not an XOR or AND what may adds more complexity to the user.

CommentFileSizeAuthor
#18 trigger_value_multi_select-2124525-18.patch6.96 KBTobias Xy
#11 trigger_value_multi_select-2124525-11.patch5.67 KBTobias Xy
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

hass’s picture

hass CreditAttribution: hass commented

If we run an upgrade of the old to the new we may end up with hundreds of conditions only on one field. In past we allowed the user to select the available values via multi selects.

Tobias Xy’s picture

Tobias Xy CreditAttribution: Tobias Xy commented
Project: Field Conditional State (unsupported) » Field Conditional States
Issue summary: View changes
Tobias Xy’s picture

Tobias Xy CreditAttribution: Tobias Xy commented
Issue tags: +7.x-2.0 release blocker
Tobias Xy’s picture

Tobias Xy CreditAttribution: Tobias Xy commented

We probably have a problem here: It isn't possible to add multiple values of one control field to a condition as long as they are connected via AND, because each value needs a separate entry within the states array, but all of them with the same key.
I assume, that doesn't apply to OR and XOR conditions, because they are built a bit different, but I didn't tested it yet.

See also: #1149078: States API doesn't work with multiple select fields
I'm not sure whether to build a workaround (how?) or to just restrict the functionality (that would mean that it's currently not possible to correctly implement all possible conditions created in version 1.x).

Tobias Xy’s picture

Tobias Xy CreditAttribution: Tobias Xy commented

How about rewriting AND conditions as OR conditions?

Example:
A is enabled when "hello" AND "world" are selected in field B.
<=>
A is disabled when either "hello" OR "world" are NOT selected in field B.

Any opinions?

hass’s picture

hass CreditAttribution: hass commented

How about rewriting AND conditions as OR conditions?

That's not good. Let's try to identify the root cause and fix it and note the limitation in known issues. If the functionalty requires a core patch to work we document this as known issue.

hass’s picture

hass CreditAttribution: hass commented

I may missunderstand something... Not sure. This was only about the admin interface and not about the conditions in javascript... It should prevent you from creating 100 single conditions if you only need a multiselect.

Tobias Xy’s picture

Tobias Xy CreditAttribution: Tobias Xy commented

That is directly related: We don't need any multiselects in the interface if it isn't possible to use more than one value from one control field, so I posted it here.
How about providing multiselects, but with a hint, that they won't work properly in some cases? That would be the easiest way for us, I guess.

hass’s picture

hass CreditAttribution: hass commented

I believe we are talking about diffent things. I'm only talking about an admin UI usability thing, not about logic. The logic will not change.

I need a multiselect in admin ui to configure field1 will be shown if at least one of 3 specific items in a 100 items taxonomy multi-select box (field2) have been selected. If only other items have been selected in field2 the field1 is kept hidden.

Tobias Xy’s picture

Tobias Xy CreditAttribution: Tobias Xy commented

Ok, what do we have to do?
-The select fields for trigger values used by some widgets should always be multiselects.
-When more than one value is selected they are connected by the logical operator set for their state group.
-When a single control field is used in multiple states with the same trigger state (value or !value) within the same state group, only one row will be rendered with all involved trigger values within the multiselect.
-I don't think it's necessary to provide multiple text fields for widgets not using a select list for the trigger values.

Did I forget something?

Tobias Xy’s picture

Tobias Xy CreditAttribution: Tobias Xy commented
Status: Active » Needs review
FileSize
trigger_value_multi_select-2124525-11.patch5.67 KB

Finally a patch :D

hass’s picture

hass CreditAttribution: hass commented
Status: Needs review » Needs work

Got this at admin/structure/types/manage/dog/fields/field_show_in_stud_dog_list/field_conditional_state. It looks like all saved settings are lost.

Notice: unserialize(): Error at offset 0 of 1 bytes in field_conditional_state_settings_form() (Zeile 174 von sites\all\modules\contrib\field_conditional_state\field_conditional_state.admin.inc).
Notice: unserialize(): Error at offset 0 of 1 bytes in field_conditional_state_settings_form() (Zeile 174 von sites\all\modules\contrib\field_conditional_state\field_conditional_state.admin.inc).
Notice: unserialize(): Error at offset 0 of 4 bytes in field_conditional_state_settings_form() (Zeile 174 von sites\all\modules\contrib\field_conditional_state\field_conditional_state.admin.inc).
Notice: unserialize(): Error at offset 0 of 1 bytes in field_conditional_state_settings_form() (Zeile 174 von sites\all\modules\contrib\field_conditional_state\field_conditional_state.admin.inc).
Notice: unserialize(): Error at offset 0 of 1 bytes in field_conditional_state_settings_form() (Zeile 174 von sites\all\modules\contrib\field_conditional_state\field_conditional_state.admin.inc).
Notice: unserialize(): Error at offset 0 of 1 bytes in field_conditional_state_settings_form() (Zeile 174 von sites\all\modules\contrib\field_conditional_state\field_conditional_state.admin.inc).
Notice: unserialize(): Error at offset 0 of 1 bytes in field_conditional_state_settings_form() (Zeile 174 von sites\all\modules\contrib\field_conditional_state\field_conditional_state.admin.inc).
hass’s picture

hass CreditAttribution: hass commented

How should this work? Usability wise it's not clear to me if a selection in the multiselect means "value is one of (OR)" or "value is all of (AND)". Shouldn't we add this logic to the TRIGGER STATE just to make it clear and also to make it flexible for both variants?

Tobias Xy’s picture

Tobias Xy CreditAttribution: Tobias Xy commented

That's a bit complicated logical stuff... Have to think about the possibilites we have there and how we can build the settings form without having logical ambiguities. We also have to keep in mind that there are still some restrictions in what kind of logical constructs we can implement through the States API.

The error from #12 is triggered by already existing states. We need a new update hook for that.

Tobias Xy’s picture

Tobias Xy CreditAttribution: Tobias Xy commented

The way I see it, that's not as easy as it seems to be. I agree with you, that it would be much better to add this logic to the state itself, but in case we would add an additional option to choose the logical operator for each state, there are a some issues too:

  • (&& A) (&& B) (|| C) - What exactly does the logical operator set for A mean? It's useless. Okay, we could just disable the option for the first state, but still:
  • A && B || C - Does it mean (A && B) || C or A && (B || C)? Because && has a higher precedence than || actually the first case would be the right interpretation, but I don't think the average user knows that.

If we would just add the extra option just to the multiselect fields (I think that is what you had in mind?), than there would be impossible constructs, given the following setup (for example):
Group level operator: OR
Some simple states A and B
A multiselect state C using operator AND
In the end there would be something (A || B || (C1 && C2)) that isn't implementable by the States API without rewriting it to (A || B || C1) && (A || B || C2).

I still have an idea; will post it later.

Tobias Xy’s picture

Tobias Xy CreditAttribution: Tobias Xy commented

A large part of the settings form has to be rewritten then. Phew! I don't feel like doing this today. :D
But because I want to finally release 2.0, I'll start doing this the next days...

hass’s picture

hass CreditAttribution: hass commented

Than let's leave this out for now. Conversion should be possible later, too.

Tobias Xy’s picture

Tobias Xy CreditAttribution: Tobias Xy commented
Status: Needs work » Needs review
FileSize
trigger_value_multi_select-2124525-18.patch6.96 KB

Okay, a new patch including the update hook.

  • Commit 58506e5 on 7.x-2.x by Tobias Xy: 
    Issue #2124525: Allow multiple trigger values within one row
Tobias Xy’s picture

Tobias Xy CreditAttribution: Tobias Xy commented
Status: Needs review » Fixed

Done.

Status: Fixed » Closed (fixed)

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