On the relation type settings form user can enter values where max arity is less than min arity. Relation type gets successfully saved, that does not make much sense.

CommentFileSizeAuthor
#17 type_form_validation_7.x-1945294-17.patch772 bytesmikran
#12 type_form_validation_8.x-1945294-12.patch4.47 KBmikran
#6 1945294-type_form_validation-6.patch4.19 KBnaught101
#3 1945294-type_form_validation-3.patch2.22 KBnaught101
#1 type_form_validate-1945294-1.patch663 bytesmikran

Comments

mikran’s picture

mikran commented
Component: User interface » API
Status: Active » Needs review
StatusFileSize
new type_form_validate-1945294-1.patch663 bytes

relation_ui_type_form_validate() function added.

naught101’s picture

naught101 commented

This will also needs some test like the ones I'm proposing in #1345648: relation_save does not properly validate $type parameter, but maybe they can be added over there, since that's an API issue.

I wonder if it might not be sensible to also add a check to make sure that max_arity<=2 when directional is selected? See my latest comment on #1259738: n->m relations (mutant squids)

naught101’s picture

naught101 commented
StatusFileSize
new 1945294-type_form_validation-3.patch2.22 KB

Use full words, and correct grammar.

Also, since we're on type validation, can we make the source bundles required, and the reverse label and target label required for directional? Included in patch.

dpi’s picture

dpi
Location Perth, Australia
commented
Issue tags: +needs-forward-port-to-8.x

Tagging for forward port

steveoliver’s picture

steveoliver commented
Status: Needs review » Needs work

This looks good. Only thing I see:

+++ b/relation_ui.module
@@ -343,6 +348,10 @@ function relation_ui_type_form($form, &$form_state, $relation_type = array(), $o
+      'required' => array(   // action to take.
+        ':input[name="directional"]' => array('checked' => TRUE),
+        ':input[name="advanced[max_arity]"]' => array('!value' => '1'),
+      ),

I don't think we need to document #states here with these two comments:
// action to take

naught101’s picture

naught101 commented
StatusFileSize
new 1945294-type_form_validation-6.patch4.19 KB

In that case, we probably should do this :)

naught101’s picture

naught101 commented
Status: Needs work » Needs review

I'd apply this, but I'm wondering if there was a reason these form items weren't marked requred to start with (especially source_bundles - how would that work?)

steveoliver’s picture

steveoliver commented
Status: Needs review » Reviewed & tested by the community

naught101: there we go.

re: required: Maybe leave that for another issue and get this patch in, since we're certain it makes sense?

naught101’s picture

naught101 commented
Status: Reviewed & tested by the community » Fixed

Thanks, commited: 8f3251c

dpi’s picture

dpi
Location Perth, Australia
commented
Version: 7.x-1.x-dev » 8.x-1.x-dev
Status: Fixed » Patch (to be ported)
mikran’s picture

mikran commented

One more spelling error for 7.x

Minmum

mikran’s picture

mikran commented
StatusFileSize
new type_form_validation_8.x-1945294-12.patch4.47 KB

patch for 8.x

mikran’s picture

mikran commented
Status: Patch (to be ported) » Needs review

testing how badly tests are broken

loziju’s picture

loziju commented
Status: Needs review » Active

The validation doesn't seem to work for unlimited arity. I suppose unlimited = 0 and it can only be selected at the maximum arity field. Thus min = 1, max = 0 and it fails validation?

mikran’s picture

mikran commented
Status: Active » Needs work
mikran’s picture

mikran commented
Version: 8.x-1.x-dev » 7.x-1.x-dev

This has been fixed and committed for 8.x. The infinite arity issue remains for 7.x, here is the code from 8.x commit:

+  function validate(array $form, array &$form_state) {
+    $max_arity = $form_state['values']['advanced']['max_arity'];
+    $min_arity = $form_state['values']['advanced']['min_arity'];
+    // Empty max arity indicates infinite arity
+    if ($max_arity && $min_arity > $max_arity) {
+      form_set_error('min_arity', t('Minimum arity must be less than or equal to maximum arity.'));
+    }
+  }
mikran’s picture

mikran commented
Issue summary: View changes
Status: Needs work » Needs review
StatusFileSize
new type_form_validation_7.x-1945294-17.patch772 bytes
mikran’s picture

mikran commented
Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

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

The last submitted patch, 12: type_form_validation_8.x-1945294-12.patch, failed testing.

The last submitted patch, 12: type_form_validation_8.x-1945294-12.patch, failed testing.