[REF] Apply fix for CRM-607 for when building the select section of t…

[seamuslee001]

…he query as well as in where

Overview

This aims to apply the fix for CRM-607 to the select function as well as in the Where clause. Australian Greens found a problem where by a smart group was referencing 3 fields that were no longer existing in the database this caused problems because it would try and join but as NULL was being passed to joinCustomTableForField then it caused a syntax error

Before

Syntax error can be caused in a specific way

After

No syntax error

ping @andrew-cormick-dockery @johntwyman @eileenmcnaughton

[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).

[seamuslee001]

Jenkins re test this please

[andrew-cormick-dockery]

I have reservations about smart groups suddenly producing different results in certain circumstances (instead of producing DB syntax errors). I think the long term fix for this is to disable smart groups which make specific references to deleted custom groups and/or fields at the point in time that the field is deleted.

Having said that, I believe we're better off merging this PR right now than waiting for the above work to be completed.

[eileenmcnaughton]

This worries me a lot - if I create a smart group that says only people with custom field x can view Donald Trump's tax returns (which of course happens to be stored as an activity in the database) & then someone deletes custom group x then the result should be no-one can view his returns -not that the whole database can - which I believe would be the outcome of this - a hard error feels better than a security breach

[seamuslee001]

@eileenmcnaughton tho when we build the where clause https://github.com/civicrm/civicrm-core/pull/16627/files#diff-f7844105b59390da9d2993f5c8e93fd0R230 we are already achieving essentially the same result. This is just putting it for when we build the select clause which seems to make things consistent tbh

[eileenmcnaughton]

@seamuslee001 so it feels like a security bug already exists....

[seamuslee001]

@eileenmcnaughton i think its always been that way but this seemed to get caused by the upgrade were a bunch of _relative = 0 keys were added to the formValues

[eileenmcnaughton]

@seamuslee001 well I'm deeply uncomfortable with us allowing custom groups to work if any of their WHERE fields are disabled - this is select so I can see your case of it being 'not worse' but @colemanw I think API v4 smart searches should hard-fail if any fields no longer exist....