-
Notifications
You must be signed in to change notification settings - Fork 122
Conversation
@turrsis Could you please:
The latter, in particular, will help us better understand the impact on end-users, and, of course, will be necessary for us to release any changes! Thanks! |
@weierophinney I'll do it, will require about a week |
@turrsis No rush! And thanks in advance! |
@weierophinney rebase done, sorry for delay. Fix CS and documentation I will do at next week. |
@weierophinney Where I should change documentation - in |
@weierophinney rebase and documentation is done |
* @return Ddl\AlterTable | ||
* @throws Exception\InvalidArgumentException | ||
*/ | ||
public function alterTable($table = null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not needed, because there is already a DDL Abstraction.
I think, this is enough. (separation of concerns)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, I will fix it.
@turrsis How much sense, if at all, does it make for me to continue working on #163 to fix PostgreSQL problems? It started as a small fix, but with lack of existing PostgreSQL decorator I had to make one. I fixed the index creation problem so far, and since there are long standing PgSQL DDL compatibility issues reported in general, I was going to expand on that decorator in attempts to make it as reliable as MySQL decorator. But the more I look at your changelist, I notice you do not just change the builder architecture but also adding lots of proper database abstractions, which is great!! So I am wondering that it would not be appropriate to duplicate the effort and confuse the maintainers with conflicting PRs if you are are targeting that area as well, but its a little hard to tell what to do without a direction/plan of what is going into this refactor. What are you thoughts? and thanks for the amazing work! |
@alextech Sometimes I rebase this branch to last. If you will write correct tests, and you PR will be acepted - I apply you PR to this PR. |
@weierophinney Can you help me - where I wrong with last commit ("add travis ZF3 tests") ? |
…r, protect all builders methods, unification names for getSqlString() and prepareStatement() methods
…t stable decision
Closing this PR since it's more than 1 year old. |
@ezimuel Why was this pull request closed? It looks like a lot of work is sitting here, including incorporating change requests, and there are older PRs in the project that are still open. Did the author reach out and say they abandoned this approach? Or is something similar being more actively developed somewhere else? thx |
Main architecture issue - current SqlObjects includes SqlBuilders.
This is not good design, it's coupe two separate things together - Describing and Building Query, including:
Expression
. As sampleconcat()
function have different syntax for various DBInsert::into('foo')->select(Select::from('bar')->limit(10))
not work for SqlServerSolution:
Expressions
Builder\Builder::getSqlString()
andBuilder\Builder::prepareStatement()
.Additionally was changed:
AbstractSqlBuilder::createSqlFromSpecificationAndParameters()
- more readable and flexible specificationsExpressionParameter
objectTableSource
orTableIdentifier
Select::limit()
andSelect::offset()
MySql\Ddl\CreateTableBuilder
andMySql\Ddl\AlterTableBuilder
toMysql\Ddl\Column\ColumnBuilder
Sql\Insert
- add multiple valuesSql\Ddl\DropTable
- add 'IF EXISTS'Sql\Ddl\Sql
oblect for DDLSql\Join
toSql\Joins
Sql\SqlInterface
toSql\SqlObjectInterface
andSql\PreparableSqlInterface
toSql\PreparableSqlObjectInterface