Fix lab issue 2254 is_bulkmail cannot be set through UI

[artfulrobot]

Overview

Fix lab 2254

Technical Details

Setting the BAO is_bulkmail to '' actually sets it to 'null' and therefore the test for "is bulkmail set?" written as if ($email->is_bulkmail...) returns TRUE since 'null' is not empty.

I don't really understand the wisdom of using the string 'null' when we have NULL, but that's what we have. I think this PR could be optimised but without understanding the whys and wherefores of when data is and isn't cast to something weird, I played it safe.

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[artfulrobot]

(sorry, for the rewording of everything, I always get confused between hub and lab)

[eileenmcnaughton]

This feels like another field that be NOT NULL at mysql level & have a db default of 0

@seamuslee001 @colemanw thoughts?

[eileenmcnaughton]

In terms of whys & wherefores - there are 2 reasons I can think of to handle null on a boolean field (this used to be a boolean although now it has a 3rd option)

1. to save db storage - I think this might have been the reason early on
2. to reduce the chance people otherwise mishandle null - ie think custom data where the field could be null because there is no row in the custom data table for the contact & we want to treat no row the same as zero

I don't find either very compelling

[seamuslee001]

I notice that at least for new sites there is already a 0 default https://github.com/civicrm/civicrm-core/blob/master/xml/schema/Core/Email.xml#L128 so maybe an upgrade script is missing here or something but I would generally agree with that @artfulrobot does that make sense to you? would be curious if your current db has something other than a default 0 on the is_bulkmail column

[artfulrobot]

So there seems to be a couple of branches to this conversation.

• should the column be NOT NULL or allow NULL? Can't think why you'd want it NULL; the data is there to identify something positive only (i.e. an email suitable for bulk use).
• what is the default? and is it different depending on the age of your Civi?!
• why we have to put up with a crappy data access layer that calls NULL 'null', breaking all reasonable comparisons and conventions, and doesn't properly cast typed properties!

@seamuslee001 my database has 10 different values for is_bulkmail. Y'know, in binary ;-) No nulls.

The problem seems to be that copyValues() creates these fake nulls.

It's quite a serious regression (I'm sure it used to work) especially for people working with EU citizens' data subject to the GDPR.

I think on this line, we're trying to ascertain if it is supposed to be for bulkmail. So how about:

if ($email->is_bulkmail == 1 && !empty($params['contact_id']) && !self::isMultipleBulkMail()) {

That allows for 1 and "1" and TRUE but not 'null', 0 or "0" or FALSE.

[eileenmcnaughton]

I think the db should be NOT NULL default 1

[artfulrobot]

@eileenmcnaughton I agree about NOT NULL; disagree about the default - I think it should be 0 (as now).

Most people will have one email for everything, which will therefore be their primary. As I understand it, this will get used for bulk mail. Some people may want to specify a different email for bulk, fine, and if they do, then with your default the next email that gets added (e.g. billing) gets set to bulk too and now we can't do what they wanted.

Also, the default setting (I believe) is that a contact can only have one bulk email, so setting the default to 1 would mean that a new email added gazumped the original intended bulk email.

[demeritcowboy]

At first I wasn't able to reproduce this, but I am able to reproduce it in the following situation:

1. Contact needs to have 2 emails.
2. Try to set the bulk setting on the non-primary one.

If the contact only has one email or you're updating the primary one, it seems to work ok. Not sure if this changes the discussion, just resolves my own confusion.

[artfulrobot]

@demeritcowboy yes, that's correct.

The problem is that the logic goes:

1. am I setting one for bulkmail? if yes...
2. unset bulk on all the others.

And at the moment the answer to the question (1) is always yes, regardless whether the bulkmail box was ticked or not. (I suppose you might get lucky occasionally if the bulk mail one is the last one it processes, it might work, but a stopped clock tells the right time twice a day :-))

[eileenmcnaughton]

@artfulrobot if 0 is the effective default now then that's fine - my assumptions were wrong it seems

[artfulrobot]

I think so https://github.com/civicrm/civicrm-core/blob/master/CRM/Core/DAO/Email.php#L291

[artfulrobot]

Can we merge this now?

✔ We've ascertained that the field should be NOT NULL, and indeed, that's what it is already!

From civicrm.mysql:

`is_bulkmail` tinyint(4) NOT NULL DEFAULT 0

✔ We've agreed it should be default 0, and that's what it is in SQL and in the BAO.

✔ the tests pass

✔ the confusion was because copyValues is weird, but the new code correctly identifies the true is bulkmail state.

So is there a reson not to merge?

[demeritcowboy]

My thoughts: seems like one of those "better in than out" fixes.

r-run: Works (although it points out a separate issue)
r-code: Given that the actual problem is hard to fix, then personally I find it clearer with an explicit check against 'null', as opposed to a semi-hidden cast to int. But there is a comment explaining though.

Aside: the pre-existing comment "only 1 email address can have is_bulkmail = true" isn't really true. It's really more like "is_bulkmail can't be true for more than one email, except when it can".

Aside 2: I have a clock on my kitchen wall that's been broken for 10 years and in all that time I've never seen it have the right time once (grin). I'm convinced there is no such time as 8:14:28, at least not in my timezone.

[seamuslee001]

Merging based on @demeritcowboy 's feedback