search

systopia / de.systopia.twingle

CiviCRM Twingle Integration
Other
4 stars 2 forks source link

fix #86 #87

Closed MarcMichalsky closed 4 months ago

MarcMichalsky commented 6 months ago

Fix issue #86

jensschuppe commented 6 months ago

See https://github.com/systopia/de.systopia.twingle/issues/86#issuecomment-2082760941

MarcMichalsky commented 6 months ago

Well, the only case I can think of where empty() might cause problems is a string containing '0'. Since the profile returns empty options as empty strings, it should be fine to just check for those.

jensschuppe commented 6 months ago

the profile returns empty options as empty strings,

Actually, that's only true for attributes that have been left empty in the form. The profile constructor fills empty attributes with NULL, i.e. those that haven't got a value at all (because the form has not been saved since they were added). I think the empty string is not a valid value for all attributes in the first place, so the getAttribute() method should not assume that.

Could you list the comparisons changed in 8bcdff4a855f621fec11ba255112c1d4ff57046e that actually cause problems? I'd be in favor of fixing those instead, as there's more context for each attribute.

MarcMichalsky commented 6 months ago

I suspect that there could be problems if a value received from a getAttribute() call is subsequently tested with isset() or is_string() because they would incorrectly assume an empty string as an option that needs to be processed.

Possible troublemakers:

I hope I have found them all! The line with the membership_type_id was where I first noticed the problem.

jensschuppe commented 5 months ago

I adressed your findings in 2b8ab813dbe9a3297edfd875539348408f8bb150, could you have another look, @MarcMichalsky?