-
-
Notifications
You must be signed in to change notification settings - Fork 6.9k
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
Pgsql driver implementation (update 4) #505
Conversation
@@ -343,6 +343,8 @@ protected function createPdoInstance() | |||
$driver = strtolower(substr($this->dsn, 0, $pos)); | |||
if ($driver === 'mssql' || $driver === 'dblib' || $driver === 'sqlsrv') { | |||
$pdoClass = 'yii\db\mssql\PDO'; | |||
} else if ($driver === 'pgsql') { |
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.
elseif
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.
Need to change, Netbeans settings :(
Do we really need the |
|
||
/** | ||
* Returns value of the last inserted ID. | ||
* @param string|null $sequence the sequence name. Defaults to 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.
What does null
mean here?
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.
If you call getLastInsertedId without inserting a record first then, this method returns null since internally postgresql returns 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.
Question was about $sequence
, not about return value.
The PDO class is the correct place to implement the schema switching functionality to my opinion. |
} | ||
} | ||
if (isset($options[self::OPT_DEFAULT_SCHEMA])) { | ||
$schema = trim($options[self::OPT_DEFAULT_SCHEMA]); |
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.
Is it possible that default schema is set to multiple spaces? If not then we can use !empty($options[self::OPT_DEFAULT_SCHEMA])
instead of isset
and trim
and empty string check.
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 check is to make sure we have a default schema set even if the option exist with en empty value or when the option has a whitespace as a value.
It seems to me we don't really need the custom |
* @author Gevik Babakhani <gevikb@gmail.com> | ||
* @since 2.0 | ||
*/ | ||
class QueryBuilder extends \yii\db\QueryBuilder { |
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.
{
should be at the next line
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.
done
Refactored == null to is_null == '' to empty(...)
I still don't understand why we need the custom |
This was in outdated diff, so maybe you did not check it: |
I have created updates as suggested in the comment.Please review again. |
still constants presents in classes, or about what comment you were talking about? |
@qiangxue I was under impression that the schema changing/seeting functionality should be as low level as possible. Changing or setting database schema is something that you do only once and to my option it should be set and passed to PDO my configuration. In event of someone needing to set it manually, then they can use the |
command since postgres does not accept db creation in multiline statement.
I think we should drop the custom |
Removed the custom PDO and added |
try { | ||
$constraints = $this->db->createCommand($sql)->queryAll(); | ||
} catch (\Exception $e) { | ||
return false; |
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.
Should not catch exceptions because it will hide the reason why the query fails.
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.
I missed that. I have removed it. thanks.
Pgsql driver implementation (update 4)
Good job! Thanks! |
@mark, I hope you get this message. I have looked in the comments but On 6/8/13 5:10 AM, Mark wrote:
|
); | ||
|
||
/** | ||
* Creates a query builder for the MySQL database. |
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.
MySQL → PostgreSQL
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.
Will fix
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.
fixed
@gevik everythng is ok, i was a little bit lost in this big number of commits ;) sorry. Thanks for the pgsql support :) |
Updated the suggested comments, please review again