Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

WIP: Sqlite migration and foreignKey support #612

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
3 participants
Owner

marcj commented Feb 18, 2013

Just need the PR id in another ticket. Maybe it's done in a few days.

marcj added some commits Feb 18, 2013

Added buildtime-conf.xml for the bookstore fixure, where we use now '…
…sqlite' in the 'bookstore' database section. (for testing purposes)

This is in WIP. Next step is that travis-ci uses several environments where we test sqlite/pgsql/mysql, not as currently where the most tests are against mysql.
This generated a couple of issues since some tests got the ($this->con) connection from the wrong database. Also fixed a bunch of errors with the new SQLite stuff.

@staabm staabm commented on the diff Feb 18, 2013

generator/lib/model/Table.php
@@ -1582,6 +1582,18 @@ public function getForeignKeys()
}
/**
+ * Removes a foreign key.
+ *
+ * @param ForeignKey $fk
+ */
+ public function removeForeignKey(ForeignKey $fk){
+ $idx = array_search($fk, $this->foreignKeys);
@staabm

staabm Feb 18, 2013

Member

use isset instead because its a lot faster

@marcj

marcj Feb 18, 2013

Owner

It's about removing the FK, not checking.

@staabm staabm commented on the diff Feb 18, 2013

generator/lib/platform/SqlitePlatform.php
@@ -21,6 +21,22 @@ class SqlitePlatform extends DefaultPlatform
{
/**
+ * If we should generate FOREIGN KEY statements.
+ * This is since SQLite version 3.6.19 possible.
+ *
+ * @var bool
+ */
+ private $foreignKeySupport = true;
+
+ /**
+ * If we should alter the table through creating a temporarily crated table,
@staabm

staabm Feb 18, 2013

Member

in which situation is this workaround required?

@marcj

marcj Feb 18, 2013

Owner

sqlite.org/lang_altertable.html

@staabm

staabm Feb 18, 2013

Member

this should be documented in the code...

@marcj

marcj Feb 18, 2013

Owner

It's a WIP. It will be included in the build properties section in a comment. This limitation is very known under SQLite developers, so no need to highlight a apparent fact in the code.

@staabm staabm commented on the diff Feb 18, 2013

generator/lib/platform/SqlitePlatform.php
+ }
+
+ }
+
+ /**
+ * Creates a temporarily created table with the new schema,
+ * moves all items into it and drops the origin as well as renames the temp table to the origin then.
+ *
+ * @param PropelTableDiff $tableDiff
+ * @return string
+ */
+ public function getMigrationTableDDL(PropelTableDiff $tableDiff){
+
+ $pattern = "
+%s;
+INSERT INTO %s (%s)
@staabm

staabm Feb 18, 2013

Member

shouldn't you use a prepared statement?

@staabm staabm commented on the diff Feb 18, 2013

generator/lib/platform/SqlitePlatform.php
@@ -90,26 +265,130 @@ public function getAddTableDDL(Table $table)
);
}
+ /**
+ * Unfortunately, SQLite does not support composite pks where one is AUTOINCREMENT,
+ * so we have so flag both as NOT NULL and create a UNIQUE constraint.
@staabm

staabm Feb 18, 2013

Member

typo: so -> to

@staabm staabm commented on the diff Feb 18, 2013

generator/lib/platform/SqlitePlatform.php
+ foreach ($pks as $pk){
+ //no pk can be NULL, as usual
+ $pk->setNotNull(true);
+ //in SQLite the column with the AUTOINCREMENT MUST be a primary key, too.
+ if (!$pk->isAutoIncrement()){
+ //for all other sub keys we remove it, since we create a UNIQUE constraint over all primary keys.
+ $pk->setPrimaryKey(false);
+ }
+ }
+
+ //search if there is already a UNIQUE constraint over the primary keys
+ $pkUniqueExist = false;
+ foreach ($table->getUnices() as $unique){
+ $allPk = false;
+ foreach ($unique->getColumns() as $columnName){
+ $allPk &= $table->getColumn($columnName)->isPrimaryKey();
@staabm

staabm Feb 18, 2013

Member

why do you assign by ref?

@staabm

staabm Feb 18, 2013

Member

I think you meant $allPk = $table->getColumn($columnName)->isPrimaryKey() && $allPk;, but than you need to init $allPk with true

@marcj

marcj Feb 18, 2013

Owner

It's a binary operation, not a ref assign.

@staabm

staabm Feb 18, 2013

Member

so you should write it in the long form to improve readability.

@marcj

marcj Feb 18, 2013

Owner

I really appreciate your work, but please, don't discuss a WIP PR yet. If your opinion is that combined operators are not readable to you, than that's totally OK to me, but please don't declare it as 'you should not do that'.

@staabm

staabm Feb 18, 2013

Member

sure. sorry for that, and don't do it ;-).

Member

staabm commented Feb 18, 2013

sorry for the early feedback, I just recognized its an WIP...

ssokolow commented May 4, 2013

Has there been any progress on this? I have a project which I'd like to move to a fatter model but whether I wait and switch to Propel or just jerry rig something on top of my existing solution will depend on how likely this is to be ready within the next few months.

Member

staabm commented May 4, 2013

You are welcome to provide the remaing changes for this PR. I don't know if @marcj will have time for this

ssokolow commented May 4, 2013

Given how little time I have, the "within the next few months" may be a limit imposed by when I have time to migrate to Propel or even just make changes to my existing database code.

As much as I'd love to help, there's no way I have time to grow familiar enough with the Propel codebase to contribute a patch in the near future.

However, in the longer term (say... July, maybe?), it's a possibility if you or someone else familiar with the project can point me to a crash course in how the Propel codebase is laid out, how comprehensive the unit tests are, what standards need to be met for this patch to get accepted (including what extra unit tests I might have to write), etc.

Member

staabm commented May 4, 2013

Sure. Just give us a ping when you have time

Owner

marcj commented May 27, 2013

I'll close this PR soon and will integrate it in Propel2.

@marcj

In three days, it'll have been two months since you said "soon". I'd like to start using Propel2 on an SQLite-backed project before my vacation ends.

Did you forget or is there some hold-up I might be able to handle for you?

Owner

marcj commented Jul 25, 2013

Unfortunately, I'm not able to work on this issue, since I'm working on another big project for the next weeks. But I guess I need this PR as well in that project (but in Propel2). However, this PR is 'nearly' done, so it shouldn't be much work to complete it.

Owner

marcj commented Aug 20, 2013

I've implemented those features in Propel2: propelorm/Propel2#433

@marcj marcj closed this Aug 20, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment