Initial file import from laminas-db (proposed structure only) #5
Conversation
Signed-off-by: Joey Smith <jsmith@webinertia.net> Signed-off-by: Joey Smith <jsmith@webinertia.net>
github-project-automation
bot
moved this from Todo
to Done
in @laminas-db-adapter-mysql
| } | ||
|
|
||
| // localize | ||
| $p = $this->connectionParameters; |
There was a problem hiding this comment.
Would prefer $p -> $parameters for the sake of clarity
There was a problem hiding this comment.
Yea, they will get normalized I'm sure. I was focused on higher level stuffz. We'll get to the granular stuffz pretty soon.
| */ | ||
| public function rollback() | ||
| { | ||
| if (! $this->isConnected()) { |
There was a problem hiding this comment.
Because of the $inTransaction check we could probably remove the isConnected() call
There was a problem hiding this comment.
Yea, but I want to get a feel for the other satellite workflows and get a few of them behind us then we can be relatively sure we're not going to be breaking major stuffz.
| */ | ||
| public function getCurrentSchema() | ||
| { | ||
| if (! $this->isConnected()) { |
There was a problem hiding this comment.
Lots of duplication for the connection check - perhaps just a separate method call prepareConnection() which performs that function across multiple methods? Could even be a method within the AbstractConnection class?
There was a problem hiding this comment.
Its not just in this family of classes. You find that nearly everywhere. So.... I need to reserve comment on that for awhile.
| return $this; | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
This whole method looks like it could be moved to the AbstractConnection class? There's not much specific to Mysqli
There was a problem hiding this comment.
Well, I'll have to look that up since all I have to work from is line numbers :P
| * @param string $nameFormat | ||
| * @return string | ||
| */ | ||
| public function getDatabasePlatformName($nameFormat = self::NAME_FORMAT_CAMELCASE) |
There was a problem hiding this comment.
Worth adding in constants for Database name?
There was a problem hiding this comment.
I'm considering it via a Backed Enum. Would like to get a little further along before we make that decision though. No sake in adding code just to be adding code because with the way this is working out there is only a single DatabasePlatformName that could possibly be valid and that is the one for whichever satellite is currently installed.
No description provided.