Closed aembler closed 9 years ago
yes, foreign keys please! I also like to have the ability to seed my database. Used to do that with some queries in the controller or my db.xml but I think it could have been implemented nicer.
So for foreign keys I think maybe something like the following:
<table name="PageTypePageTemplates">
<field name="ptID" type="I" size="10">
<key/>
<unsigned/>
<default value="0" />
</field>
<field name="pTemplateID" type="I" size="10">
<key/>
<unsigned/>
<default value="0" />
</field>
<index name="pTemplateID">
<col>pTemplateID</col>
</index>
<foreignKey name="SomeForeignKeyname" table="PageTypes" delete="RESTRICT" update="CASCADE">
<column local="ptID" foreign="ptID" />
<foreignKey>
</table>
The delete
and update
options should be optional and default to RESTRICT
. I'm still not sure about the name foreignKey
but I like this syntax overall. Improvements welcome :)
@EC-Joe I'd avoid the the name
attribute, or at least make it optional: it's useless in 99.99% of cases.
Furthermore what about naming this new node as references
? I'm thinking about something like this:
<table name="PageTypePageTemplates">
<field name="ptID" type="I" size="10">
<key/>
<unsigned/>
<default value="0" />
</field>
<field name="pTemplateID" type="I" size="10">
<key/>
<unsigned/>
<default value="0" />
</field>
<index name="pTemplateID">
<col>pTemplateID</col>
</index>
<references table="PageTypes" ondelete="RESTRICT" onupdate="CASCADE">
<column local="ptID" foreign="ptID" />
<foreignKey>
</table>
I agree with @mlocati I'd go with something like Laravel, call it <table>_<field>_foreign
by default and make name
optional.
If we're going to get rid of the name
attribute from foreign keys we should also do it for indexes to be as consistent as possible. My biggest concern with not requiring this is then we run the risk of developers getting unexpected errors if their name exceeds the 64 character length limit on mysql identifiers, at which point they then have to determine that they need to use the name attribute (which probably wouldn't be obvious). I just think it's easier to have it as a required field and then not have to worry about unexpected errors caused by a system generated name.
As for the "uselessness" argument. I would agree that usually it's not necessary, however if you ever did want to drop a foreign key it's a pretty crucial piece of information. It doesn't happen very often but occasionally I have worked on databases where we eliminated composite foreign keys and fields from tables in favor of using surrogate keys, so it does happen.
As far as the naming convention switching to references
makes sense to me.
About uselessness: That's why I suggest to use the naming convention of laravel where you can easily read the name of the foreign key!
About the length: I see that as a benefit, if we wanted to support other databases, we might not even have 64 characters and thus using a predefined name would cause problems.
Personally I think it would be easier to implement a restriction check on that name attribute far easier than it would be to come up with a reliable / predictable way to generate foreign key names of a specified length. But other than that I really don't have any qualms with the idea
Yes, it would be easier, but how would you do that if you wanted to be multi database capable - restrict the name to 35 characters for all databases? I guess all ways have at least one downside..
I guess honestly if we're requiring a developer to add the name attribute I almost feel like we're putting the burden on their shoulders, same as we do with naming their tables and columns (as they all have limits as well).
Yes, it would be easier, but how would you do that if you wanted to be multi database capable - restrict the name to 35 characters for all databases? I guess all ways have at least one downside..
Something I've actually thought about a little bit recently is "Why not create .xsd files to validate these db.xml files"? I think that it could make sense to have a .xsd for each database engine that concrete5 supports. And then they could be used for core testing, but additionally they could give package developers a way to validate their db.xml files. And on install we could then throw errors if the package couldn't be installed for the database engine they are using and it would even then be possible to add automated tests to the marketplace which indicate what databases packages support. (obviously we're still a looooong way away from getting other RDBMS support but a man can dream right?)
What about defaulting the name
attribute to the md5 of ($tableName.$foreignTableName.$localField1.$foreignField1. ... .$localFieldN.$foreignFieldN)
?
So, people that want a name may set it, and the others don't need to do it...
About the xsd: I'm creating it right now :wink:
@EC-Joe Yes, but we already have a lot of issues with reserved keywords and over long table names. Not sure how to get there very easily and I'm not even speaking of noSQL, but I don't mind SQL databases, they have quite a few advantages too..
@mlocati I wouldn't do that because it makes it hard to the database
I'll agree with the hard to read the database part, but it does sound like a surefire way to generate system strings and avoid length issues (lets hope for no collisions). Also doctrine ORM from what i'm reading also appears to use random foreign key names fwiw.
As far as the reserved keywords and over long table names, I think that we'll be able to put a stop to both of those by using .xsd validation very easily for the length limits, and probably the reserved words by putting a little gusto into some regex restrictions.
some light reading for those not familiar with xsd restrictions
About the xsd: I'm creating it right now :wink:
:clap: My Hero
Oracle has a limit of 30 characters for database objects and UID
is a reserved keyword. Those two stopped me a while ago on my work to make concrete5 usable on Oracle. Not sure how to come around those..
Here's the xsd (for MySQL):
<?xml version="1.0" encoding="utf-8"?>
<schema
xmlns="http://www.w3.org/2001/XMLSchema"
xmlns:tns="http://www.concrete5.org/axmls/0.5"
targetNamespace="http://www.concrete5.org/axmls/0.5"
elementFormDefault="qualified"
>
<element name="schema" type="tns:schemaType" />
<complexType name="schemaType">
<sequence>
<element name="table" type="tns:tableType" minOccurs="0" maxOccurs="unbounded" />
</sequence>
<attribute name="version" type="string" fixed="0.5" use="required" />
</complexType>
<complexType name="tableType">
<sequence>
<element name="field" type="tns:fieldType" minOccurs="1" maxOccurs="unbounded" />
<element name="index" type="tns:indexType" minOccurs="0" maxOccurs="unbounded" />
<element name="references" type="tns:referencesType" minOccurs="0" maxOccurs="unbounded" />
</sequence>
<attribute name="name" type="tns:databaseEntityNameType" use="required" />
</complexType>
<complexType name="fieldType">
<sequence>
<element name="key" type="tns:emptyElementType" minOccurs="0" maxOccurs="1" />
<element name="autoincrement" type="tns:emptyElementType" minOccurs="0" maxOccurs="1" />
<element name="unsigned" type="tns:emptyElementType" minOccurs="0" maxOccurs="1" />
<choice minOccurs="0" maxOccurs="1">
<element name="default" type="tns:fieldDefaultType" />
<element name="deftimestamp" type="tns:emptyElementType" />
</choice>
</sequence>
<attribute name="name" type="tns:databaseEntityNameType" use="required" />
<attribute name="type" type="tns:fieldTypeType" use="required" />
<attribute name="size" type="tns:fieldSizeType" use="optional" />
</complexType>
<simpleType name="databaseEntityNameType">
<restriction base="string">
<pattern value="[0-9a-zA-Z$_]{1,64}" />
</restriction>
</simpleType>
<complexType name="emptyElementType">
</complexType>
<complexType name="fieldDefaultType">
<attribute name="value" type="tns:fieldDefaultValueType" use="required" />
</complexType>
<simpleType name="fieldDefaultValueType">
<restriction base="string">
<minLength value="1" />
</restriction>
</simpleType>
<simpleType name="fieldTypeType">
<restriction base="string">
<!-- boolean -->
<enumeration value="L" />
<!-- boolean (if size == 1) or smallint (if size not set or != 1) -->
<enumeration value="I1" />
<!-- smallint -->
<enumeration value="I2" />
<!-- int -->
<enumeration value="I4" />
<!-- boolean (if size == 1) or smallint (if size between 2 and 4) or int (if size not set or greater than 4) -->
<enumeration value="I" />
<!-- bigint -->
<enumeration value="I8" />
<!-- string -->
<enumeration value="C" />
<!-- string -->
<enumeration value="C2" />
<!-- text -->
<enumeration value="X" />
<!-- text -->
<enumeration value="XL" />
<!-- text -->
<enumeration value="X2" />
<!-- float -->
<enumeration value="F" />
<!-- decimal -->
<enumeration value="N" />
<!-- datetime -->
<enumeration value="T" />
<!-- datetime -->
<enumeration value="TS" />
<!-- time -->
<enumeration value="TIME" />
<!-- date -->
<enumeration value="D" />
<!-- blob -->
<enumeration value="B" />
</restriction>
</simpleType>
<simpleType name="fieldSizeType">
<restriction base="string">
<pattern value="[1-9][0-9]*(\.[0-9]+)?" />
</restriction>
</simpleType>
<complexType name="indexType">
<sequence>
<element name="unique" type="tns:emptyElementType" minOccurs="0" maxOccurs="1" />
<element name="fulltext" type="tns:emptyElementType" minOccurs="0" maxOccurs="1" />
<element name="col" type="tns:databaseEntityNameType" minOccurs="1" maxOccurs="unbounded" />
</sequence>
</complexType>
<complexType name="referencesType">
<sequence>
<element name="column" type="tns:foreingColumnType" minOccurs="1" maxOccurs="unbounded" />
</sequence>
<attribute name="name" type="tns:databaseEntityNameType" use="optional" />
<attribute name="table" type="tns:databaseEntityNameType" use="required" />
<attribute name="onupdate" type="tns:foreingDepenencyActionType" use="optional" default="restrict" />
<attribute name="ondelete" type="tns:foreingDepenencyActionType" use="optional" default="restrict" />
</complexType>
<simpleType name="foreingDepenencyActionType">
<restriction base="string">
<enumeration value="restrict" />
<enumeration value="cascade" />
<enumeration value="set null" />
<enumeration value="no action" />
</restriction>
</simpleType>
<complexType name="foreingColumnType">
<attribute name="local" type="tns:databaseEntityNameType" use="required" />
<attribute name="foreign" type="tns:databaseEntityNameType" use="required" />
</complexType>
</schema>
Great! Afaik only the keywords are missing http://dev.mysql.com/doc/mysqld-version-reference/en/mysqld-version-reference-reservedwords-5-6.html
Good work @mlocati. Thinking that we should find a way to leverage something like this to resolve #1893
@Remo Yes, that would be useful. BTW I'm not sure about which could be the best way to do it...
For instance, to exclude the use of add
and accessible
we could do this:
<simpleType name="databaseEntityNameType">
<restriction base="string">
<minLength value="1" />
<maxLength value="64" />
<pattern value="(?!^(add|accessible)$).*" />
</restriction>
</simpleType>
But I think it's case-sensitive, so we should write add|adD|aDd|aDD|Add|AdD|ADd|ADD
. And this for all the reserved keywords... Any clue about how to solve this?
@EC-Joe Yes, an xsd could be useful to check packages installation. We may have also an online web page where developers can send their xml files to check for correctness... A five minute work that could save headaches for many people ...
@ both... Thanks :wink:
But I think it's case-sensitive, so we should write add|adD|aDd|aDD|Add|AdD|ADd|ADD. And this for all the reserved keywords... Any clue about how to solve this?
I don't actually think that xsd supports a case insensitive search :frowning:. Just to make things fun... maybe we could pass the db.xml through an xslt file which uppercases or lowercases name attributes, the resulting xml is then passed into the xsd file and validated. Just a thought... it's probably going to be kind of a pain to write up if memory serves...
I'm also not sure if it's to much shorter but would something like [aA][dD][dD]
work as well? It's a little shorter and feels a little less awkward and less prone to missing a combination... but still kind of a pain...
@EC-Joe Yes, I came across a solution like yours:
<simpleType name="databaseEntityNameType">
<union>
<simpleType>
<restriction base="string">
<pattern value="[0-9a-zA-Z$_]{1,64}" />
</restriction>
</simpleType>
<simpleType><restriction base="string"><pattern value="(?!^[Aa]Dd]Dd]$).*" /></restriction></simpleType>
<simpleType><restriction base="string"><pattern value="(?!^[Aa][Cc][Cc][Ee][Ss][Ss][Ii][Bb][Ll][Ee]$).*" /></restriction></simpleType>
</union>
</simpleType>
And like yours is quite awful :wink:
But maybe my solution doesn't work, I think that the <union>
puts in or
all the base simple types...
I think you can just have multiple patterns can't you? Something like:
<simpleType name="databaseEntityNameType">
<restriction base="string">
<pattern value="[0-9a-zA-Z$_]{1,64}" />
<pattern value="(?!^[Aa]Dd]Dd]$).*" />
<pattern value="(?!^[Aa][Cc][Cc][Ee][Ss][Ss][Ii][Bb][Ll][Ee]$).*" />
</restriction>
</simpleType>
I've seen people incrementing the version
attribute because they thought that it should be incremented when updating the DB structure.
So, what about using the namespace instead of the version
attribute?
I mean, something like this:
<?xml version="1.0" encoding="UTF-8"?>
<schema xmlns="http://www.concrete5.org/axmls/0.5">
...
</schema>
@mlocati Yeah that'd be fine.
@mlocati FYI looking at your example above it looks like it still uses AXMLS type fields (X2, etc...) I think we were thinking of ditching those for a strict mapping to doctrine types, lengths, and their attributes.
Yes, I have to fix the types list. And what about comments support? I always missed them...
Instead of pasting here long XML files I created a repository: see https://github.com/mlocati/concrete5-axmls
I also created an xslt (as for the suggestion of @EC-Joe :wink:)
You can try the xlst and the xsd with Firefox or Chrome at http://mlocati.github.io/concrete5-axmls/. The xlst fixes some potential mistake (wrong case of element names, wrong items order, ...)
I have a potentially radical idea here – what if instead of AXMLS (which is named after ADODB, which we're not using) we named this something like concrete5-doctrine-xml ? I was thinking we could make this be a third party library delivered via composer that anyone could use who wants to store their databases in a declarative XML format (with keys and all the other niceties we're bringing in this format) and who is using Doctrine DBAL. That will mostly be concrete5 – but it may not just be concrete5. In theory any framework that used DBAL could make use of this.
On Wed, Mar 4, 2015 at 7:04 AM, Michele Locati notifications@github.com wrote:
Instead of pasting here long XML files I created a repository: see https://github.com/mlocati/concrete5-axmls
I also created an xslt (as for the suggestion https://github.com/concrete5/concrete5-5.7.0/issues/2025#issuecomment-76748610 of @EC-Joe https://github.com/EC-Joe [image: :wink:])
You can try the xlst and the xsd with Firefox or Chrome at http://mlocati.github.io/concrete5-axmls/. The xlst fixes some potential mistake (wrong case of element names, wrong items order, ...)
— Reply to this email directly or view it on GitHub https://github.com/concrete5/concrete5-5.7.0/issues/2025#issuecomment-77173399 .
Hmmm.. It appears like we might run into some issues with the default values. in the validation. For instance one@1\nother@0, 2~16, 100, 1000, 10000, 100000, 1000000, …
is failing to validate. I think that the ellipses are causing the issue in this case. My guess is it is something to do with being a non-standard ascii character but that's complete speculation on my part.
have a potentially radical idea here – what if instead of AXMLS (which is named after ADODB, which we're not using) we named this something like concrete5-doctrine-xml ?
@aembler I agree. And do you want to keep concrete5
inside the name for 'marketing' purposes :wink:? What about simply doctrine-xml
? Or something like doctrine-xml-schema-definition
?
@EC-Joe I did a test with this XML
<?xml version="1.0" encoding="UTF-8"?>
<SCHEMA xmlns="http://www.concrete5.org/axmls/0.5">
<Table name="TableName">
<field name="FieldName" type="string" size="255">
<DEfaUlt value="one@1\nother@0, 2~16, 100, 1000, 10000, 100000, 1000000, …" />
</field>
</Table>
</SCHEMA>
That gets normalized by the XLST in
<?xml version="1.0" encoding="UTF-8"?>
<schema xmlns="http://www.concrete5.org/axmls/0.5">
<table name="TableName">
<field name="FieldName" type="string" size="255">
<default value="one@1\nother@0, 2~16, 100, 1000, 10000, 100000, 1000000, …"/>
</field>
</table>
</schema>
In the javascript version I receive this error:
transformed.xml:5: parser error : xmlParseEntityRef: no name
<default value="one@1\nother@0, 2~16, 100, 1000, 10000, 100000, 1000000, &
^
But the PHP version works like a charm.
So I think it's a problem of the javascript code...
@mlocati – What if we hosted it at concrete5/doctrine-xml? Then it could just be named doctrine xml, but the composer package would have concrete5 in the path (which would be enough credit for what we'd need.) I could create a new repository for it and give everyone involved in these discussions commit access.
On Wed, Mar 4, 2015 at 8:04 AM, Michele Locati notifications@github.com wrote:
have a potentially radical idea here – what if instead of AXMLS (which is named after ADODB, which we're not using) we named this something like concrete5-doctrine-xml ?
@aembler https://github.com/aembler I agree. And do you want to keep concrete5 inside the name for 'marketing' purposes [image: :wink:]? What about simply doctrine-xml? Or something like doctrine-xml-schema-definition?
@EC-Joe https://github.com/EC-Joe I did a test with this XML
<?xml version="1.0" encoding="UTF-8"?>
That gets normalized by the XLST in
<?xml version="1.0" encoding="UTF-8"?>
In the javascript version http://mlocati.github.io/concrete5-axmls/ I receive this error:
transformed.xml:5: parser error : xmlParseEntityRef: no name <default value="one@1\nother@0, 2~16, 100, 1000, 10000, 100000, 1000000, & ^
But the PHP version https://github.com/mlocati/concrete5-axmls/blob/master/axml.php works like a charm.
So I think it's a problem of the javascript code...
— Reply to this email directly or view it on GitHub https://github.com/concrete5/concrete5-5.7.0/issues/2025#issuecomment-77186008 .
@aembler :+1:
So, let's move to https://github.com/concrete5/doctrine-xml
Apologies if this has already been discussed, but how much of Doctrine's XML mapping already handles much of this? Could we use that instead?
Hi @mkly! We didn't discussed it before. But yesterday I took a look at it and I find their implementation quite too much complicated. I find concrete5 AXMLS more easy to read (and to write). Furthermore, current concrete5 developers would need to study it, and package developers would need more work to produce their packages (and moving from 5.6 to 5.7 would be an even harder step).
Just my 2 euro-cents :wink:
Apologies if this has already been discussed, but how much of Doctrine's XML mapping already handles much of this? Could we use that instead?
I also took a look at it as well. Like @mlocati I found it to be quite a bit more complicated. Especially with regard to setting up relationships. Having to define the type of relationship (one to many, one to one, etc) didn't seem to make sense to me to try and create for schema generation. I think that in concrete5's scenario if that sort of stuff needs defined it's defined in an ORM class, rather than forcing it to be required by developers who might only want to use the DBAL and db.xml files.
Just my 2 euro-cents :wink:
I always wondered why @mlocati's opinion was worth more... it's all clear now... :stuck_out_tongue_winking_eye:
@mlocati @EC-Joe Makes sense to me. Just wanted to bring it up to see if there was anything specific outside of being simpler and more concrete5 specific. I imagine there are some who will not be using Doctrine's ORM as well.
The online tool works quite well with IE 8+. With Chrome/Firefox it does not support validation of multibyte characters (that's a limit of the xsd validation library I found - http://syssgx.github.io/xml.js/ )
I've added support for table and field comments. I also added a full example to the README.md.
I will create a class to create a Doctrine\DBAL\Schema\Schema
instance given this new xml representation, like we currently do with the Axmls parser.
I think I'm done with doctrine-xml. Here's a sample XML file:
<?xml version="1.0" encoding="UTF-8"?>
<schema xmlns="http://www.concrete5.org/doctrine-xml/0.5"
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:schemaLocation="http://www.concrete5.org/doctrine-xml/0.5
http://concrete5.github.io/doctrine-xml/doctrine-xml-0.5.xsd"
>
<table name="Companies" engine="INNODB" comment="List of companies">
<field name="Id" type="integer" comment="Record identifier">
<unsigned />
<autoincrement />
<key />
</field>
<field name="Name" type="string" size="50" comment="Company name">
<notnull />
</field>
</table>
<table name="Employees" engine="INNODB">
<field name="Id" type="integer">
<unsigned />
<autoincrement />
<key />
</field>
<field name="IdentificationCode" type="string" size="20" />
<field name="Company" type="integer">
<unsigned />
<notnull />
</field>
<field name="FirstName" type="string" size="50">
<default value="" />
<notnull />
</field>
<field name="LastName" type="string" size="50">
<notnull />
</field>
<field name="Income" type="decimal" size="10.2">
<default value="1000" />
</field>
<field name="HiredOn" type="datetime">
<deftimestamp />
</field>
<index>
<fulltext />
<col>FirstName</col>
</index>
<index name="IX_EmployeesIdentificationCode">
<unique />
<col>IdentificationCode</col>
</index>
<references table="Companies" onupdate="cascade" ondelete="restrict">
<column local="Company" foreign="Id" />
</references>
</table>
</schema>
And here's the generated SQL:
CREATE TABLE Companies
(
Id INT UNSIGNED AUTO_INCREMENT NOT NULL COMMENT 'Record identifier',
Name VARCHAR(50) NOT NULL COMMENT 'Company name',
PRIMARY KEY(Id)
)
DEFAULT CHARACTER SET utf8 COLLATE utf8_unicode_ci
ENGINE = INNODB
COMMENT = 'List of companies'
;
CREATE TABLE Employees
(
Id INT UNSIGNED AUTO_INCREMENT NOT NULL,
IdentificationCode VARCHAR(20) DEFAULT NULL,
Company INT UNSIGNED NOT NULL,
FirstName VARCHAR(50) DEFAULT '' NOT NULL,
LastName VARCHAR(50) NOT NULL,
Income NUMERIC(10, 2) DEFAULT '1000',
HiredOn DATETIME DEFAULT CURRENT_TIMESTAMP,
FULLTEXT INDEX IDX_387341A3A16323F5 (FirstName),
UNIQUE INDEX IX_EmployeesIdentificationCode (IdentificationCode),
INDEX IDX_387341A3800230D3 (Company),
PRIMARY KEY(Id)
)
DEFAULT CHARACTER SET utf8 COLLATE utf8_unicode_ci
ENGINE = INNODB
;
ALTER TABLE Employees
ADD CONSTRAINT FK_387341A3800230D3
FOREIGN KEY (Company)
REFERENCES Companies (Id)
ON UPDATE CASCADE
ON DELETE RESTRICT
;
About the names of foreign keys and indexes: since for Doctrine they are optional, I decided to make them optional. PS: Here's how Doctrine builds the index/foreign key names:
IDX_
UNIQ_
FK_
Thanks for all the good work on this, will take a look soon.
On Fri, Mar 6, 2015 at 3:26 AM, Michele Locati notifications@github.com wrote:
About the names of foreign keys and indexes: since for Doctrine they are optional, I decided to make them optional. PS: Here's https://github.com/doctrine/dbal/blob/v2.4.4/lib/Doctrine/DBAL/Schema/AbstractAsset.php#L218-L225 how Doctrine builds the index/foreign key names.
- normal Index names are prefixed with IDX_
- unique index names are prefixed with UNIQ_
- foreign key names are prefixed with FK_
— Reply to this email directly or view it on GitHub https://github.com/concrete5/concrete5-5.7.0/issues/2025#issuecomment-77545056 .
I just merged this in for inclusion with 5.7.5. I installed 5.7.4.3 with the legacy way, and 5.7.5a1 with the new doctrine XML, and the exported SQL was IDENTICAL. Awesome job. This is great and wouldn't have happened were it not for @mlocati getting it done. :beer:
And without you, the whole concrete5 project wouldn't have even existed :wink:
So, two beers for you :beers:
We currently use AXMLS for our db schema files (version 0.3.) We should create version 0.5, based on Doctrine naming.
Make sure all type and length fields honor Doctrine values. Basically, this is a declarative XML format for Doctrine DBAL schema values.
We should make sure this handles foreign key constraints in a syntactically nice way.
We need to create a schema parser for this that is loaded based on the version (this is probably just an XML load into a doctrine schema object, very little parsing required.)
Thoughts? Thoughts on syntax for stuff that isn't supported out of the box, like foreign keys, other things we could add?