Add in Coding Standards Docs for PHP and SQL #222

seamuslee001 · 2017-07-17T22:01:30Z

Ping @seanmadsen @xurizaemon Chris are you able to check the SQL one to see if it makes sense i know that you did a talk on writing proper SQL code at the code sprint in Fort Collins last year and just want to make sure its consistent, Sean can you check if i have got the style / phrasing right i think so but just would appreciate a good review.

xurizaemon · 2017-07-17T22:03:24Z

I'll take a look! But are you sure that was me talked at Ft Collins? I am ... less certain.

xurizaemon · 2017-07-17T22:24:02Z

I gave up on my review and PR'd my feedback as seamuslee001#1

seancolsen · 2017-07-17T22:25:51Z

+1 for the practice of submitting feedback as a PR! 🙂

I'll look at this too... soon, but not right now.

xurizaemon · 2017-07-17T22:07:03Z

docs/standards/sql.md

+$genderId = 2;
+$result = CRM_Core_DAO::executeQuery("SELECT FROM civicrm_contact WHERE display_name like %1 AND gender = %2", array(
+  1 => array('%' . $name . '%', 'String'),
+  2 => array($genderId, 'Integer'),


For non-SQL reasons, I'm not a fan of CiviCRM's storage of gender data. Maybe is_opt_out would be equally effective at demonstrating parameterization?

xurizaemon · 2017-07-17T22:07:18Z

docs/standards/sql.md

+
+When writing SQL developers should be concious to try to always ensure that all variables that are passed into the SQL are done in a safe and secure maner.
+
+In CiviCRM the best way is to use the inbuild paramatisation tools to do the job. 


"inbuilt", "parameterization"

xurizaemon · 2017-07-17T22:07:27Z

docs/standards/sql.md

@@ -0,0 +1,24 @@
+# SQL Coding Standars
+
+When writing SQL developers should be concious to try to always ensure that all variables that are passed into the SQL are done in a safe and secure maner.


"conscious", "manner"

xurizaemon · 2017-07-17T22:29:05Z

ignore that review - yeah the PR was way better flow!

totten · 2017-07-17T23:07:02Z

docs/standards/sql.md

+  1 => array('%' . $name . '%', 'String'),
+  2 => array($optedOut, 'Integer'),
+));
+```


This is certainly the material we need to present for common queries.

totten · 2017-07-17T23:07:19Z

docs/standards/sql.md

+
+The variable types available for this can be found in [CRM_Utils_Type::validate](https://github.com/civicrm/civicrm-core/blob/master/CRM/Utils/Type.php#L378). The query engine then applies appropriate escaping for the type.
+
+In some circumstances you may find that a complex query is easier to build by directly escaping values using the `CRM_Utils_Type::escape()` method. It's preferable to use the form above.


For complex query building, I think that we're inviting trouble if we suggest that people use DAO. I'd rather point towards:

https://github.com/civicrm/civicrm-core/blob/master/CRM/Utils/SQL/Select.php#L33

Since the topic of complex query-building came up, I'm ready to cave on the purity of CRM_Utils_SQL_Select... civicrm/civicrm-core#10686

@totten i have attempted to put some docs in let me know what you think.

… CRM_Utils_SQL_Select Fix example

Migration was done by Seamus. This commit simply adds the redirect

seancolsen · 2017-07-19T18:17:56Z

@seamuslee001 thanks for this work. This stuff is really great! I made some small improvements:

Formatting
- It's worth pointing out that I changed the html table to separate sections with headings because this means that we can use markdown for links and inline code. I'll admit that I think the table you created was slightly easier to read. My change makes a small sacrifice in readability, but comes with some gains in readability (for inline code) and comes with gains in terms of maintainability (since the doc source is cleaner). Also, when we use markdown for links, it becomes easier for us to ensure that the guides don't have any broken internal links. Let me know if you have opinions about this change. I'm open to other approaches.
Added the pages you created to the nav menu mkdocs.yml.
Added a general "Code standards" page
Added a redirect from the wiki's "PHP standards" page so we can officially mark that wiki page as having been migrated.

seancolsen · 2017-07-19T18:24:38Z

Given @totten's concerns about suggesting that people use the DAO, we are now waiting on @totten to either:

give the go-ahead for merging this PR as-is (with additions from @seamuslee001 about CRM_Utils_SQL_Select)
OR suggest an alternative

I will merge this PR in 5 days if there are no further comments. I think that right now this content is "better in than out". Reading this page was actually the first time I had ever heard about CRM_Utils_SQL_Select!

totten · 2017-07-20T00:51:34Z

I agree. This is a good improvement to the docs, and it's better in than out.

seamuslee001 · 2017-07-20T01:05:36Z

@seanmadsen yep your changes are good mate, I just flowed on from the previous, I think it would be good if we could get a redirect @totten @mlutfy https://wiki.civicrm.org/confluence/display/CRMDOC/PHP+Code+and+Inline+Documentation to https://docs.civicrm.org/dev/en/latest/standards/php/ now

seancolsen · 2017-07-20T01:07:43Z

@seamuslee001 the redirect happens automatically when we add it to the book's redirect file as I did in this commit, but it's not instantaneous. Apparently there's roughly a 10 minute delay. So, no further action should be needed here.

seamuslee001 · 2017-07-20T01:08:36Z

@seanmadsen ahhh cool :-)

seamuslee001 · 2017-07-20T20:53:33Z

@JoeMurray Joe i think an example would be probably which is something i have seen in one of our custom modules

A developer realises they are going to be using the same base block of sql in multiple places so rather than writing it in each they generic block is put into a helper function where it builds joins + some of the where perhaps but then is fed out to the out functions where its put together with some more sql probably more where clauses and then run through the CRM_Core_DAO::executeQuery, So in this case in that helper function it would make sense to use the CRM_Utils_Escape:; if your only wanting to send back an SQL string.

I can also point to this example in the current codebase as a situation https://github.com/civicrm/civicrm-core/blob/master/CRM/Mailing/Event/BAO/TrackableURLOpen.php#L58 i think Chris or Tim talked with Mathieu when coming up with the appropriate escaping strategy in Colarado last year.

seamuslee001 added 2 commits July 18, 2017 07:45

PHP coding standards

3896d3c

Add in SQL coding standards doc

de65aa7

Rewrite SQL docs on parameterization.

3b28576

xurizaemon reviewed Jul 17, 2017

View reviewed changes

totten reviewed Jul 17, 2017

View reviewed changes

Fix spelling mistake

363b1f5

seamuslee001 force-pushed the coding_standards branch 2 times, most recently from 6188b5d to f46b8f9 Compare July 18, 2017 21:20

Update URL as per chris and attempt to add in some docs in regards to…

acbfda9

… CRM_Utils_SQL_Select Fix example

seamuslee001 force-pushed the coding_standards branch from f46b8f9 to acbfda9 Compare July 18, 2017 21:21

seancolsen added 4 commits July 19, 2017 10:43

Add new pages to the navigation menu

a7279bd

Formatting improvements

58c6bad

Add general "Code standards" page

8ade808

Migrate wiki "PHP Code and Inline Documentation"

bb435f2

Migration was done by Seamus. This commit simply adds the redirect

seancolsen merged commit 4c0b926 into civicrm:master Jul 20, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add in Coding Standards Docs for PHP and SQL #222

Add in Coding Standards Docs for PHP and SQL #222

seamuslee001 commented Jul 17, 2017

xurizaemon commented Jul 17, 2017

xurizaemon commented Jul 17, 2017

seancolsen commented Jul 17, 2017

xurizaemon Jul 17, 2017

xurizaemon Jul 17, 2017

xurizaemon Jul 17, 2017

xurizaemon commented Jul 17, 2017

totten Jul 17, 2017

totten Jul 17, 2017 •

edited

Loading

totten Jul 17, 2017

seamuslee001 Jul 18, 2017

seancolsen commented Jul 19, 2017

seancolsen commented Jul 19, 2017

totten commented Jul 20, 2017

seamuslee001 commented Jul 20, 2017

seancolsen commented Jul 20, 2017 •

edited

Loading

seamuslee001 commented Jul 20, 2017

seamuslee001 commented Jul 20, 2017


		When writing SQL developers should be concious to try to always ensure that all variables that are passed into the SQL are done in a safe and secure maner.

		In CiviCRM the best way is to use the inbuild paramatisation tools to do the job.

		@@ -0,0 +1,24 @@
		# SQL Coding Standars

		When writing SQL developers should be concious to try to always ensure that all variables that are passed into the SQL are done in a safe and secure maner.


		The variable types available for this can be found in [CRM_Utils_Type::validate](https://github.com/civicrm/civicrm-core/blob/master/CRM/Utils/Type.php#L378). The query engine then applies appropriate escaping for the type.

		In some circumstances you may find that a complex query is easier to build by directly escaping values using the `CRM_Utils_Type::escape()` method. It's preferable to use the form above.

Add in Coding Standards Docs for PHP and SQL #222

Add in Coding Standards Docs for PHP and SQL #222

Conversation

seamuslee001 commented Jul 17, 2017

xurizaemon commented Jul 17, 2017

xurizaemon commented Jul 17, 2017

seancolsen commented Jul 17, 2017

xurizaemon Jul 17, 2017

Choose a reason for hiding this comment

xurizaemon Jul 17, 2017

Choose a reason for hiding this comment

xurizaemon Jul 17, 2017

Choose a reason for hiding this comment

xurizaemon commented Jul 17, 2017

totten Jul 17, 2017

Choose a reason for hiding this comment

totten Jul 17, 2017 • edited Loading

Choose a reason for hiding this comment

totten Jul 17, 2017

Choose a reason for hiding this comment

seamuslee001 Jul 18, 2017

Choose a reason for hiding this comment

seancolsen commented Jul 19, 2017

seancolsen commented Jul 19, 2017

totten commented Jul 20, 2017

seamuslee001 commented Jul 20, 2017

seancolsen commented Jul 20, 2017 • edited Loading

seamuslee001 commented Jul 20, 2017

seamuslee001 commented Jul 20, 2017

totten Jul 17, 2017 •

edited

Loading

seancolsen commented Jul 20, 2017 •

edited

Loading