This repository has been archived by the owner. It is now read-only.

Added some method inside JDatabaseDriver to create database and alter character set [v2] #1625

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
5 participants
@gpongelli
Contributor

gpongelli commented Oct 21, 2012

Added two public method inside JDatabaseDriver:

  • alterDbCharacterSet to change database's character set
  • createDatabase to create a new database

AlterDbCharacterSet needs database name as parameter, createDatabase needs a JObject, similar to that used inside CMS' "initialise" function, and a bool to set UTF8 database's support.

They use two protected method to obtain correct query in database's
supported syntax:

  • getAlterDbCharacterSet
  • getCreateDatabaseQuery

Protected method inside JDatabaseDriver return query for MySQL and MySQLi syntax, this commit contain also overridden version for PostgreSQL database.

There are tests only for protected methods inside MySQL, MySQLi and PostgreSQL test classes.

Old Pull request

Added some method inside JDatabaseDriver to create database and alter
character set for MySQL, MySQLi and PostgreSQL.
Added test too.
@elinw

This comment has been minimized.

Show comment Hide comment
@elinw

elinw Oct 21, 2012

Contributor

Gabriele you have a unit test failure but I'm not sure why.

Contributor

elinw commented Oct 21, 2012

Gabriele you have a unit test failure but I'm not sure why.

@gpongelli

This comment has been minimized.

Show comment Hide comment
@gpongelli

gpongelli Oct 22, 2012

Contributor

Could you write the error?
Thanks.

Contributor

gpongelli commented Oct 22, 2012

Could you write the error?
Thanks.

@elkuku

This comment has been minimized.

Show comment Hide comment
@elkuku

elkuku Oct 22, 2012

Member
@gpongelli

This comment has been minimized.

Show comment Hide comment
@gpongelli

gpongelli Oct 22, 2012

Contributor

I see it's related to JOAuth2ClientTest, I've not developed that test.
Check if this error is also present on master branch, I forked from it yesterday to create this pull request.

Contributor

gpongelli commented Oct 22, 2012

I see it's related to JOAuth2ClientTest, I've not developed that test.
Check if this error is also present on master branch, I forked from it yesterday to create this pull request.

*
- * @since 12.1
+ * @since 12.2

This comment has been minimized.

Show comment Hide comment
@elinw

elinw Oct 22, 2012

Contributor

12.3

@elinw

elinw Oct 22, 2012

Contributor

12.3

+ *
+ * @return array
+ *
+ * @since 11.3

This comment has been minimized.

Show comment Hide comment
@elinw

elinw Oct 22, 2012

Contributor

12.3

@elinw

elinw Oct 22, 2012

Contributor

12.3

+ * @return array
+ *
+ * @since 11.3
+ */

This comment has been minimized.

Show comment Hide comment
@elinw

elinw Oct 22, 2012

Contributor

12.3

@elinw

elinw Oct 22, 2012

Contributor

12.3

This comment has been minimized.

Show comment Hide comment
@gpongelli

gpongelli Oct 22, 2012

Contributor

Changed.

@gpongelli

gpongelli Oct 22, 2012

Contributor

Changed.

@elinw

This comment has been minimized.

Show comment Hide comment
@elinw

elinw Oct 22, 2012

Contributor

So in some indirect way this change is impacting OAuth2 the issue to figure out is how.

Contributor

elinw commented Oct 22, 2012

So in some indirect way this change is impacting OAuth2 the issue to figure out is how.

@ianmacl

This comment has been minimized.

Show comment Hide comment
@ianmacl

ianmacl Oct 23, 2012

Contributor

The error is an intermittent error related to comparing timestamps and has been fixed in master so don't worry about it (though if you rebase it will go away).

My main question regarding this pull request is that it would seem better to use the PHP methods to do this so that the client is aware of what it is supposed to be sending rather than using direct queries.

Contributor

ianmacl commented Oct 23, 2012

The error is an intermittent error related to comparing timestamps and has been fixed in master so don't worry about it (though if you rebase it will go away).

My main question regarding this pull request is that it would seem better to use the PHP methods to do this so that the client is aware of what it is supposed to be sending rather than using direct queries.

@eddieajau

This comment has been minimized.

Show comment Hide comment
@eddieajau

eddieajau Oct 25, 2012

Contributor

The JDatabaseImporter classes are already handling schema manipulation. My personal preference would be to continue to do this outside of the driver itself, maybe even starting some sort of ETL layer (which I had hoped to do someday myself) or schema helper, and moving stuff like dropTable in there as well.

Contributor

eddieajau commented Oct 25, 2012

The JDatabaseImporter classes are already handling schema manipulation. My personal preference would be to continue to do this outside of the driver itself, maybe even starting some sort of ETL layer (which I had hoped to do someday myself) or schema helper, and moving stuff like dropTable in there as well.

@ianmacl

This comment has been minimized.

Show comment Hide comment
@ianmacl

ianmacl Dec 18, 2012

Contributor

IMO those sorts of manipulation methods belong in the JDatabaseDriver, not the JDatabaseImporter class, IMO. The importer methods will certainly use them, but shouldn't own them. In some cases you can't create, remove databases and tables without access to the underlying connection object which shouldn't be something that JDatabaseImporter has access to.

Contributor

ianmacl commented Dec 18, 2012

IMO those sorts of manipulation methods belong in the JDatabaseDriver, not the JDatabaseImporter class, IMO. The importer methods will certainly use them, but shouldn't own them. In some cases you can't create, remove databases and tables without access to the underlying connection object which shouldn't be something that JDatabaseImporter has access to.

@gpongelli

This comment has been minimized.

Show comment Hide comment
@gpongelli

gpongelli Dec 31, 2012

Contributor

I added these methods to let installation phase works.
I did these changes for J2.5 , then I haven't followed subsequent joomla's changes.

I agree with Ian, the driver has to own all kinds of database's manipulation (tables, schema, etc); the other classes use the driver to perform their task without owning this code.
In my humble opinion this is a better solution for a logical point of view (who owns what) and for code clearness (piece of code exists at only one point).

Eng. Gabriele Pongelli.

Contributor

gpongelli commented Dec 31, 2012

I added these methods to let installation phase works.
I did these changes for J2.5 , then I haven't followed subsequent joomla's changes.

I agree with Ian, the driver has to own all kinds of database's manipulation (tables, schema, etc); the other classes use the driver to perform their task without owning this code.
In my humble opinion this is a better solution for a logical point of view (who owns what) and for code clearness (piece of code exists at only one point).

Eng. Gabriele Pongelli.

@elinw

This comment has been minimized.

Show comment Hide comment
@elinw

elinw Jan 31, 2013

Contributor

It would be so helpful to have create database. The current importer is extremely limited and there is no sqlsrv support so just as a side consideration the idea of being able to create and actually potentially do a complete installation of an application including creating the database in the abstraction layer is very exciting.

Contributor

elinw commented Jan 31, 2013

It would be so helpful to have create database. The current importer is extremely limited and there is no sqlsrv support so just as a side consideration the idea of being able to create and actually potentially do a complete installation of an application including creating the database in the abstraction layer is very exciting.

@eddieajau

This comment has been minimized.

Show comment Hide comment
@eddieajau

eddieajau Mar 16, 2013

Contributor

I'm going to close this because I really want to see database manipulation handled in another class.

Contributor

eddieajau commented Mar 16, 2013

I'm going to close this because I really want to see database manipulation handled in another class.

@eddieajau eddieajau closed this Mar 16, 2013

@gpongelli

This comment has been minimized.

Show comment Hide comment
@gpongelli

gpongelli Mar 16, 2013

Contributor

Ok.
I did these changes to let CMS installs correctly on PostgreSQL.

Contributor

gpongelli commented Mar 16, 2013

Ok.
I did these changes to let CMS installs correctly on PostgreSQL.

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