New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[wip] 3.0 - Implement connection prefixes (#2666) #3843

Closed
wants to merge 49 commits into
base: 3.0
from

Conversation

Projects
None yet
5 participants
@HavokInspiration
Member

HavokInspiration commented Jun 30, 2014

This PR aims to implements connection prefixes (#2666).

Since it's my first PR ever on an open source project, feel free to tell me if I did anything wrong.
I also have a some questions regarding "Cakey" way to do things :

1/ Should the Connection::fullTableName() method throws an exception if the $name parameter is neither a string nor an array ?
2/ Are there any missing tests ? (ie. should I write test for the Query::from() and Query::join() methods as well as the Collection::describe() ? I tried for the last one but was not successful in having correct results. I was hopping a core contributor could help me write this test)

@lorenzo

View changes

Show outdated Hide outdated src/Database/Connection.php

@markstory markstory added this to the 3.0.0 milestone Jun 30, 2014

@markstory

View changes

Show outdated Hide outdated src/Database/Connection.php
@HavokInspiration

This comment has been minimized.

Show comment
Hide comment
@HavokInspiration

HavokInspiration Jul 1, 2014

Member

@lorenzo Indeed, if $tableName is the loop happened to be a Query object, everything broke.

@markstory While I understand the issue concerning double prefixed table names, I'm not quite sure how to proceed. I could check if the table name doesn't start with the prefix but if one of the table name (not prefixed) starts with the prefix defined, then the name would not be prefixed (I don't know if I am being really clear...)
The other solution would be to have an array that stores all prefixed tables but the problem described above would still occur...

Member

HavokInspiration commented Jul 1, 2014

@lorenzo Indeed, if $tableName is the loop happened to be a Query object, everything broke.

@markstory While I understand the issue concerning double prefixed table names, I'm not quite sure how to proceed. I could check if the table name doesn't start with the prefix but if one of the table name (not prefixed) starts with the prefix defined, then the name would not be prefixed (I don't know if I am being really clear...)
The other solution would be to have an array that stores all prefixed tables but the problem described above would still occur...

@markstory

This comment has been minimized.

Show comment
Hide comment
@markstory

markstory Jul 2, 2014

Member

@HavokInspiration One option would be instead of manipulating strings for table names, table names could be wrapped in a TableNameExpression that appends the prefix name when it is converted into SQL. Having a wrapper object for prefixed table names would prevent double prefixing and should work well with the existing query builder internals.

Member

markstory commented Jul 2, 2014

@HavokInspiration One option would be instead of manipulating strings for table names, table names could be wrapped in a TableNameExpression that appends the prefix name when it is converted into SQL. Having a wrapper object for prefixed table names would prevent double prefixing and should work well with the existing query builder internals.

@@ -374,6 +374,10 @@ public function from($tables = [], $overwrite = false) {
$tables = [$tables];
}
if ($this->_connection instanceof \Cake\Database\Connection) {

This comment has been minimized.

@markstory

markstory Jul 8, 2014

Member

Why is this here?

@markstory

markstory Jul 8, 2014

Member

Why is this here?

This comment has been minimized.

@HavokInspiration

HavokInspiration Jul 9, 2014

Member

Some tests (in \Cake\ORM\ if I remember correctly) creates an instance of \Cake\ORM\Query with a null connection parameter. Errors were raised by those tests since it was calling a method from a non-object. So I had to make sure that the _connection property was what it should be.

@HavokInspiration

HavokInspiration Jul 9, 2014

Member

Some tests (in \Cake\ORM\ if I remember correctly) creates an instance of \Cake\ORM\Query with a null connection parameter. Errors were raised by those tests since it was calling a method from a non-object. So I had to make sure that the _connection property was what it should be.

This comment has been minimized.

@markstory

markstory Jul 9, 2014

Member

We should probably fix those tests, they are probably wrong in other ways as well.

@markstory

markstory Jul 9, 2014

Member

We should probably fix those tests, they are probably wrong in other ways as well.

This comment has been minimized.

@dereuromark

dereuromark Jul 9, 2014

Member

Yep, the second argument is already fixed in https://github.com/cakephp/cakephp/pull/3802/files#diff-697d798d5d5411ad5dce707736c9cfb5R105 - as it now has to be of type Table.
But the first one still often can be null.

@dereuromark

dereuromark Jul 9, 2014

Member

Yep, the second argument is already fixed in https://github.com/cakephp/cakephp/pull/3802/files#diff-697d798d5d5411ad5dce707736c9cfb5R105 - as it now has to be of type Table.
But the first one still often can be null.

HavokInspiration added some commits Jul 11, 2014

Clean up TableNameExpression constructor
Removed parameters that were not used anymore because of changes made in past commits.
Reduces indentation by creating a function
This also helps keep the code DRY
Change param and return variable type
According to \Cake\Database\Query::from() and \Cake\Database\Query::join(), only string, array and ExpressionInterface type can be passed as table name. These changes aims to follow this.
@HavokInspiration

This comment has been minimized.

Show comment
Hide comment
@HavokInspiration

HavokInspiration Jul 11, 2014

Member

Hm... Tests are failing on Travis but when I run them on my local workspace, all tests pass... I don't understand why.

Edit : OK, I understand why... they are all failing except with SQLite.

Edit 2 : From what I gathered in other PR, it seems to be an "general" issue.

Member

HavokInspiration commented Jul 11, 2014

Hm... Tests are failing on Travis but when I run them on my local workspace, all tests pass... I don't understand why.

Edit : OK, I understand why... they are all failing except with SQLite.

Edit 2 : From what I gathered in other PR, it seems to be an "general" issue.

HavokInspiration added some commits Jul 11, 2014

Update TableNameExpression tests
Removes the hardcoded quoters
Revert previous commit (3611a04) and implements a more general way of…
… resolving table name to TableNameExpression in Query::join()

While testing, I realized some of the tests did not convert table names to TableNameExpression as a complex array can be given as first argument to Query::join().
This change makes the previous one useless as the present commit is more "general" and takes into account the complex array (with the table name given in the array with the "table" key).
@HavokInspiration

This comment has been minimized.

Show comment
Hide comment
@HavokInspiration

HavokInspiration Jul 20, 2014

Member

I made an update to the recent addition made in #4017.
While testing I realized some table names in Query::join() where not converted to TableNameExpression as the fullTableName method did not took into account all the forms the first argument of Query::join() could be given.
So I reverted it and implemented a more general way that covers more cases than it did (and hopefully all cases).
I will probably try to refactor this a bit as I'm starting to find the fullTableName method a bit messy...

I also need to update the fullTableName tests. Done.

Member

HavokInspiration commented Jul 20, 2014

I made an update to the recent addition made in #4017.
While testing I realized some table names in Query::join() where not converted to TableNameExpression as the fullTableName method did not took into account all the forms the first argument of Query::join() could be given.
So I reverted it and implemented a more general way that covers more cases than it did (and hopefully all cases).
I will probably try to refactor this a bit as I'm starting to find the fullTableName method a bit messy...

I also need to update the fullTableName tests. Done.

HavokInspiration added some commits Jul 20, 2014

Update fullTableName method tests
This add a test to take into account the complex array syntax of the first argument that can be given to a Query::join()
Simplify the purpose of the fullTableName method
While refactoring the fullTableName method, I realized that some Expressions (such as QueryExpression) and even Query object where wrapped in a TableNameExpression which was pointless.
This change simplifies the use of TableNameExpression : it now only wraps strings to prepend the prefix if needed.
@HavokInspiration

This comment has been minimized.

Show comment
Hide comment
@HavokInspiration

HavokInspiration Jul 20, 2014

Member

I pushed more commits where I attempt to simplify the purpose of the fullTableName method and TableNameExpression.
TableNameExpression now only wraps strings as it was pointless for it to wrap QueryExpressions or Query (as when TableNameExpression was rendered as SQL it was only rendering every expression into SQL)

Member

HavokInspiration commented Jul 20, 2014

I pushed more commits where I attempt to simplify the purpose of the fullTableName method and TableNameExpression.
TableNameExpression now only wraps strings as it was pointless for it to wrap QueryExpressions or Query (as when TableNameExpression was rendered as SQL it was only rendering every expression into SQL)

@markstory

This comment has been minimized.

Show comment
Hide comment
@markstory

markstory Jul 20, 2014

Member

I think the fixtures should also apply the connection prefix, which doesn't look like it is hooked up yet. But the changes so far are looking good.

Member

markstory commented Jul 20, 2014

I think the fixtures should also apply the connection prefix, which doesn't look like it is hooked up yet. But the changes so far are looking good.

@HavokInspiration

This comment has been minimized.

Show comment
Hide comment
@HavokInspiration

HavokInspiration Sep 5, 2014

Member

I'm starting to find time to dive in this again.
I'm closing this PR as it's almost 2 months late from the current state of Cake 3.0.
I will start again and open a new one with a fresh fork (I also -very- poorly managed my branches so it will give me the occasion to do things better...)

I'll submit a new PR soon (I hope I can keep that promise).

Member

HavokInspiration commented Sep 5, 2014

I'm starting to find time to dive in this again.
I'm closing this PR as it's almost 2 months late from the current state of Cake 3.0.
I will start again and open a new one with a fresh fork (I also -very- poorly managed my branches so it will give me the occasion to do things better...)

I'll submit a new PR soon (I hope I can keep that promise).

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