CRM-18811: Permit spaces in table and column aliases.

[xurizaemon]

Spaces are permitted provided the aliases are surrounded by backticks.

This is not intended to be a complete representation of what MySQL permits, it's just expanding to permit things that CiviCRM actually generates.

---

• CRM-18811: MySQL table and column aliases can contain spaces

[josephlacey]

@xurizaemon How can I test this? Through an extension that adds a table with a space? Regex looks fine, and the unit test could be better than my manual testing. Not sure. Let me know.

[xurizaemon]

Steps to reproduce I believe are -

• Create a location with a space in the name
• Include information from that location in a profile
• View the profile

[josephlacey]

@xurizaemon This in fact fixes the problem at hand, but a few notes

I had to remove the form rule for adding locations which was restricting the above reproducibility. Not sure that's completely germane here though.

diff --git a/CRM/Admin/Form/LocationType.php b/CRM/Admin/Form/LocationType.php
index 68d2980..8ea76be 100644
--- a/CRM/Admin/Form/LocationType.php
+++ b/CRM/Admin/Form/LocationType.php
@@ -59,10 +59,6 @@ class CRM_Admin_Form_LocationType extends CRM_Admin_Form {
       'objectExists',
       array('CRM_Core_DAO_LocationType', $this->_id)
     );
-    $this->addRule('name',
-      ts('Name can only consist of alpha-numeric characters'),
-      'variable'
-    );

     $this->add('text', 'display_name', ts('Display Name'), CRM_Core_DAO::getAttribute('CRM_Core_DAO_LocationType', 'display_name'), TRUE);
     $this->add('text', 'vcard_name', ts('vCard Name'), CRM_Core_DAO::getAttribute('CRM_Core_DAO_LocationType', 'vcard_name'));

I setup the location fields as searchable columns on the profile and found a few bugs when searching, again not sure they would be blockers on this. Note that in all of the below I've replaced proper backticks with a single quote for formatting reasons; the backticks are properly set in the queries.

Searching by a state_province_id resulted in this WHERE clause.

WHERE ( 'location with a space-address'.state_province_id = '1009' )

instead of the the properly formed that includes the under scores

WHERE ( 'location_with_a_space-address'.state_province_id = '1009' )

Searching by the city name resulted in a missing location_type_id in the JOIN

LEFT JOIN civicrm_address 'location_with_a_space-address' ON ( contact_a.id = 'location_with_a_space-address'.contact_id ) and 'location_with_a_space-address'.location_type_id = WHERE

instead of

LEFT JOIN civicrm_address 'location_with_a_space-address' ON ( contact_a.id = 'location_with_a_space-address'.contact_id ) and 'location_with_a_space-address'.location_type_id = 6 WHERE

[xurizaemon]

Ugh sorry. My instructions in https://issues.civicrm.org/jira/browse/CRM-18811 were better!

• There's a "Location name" and a "Display name"?
• The alias comes from the Location's Display Name, not the Location Name
• So the patch shouldn't be required, and this might need more testing
• I think the report from the user related to city, yes (not that this should matter, but who knows)

I got the feel that there's some confusion internally about the location name vs display name which probably led to this. I didn't dig further at the time.

[josephlacey]

I see. With the form rule in place, the listings do work, but those errors on searching within the profile listings still happen. Probably need to sort those bugs out before merging this in.

[eileenmcnaughton]

Reading the discussion I feel like this should be tagged [WIP] ?

[AkA84]

Seems this PR is not yet ready to be reviewed given the discussion above

[eileenmcnaughton]

OK - I've marked this WIP & returned to development on the ticket & removed the fix version.

Note that I think that PRs that are stalled with no prospect of outstanding issues being resolved in the foreseeable future should be closed & the JIRA used to track the work done. Not sure if this has reached that point yet

[eileenmcnaughton]

@xurizaemon I just read through this (I was looking for another PR of yours) and I feel that I'm happy to merge it despite the comments above. While the testing showed the format you described appears invalid in other scenarios I would counter that as a blocker on the following basis:

• the PR addresses a recent rule applied for the purpose of enforcing security
• you are describing a situation where the rule is blocking something that could be valid sql and be safe. - the fact the whitespace must fall within backticks seems to preclude any 'naughtiness' (and I do defer to your security expertise here too).
• UNIT TESTS
• comment improvements

I'm going to push back on the scope creep here & go for merge on pass. @josephlacey did agree this fixes a very narrow definition of the problem and I think we can accept that.

[eileenmcnaughton]

test this please

[xurizaemon]

Wow, I'd totally forgotten about this one ... go for it!

[eileenmcnaughton]

Just confirmed with @xurizaemon he does still think it's worth merging

i reviewed this & thought it could merge but I just re-read your comment & you seemed to have backed off it #8548
5:47 PM
I think we should merge or close - I don't think it 'hurts' but may not help?

[josephlacey]

I'm going to push back on the scope creep here & go for merge on pass. @josephlacey did agree this fixes a very narrow definition of the problem and I think we can accept that.

Late to this party, but yeah, merging this seems fine.