Skip to content

Sql #85

Closed
wants to merge 12 commits into from

10 participants

@Faryshta
Faryshta commented Feb 8, 2013

Proposal for coding standards on SQL and PHP.

This is an eraser and must be interpreted as a guide of what it might look like after the voting decides which standards to recommend.

Faryshta added some commits Feb 6, 2013
@Faryshta Faryshta Create PSR-sql.md
Created structure to fill and starting to create general and SQL syntax
9126e31
@Faryshta Faryshta Update proposed/PSR-sql.md
`:Table.field` was replaced with `:Table_field`
c10650c
@Faryshta Faryshta Keyword Section
added keyword section
f266544
@Faryshta Faryshta typos
fixing two typos
89bc129
@Faryshta Faryshta Survey and eraser end
Finished the main skeleton of the eraser and started the survey questions.
30f5474
@Faryshta Faryshta survey update
Adding more questions to the surve and a notice about dynamic queries
faa8524
@Faryshta Faryshta notice
Extends the notice to explain that naming conventions for tables, fields and alias are purposely avoided.
8c427f6
@Faryshta Faryshta backtick
Update on the backtick ussage
ec7082f
@Faryshta Faryshta typo
Extra backtick deleted
3250606
@Faryshta Faryshta Rewording
Changing the explanation of several items and survey questions
5fb4032
@Faryshta Faryshta Corrections
@oaass corrections
ec47903
@rvbhute
rvbhute commented Feb 9, 2013

Please wrap the document content at 80 characters, even for code blocks.

I believe UPPERCASE for all SQL keywords and lowercase for table and fields is preferred than having different case styles for categories of keywords. In addition, you have mixed the capitalization styles for specific SQL keywords and table names. This will lead to confusion rather than swifter comprehension of a query.

Can you provide a source from where you are getting your definition of definition, block and operation keywords?

@Faryshta
Faryshta commented Feb 9, 2013

@rvbhute the keyword capitalization is one of the items to vote at the end of the file. But the table and field names lack of any sort of case style, its explained in the notice at the beginning.

@judgej
judgej commented Feb 9, 2013

I would also agree that UPPER for all keywords, rather than a mix of different styles for different classes of keyword is a clear convention to follow.

And on the classes of keyword, and ultimately the class of statement, we have three different types:

  • SQL statements retrieve data. They are not used to change anything, but just to retrieve data.
  • DML statements manipulate the data. They do not retrieve data, but change data - create/update/delete.
  • DDL statements manipulate the structure of the database. They may update data in the course of changing the structure, but its purpose is to define the objects in the database - tables, views, procedures, functions, etc.

I'm not sure whether this has an impact on the interface, but it is a clear distinction that should be considered when putting a spec together. In most PHP applications these different classes of statement are all mixed up and the big assumption is that the user running the application is able to do all these things. Outside of the MySQL world, this is generally seen as a very insecure way of working - a live query user able to update anything and change the structure of the database? Perish the thought.

(There is also other classes such as TCL to handle transactions and DCL to manage permissions and users. These are much more platform-specific than the others, but perhaps should still be considered individual classes of RDBMS statement.)

Edit: I guess my main point, is that what we have come to view as "SQL" with "keywords" is actually a collection of a handful of separate languages with their own syntaxes and forms and purposes. We just happen to be able to execute them all through the same tools since they are essentially text-based languages (albeit with bind variable support for SQL and DML), so they tend to be mixed together (CREATE ... AS SELECT ... goes a step further and really combines them). Again, I am not sure how much this impacts on the proposal, but I just have a hunch it is something that needs to be considered.

@Faryshta
Faryshta commented Feb 9, 2013

@judgej how do you propose making those difference into the style guide?

@judgej
judgej commented Feb 9, 2013

So far as syntax rules are concerned, I don't really see any reason to distinguish between them. They all share common features such as case-insensitive keywords and case-insensitive object names when not quoted in some way (and generally case-sensitive when they are quoted).

Quoting table names is always a funny one. Some database systems will convert unquoted table names to UPPER CASE and some to lower case. If you do start quoting table names, then you generally have to get the letter case exactly right. For that reason, I personally tend to avoid quoting table names, and then need to watch out for reserved words.

Thinking further ahead, if we have interfaces for passing statements around, then perhaps there should be a distinction. For example, there would be no need to handle bind variables in DDL statements.

@vendethiel vendethiel and 2 others commented on an outdated diff Feb 11, 2013
proposed/PSR-sql.md
+### 2.2 Keywords
+
+The keywords are divided in three types
+
+- Definition keyword: Define the type or query `INSERT`, `SELECT`, `CREATE`, `UPDATE`
+- Block keyworkds: Are used at most once and separate the query in sections `where`, `from`, `limit`, `order by`, `join`
+- Operation keywords: Are used inside operations `and`, `or`, `not`, `null`
+
+Dividing the keywords in types help to organize and understand the query, not all keywords work the same and shouldn't be treated the same.
+
+### 2.3 Basic Syntax
+
+- Definition keywords MUST be written in UPPERCASE
+- Block keywords MUST be written Capitalized
+- Operational keywords MUST be written in lowercase
+- `*` asterisk MUST NOT be used. Instead its necessary to make a list of all the required fields. This helps redability and avoid redundancy
@vendethiel
vendethiel added a note Feb 11, 2013

Is that only for SELECTs or valid for COUNT()s too ? IIRC, COUNT(*) can be faster (not faster than COUNT(pk), but it does not check for null).

@judgej
judgej added a note Feb 11, 2013

So long as there is at least one NOT NULL column on the table, then COUNT(*) should be as fast as COUNT(pk) - any RDBMS optimiser worth its salt will recognise that.

Saying SELECT * "MUST NOT" be used may go unnecessarily too far. Although it is seldom needed, it can be useful to analyse or export table data.

@Faryshta
Faryshta added a note Feb 11, 2013

Good point, for now I will just put

SELECT * MUST NOT....

Until we reach an agreement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@oaass oaass commented on the diff Feb 11, 2013
proposed/PSR-sql.md
+SQL;
+}
+```
+
+- .sql files MUST NOT be used inside the DOCUMENT ROOT
+- Query declaration MUST use single quotes
+- Query declaration SHOULD use `nowdoc` for PHP 5.3 using SQL to add syntax higlight
+- When the query is executed inside a class method the query variables SHOULD be stored in private propertys. This will allow to keep the queries separated from the methods definition
+- When used inside a function, queries SHOULD be declared at the beggining of the function definition. This will allow to keep the queries separated from the function definition
+
+4. Best Practices
+-----------------
+
+### 4.1 Fetch
+
+The `PDOStatement::fetchAll` method SHOULD NOT be used.
@oaass
oaass added a note Feb 11, 2013

This can be misleading in that it makes PDOStatement::fetchAll() look like a bad method to use. So maybe you should append "when a large result set can be expected" or something simular?

@Faryshta
Faryshta added a note Feb 12, 2013

Ok will think on a way to do it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Faryshta Faryshta Asterisk
Changing line 65
58e467e
@FlorianWolters

I do think this is out of scope and should not be a part of FIG. This should be added to http://phptherightway.com (if added at all):

IMO, SQL should not be mixed with the used programming language (one also does not use presentation logic in the models). If one wants to create custom statements, a SQL query builder component should be used. Stored procedures (if available in the used DBMS) and an ORM should be prefered, though.

In addition, SQL is standardized by the ISO:

  • 1989 - SQL-89, Standard SQL, SQL-1
  • 1992 - SQL-92, SQL-2
  • 1999 - SQL-99, SQL-3
  • 2003 - SQL-2003

Im guess these also include recommendations for case sensitivity etc (though I am not sure).

Plus I don't like that plain PDO is referenced...

FIG is for PHP, not for SQL.

@olekhy
@Faryshta

AFAIK SQL:2003 and its succesors including SQL 2008 are for the way SQL works, this is a style guide which means a totally different purpose.

SQL is vital in developing and implementing PHP, also one of the most overlooked features where developers write most of malpractices and vulnerabilities so I think this is a very important topic

@AmyStephen

I have many years in as DBA, data warehouse analyst, oversight for large University databases and applications. I agree that it is an important area.

Having said that, most developers are likely using an ORM or abstracted DBMS of sorts, and an increasingly smaller subset of developers actually write most of the database software. These people tend to have higher skill in this area, and are largely taking care of generated SQL for most "modern" applications.

The interface projects do a good job of helping standardize subsystems, basic processes and data requirements. Over time, this should help consolidate and then strengthen the software available.

I don't see that potential in this initiative. I would prefer to keep the focus on those application interfaces, at least until it's well underway.

@Faryshta

@AmyStephen what do you need it takes to get that potential?

@AmyStephen

I don't believe it can, given the purpose of this group. Someone mentioned that this might be more suitable for php the Right Way. I actually can see it useful as "SQL the Right Way" if it doesn't focus on the php Language, but SQL's use in languages.

If you want to continue pursuing it as a possibility for this group, then, IMO, you need to start with figuring out where the membership projects are at. Turn your recommendations into a checklist and inventory the member projects. Get a handle on impact, what would it take for compliance?

Understand, given your recommendations, in order to comply:

  • many projects would have to get rid of entire database subsystems, given the PDO requirement;
  • other projects do not have any capability at this time of storing source beneath the document root;
  • in most cases, developer don't write SQL, but use models, XML, YAML, some kind of ORM-like approach that precedes a core SQL generation class.

Lastly, don't assume that getting rules defined as PSR's is the best or the only way to raise awareness and strengthen skills in the industry. If you are passionate about this, if you believe it is important, if you think there is a need, continue working. But, definitely figure out where the member code bases are at right now, related to this area, do the homework and research on that. You might find that awareness building is needed before the community is even ready for this. You might find it's not relevant, given the nominal SQL and trend towards ORM.

Recommendations of this nature, call for projects to change code. On the other hand, Interfaces describe how projects can participate, increase their install base, find new users. When asking that projects invest in code changes, one must clearly describe the cost and benefit of impact, in terms of work required to comply, and value. And, there must be tools to measure and assist with compliance.

Please don't be discouraged by my comments. I do appreciate your work.

@philsturgeon

This is completely outside of the scope of the PHP-FIG. We write PHP, not SQL. We don't tell you how to write your XML, JavaScript, CSS, htaccess files or anything else either.

Please send a pull request to phptherightway.com and I'll happily look into merging it.

Please can this one get shut down @dragoonis?

@dragoonis

Hi @Faryshta,

This proposal has some merit on making sure inline SQL statements are cleanly formatted but as of right now it's not something the FIG project representatives or FIG community members are raising as a priority concern.

Closing off.

Cheers,
Paul

@dragoonis dragoonis closed this Mar 1, 2013
@Faryshta

@philsturgeon cool i will try to send it.

i will have to figure out first how to send it there since i don't know the phptherightway guides to create merge requests.

@philsturgeon

It's just a bunch of markdown files dude, fork and edit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.