#multiple matched $field['multiple'] in D6, which has become $field['cardinality']

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

yched’s picture

Issue tags: +Fields in Core

Side note: should be super easy for a Field API novice.

tstoeckler’s picture

Assigned: Unassigned » tstoeckler

Will have a go at this tomorrow!

tstoeckler’s picture

Assigned: tstoeckler » Unassigned

Well, tomorrow is gone by now. I'm still on this, theoretically, but my network driver has crapped on my linux system, so I'm back to Windows until that is fixed. Shouldn' take me more than a few days nonetheless. Still, if someone beats me to it, I wouldn't be mad.

dakala’s picture

Assigned: Unassigned » dakala

I'd like to give this a go, please:-)

dakala’s picture

Please be gentle, it's my first attempt at creating a patch for core. Only a few occurrences remain but hey, who cares? A patch is a patch:-)

bjaspan’s picture

Status: Active » Needs work

dakala, thanks for taking this on!

Please read http://drupal.org/patch for instructions about how to create core patches for Drupal. Proper patches make reviewing easier, allow the test bot to work, and make it possible to commit the patch when it is ready.

dakala’s picture

Thanks bjaspan. Here's what I've done - updated my working copy of files, checked my changes and ran cvs diff from the root directory. Here's the output.

yched’s picture

Patch looks great, thanks dakala ! I would have expected there would be more ocurrences to replace, but I'm not where I can check myself. Please make sure you did a search / replace on the whole of core.

dakala’s picture

I'll do a search on the whole of core and not only the field modules as the issue affects all forms indeed. Then, I'll re-roll the patch.

dakala’s picture

There's loads more occurrences of #multiple and they've all been replaced. The new patch is attached. Thanks.

yched’s picture

Er, my awful bad :-), most core occurrences of #multiple in core should remain #multiple.
AAMOF, only the field.attach.inc and field.form.inc hunks are valid changes - er, those are the ones in your patch #7...)

I'm not completely sure about the options.module changes. We do need to keep '#multiple' => at the left of => assignments, but the $element['#muliple'] we read is probably the property renamed to #cardinality in field.form.inc. Would need some manual testing...

bjaspan’s picture

@dakala, yeah, yched gave you bad advice. Go easy on him, he's on vacation. :-)

You need to distinguish between #multiple for Form API (which should stay) and #multiple for Field API (which should change to #cardinality). So your patch in #7 may be right. I'll re-attach it here and set CNR to see what hte testbot thinks.

bjaspan’s picture

Status: Needs work » Needs review

forgot to change status

dakala’s picture

@bjaspan, @yched: Thanks guys. Actually, there were a few obvious places I'd left out and I suspected there was something going on between FAPI and Field API that I didn't understand. Thanks for the clarification.

Dries’s picture

Status: Needs review » Fixed

Makes sense. Committed.

yched’s picture

Priority: Minor » Critical
Status: Fixed » Active

Cool, thanks dakala !

Not where I can test, as bjaspan explained in #12 :-p, but I'd say option.module needs some changes too. The #multiple boolean set on select dropdowns, or the fact we chose radios or checkboxes, should depend on #cardinality.

Thus now raising to critical, because I suspect option.module is currently broken

dakala’s picture

Ok. I'll look into getting that sorted right away.

dakala’s picture

@yched: Maybe this should wait till you're able to check?

The 2 functions (options_buttons_elements_process() and options_select_elements_process() in options.module) dealing with #multiple/cardinality seem okay to me. In order to set the #multiple boolean, the ternary expressions check for $element['#multiple'] before $field['cardinality'].

bjaspan’s picture

Assigned: dakala » Unassigned

@dakala: Your help on this issue has been great and very much appreciated... Thanks!

However, the remaining issue of fixing #multiple vs. #cardinality in exactly the right places is frankly not a Field API novice job. In fact I think yched is probably the only person that knows the right answer. Even if you submitted a patch, I wouldn't really know how to verify it is correct.

There are tests for option.widgets now that make it seem they are working, so this can probably wait until yched gets back from vacation, or at least clues me in here about how to understand #multiple vs. #cardinality in these forms. :-)

yched’s picture

Status: Active » Fixed

Getting back late here.
My bad, I was on crack in #16, everything is at it should. Thanks again, dakala.

Status: Fixed » Closed (fixed)
Issue tags: -Fields in Core

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