Thread: PostgreSQL driver for Joomla review

PostgreSQL driver for Joomla review

From
Thom Brown
Date:
If anyone has a moment, could they review the PostgreSQL driver I
wrote for Joomla's next major release?  The developers at Joomla have
listened to the persistent noise created about only having MySQL as an
option and are now accepting submissions for alternative database
systems. (see http://groups.google.com/group/joomla-dev-cms/browse_thread/thread/1382dc6f4af56278#msg_9b95648941ef6fa7
for this development)

My submission can be found at:
http://joomlacode.org/gf/project/joomla/tracker/?action=TrackerItemEdit&tracker_item_id=18384

Things to watch out for a version-dependent features and clumsy implementations.

Thanks

Thom Brown

Re: PostgreSQL driver for Joomla review

From
Csaba Nagy
Date:
Hi Thom,

I would like to review it, but I get "403 - Forbidden" when clicking:

http://downloads.joomlacode.org/trackeritem/4/5/0/45041/postgresql.php

Not sure what that means, probably I need some kind of login to the
joomla tracker system, and I don't have one, and I would prefer not to
create one... is it possible to access that somehow without full access
to the joomla tracker ?

Cheers,
Csaba.

On Tue, 2009-10-20 at 14:02 +0200, Thom Brown wrote:
> If anyone has a moment, could they review the PostgreSQL driver I
> wrote for Joomla's next major release?  The developers at Joomla have
> listened to the persistent noise created about only having MySQL as an
> option and are now accepting submissions for alternative database
> systems. (see
http://groups.google.com/group/joomla-dev-cms/browse_thread/thread/1382dc6f4af56278#msg_9b95648941ef6fa7
> for this development)
>
> My submission can be found at:
> http://joomlacode.org/gf/project/joomla/tracker/?action=TrackerItemEdit&tracker_item_id=18384
>
> Things to watch out for a version-dependent features and clumsy implementations.
>
> Thanks
>
> Thom Brown
>


Re: PostgreSQL driver for Joomla review

From
Thom Brown
Date:
2009/10/20 Csaba Nagy <nagy@ecircle-ag.com>:
> Hi Thom,
>
> I would like to review it, but I get "403 - Forbidden" when clicking:
>
> http://downloads.joomlacode.org/trackeritem/4/5/0/45041/postgresql.php
>
> Not sure what that means, probably I need some kind of login to the
> joomla tracker system, and I don't have one, and I would prefer not to
> create one... is it possible to access that somehow without full access
> to the joomla tracker ?

Hi Csaba,

I didn't realise it wasn't publicly viewable.  I've attached a copy of
the php file anyhow.  Can you see the tracker page at all?

Thom

Attachment

Re: PostgreSQL driver for Joomla review

From
Thom Brown
Date:
2009/10/20 Reid Thompson <reid.thompson@ateb.com>:
> your attachment contains this...
>
> <?xml version="1.0" encoding="iso-8859-1"?>
> <!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN"
>         "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
> <html xmlns="http://www.w3.org/1999/xhtml" xml:lang="en" lang="en">
>  <head>
>  <title>403 - Forbidden</title>
>  </head>
>  <body>
>  <h1>403 - Forbidden</h1>
>  </body>
> </html>
>

Erk.. that's weird.  I got that too even after being logged in.  I'm
not sure how anyone can review it if no-one has access to it.

I've attached my working version which differs only slightly to
conform with coding-styles required by Joomla.

Apologies

Thom

Attachment

Re: PostgreSQL driver for Joomla review

From
Merlin Moncure
Date:
On Tue, Oct 20, 2009 at 8:02 AM, Thom Brown <thombrown@gmail.com> wrote:
> If anyone has a moment, could they review the PostgreSQL driver I
> wrote for Joomla's next major release?  The developers at Joomla have
> listened to the persistent noise created about only having MySQL as an
> option and are now accepting submissions for alternative database
> systems. (see
http://groups.google.com/group/joomla-dev-cms/browse_thread/thread/1382dc6f4af56278#msg_9b95648941ef6fa7
> for this development)
>
> My submission can be found at:
> http://joomlacode.org/gf/project/joomla/tracker/?action=TrackerItemEdit&tracker_item_id=18384
>
> Things to watch out for a version-dependent features and clumsy implementations.

*) why bother with the version check in queryBatch?  why not just do
begin->commit always?
*) no ability to delete?
*) looks like typo on line 713


keep up the good work...

merlin

Re: PostgreSQL driver for Joomla review

From
Thom Brown
Date:
2009/10/20 Merlin Moncure <mmoncure@gmail.com>:
> On Tue, Oct 20, 2009 at 8:02 AM, Thom Brown <thombrown@gmail.com> wrote:
>> If anyone has a moment, could they review the PostgreSQL driver I
>> wrote for Joomla's next major release?  The developers at Joomla have
>> listened to the persistent noise created about only having MySQL as an
>> option and are now accepting submissions for alternative database
>> systems. (see
http://groups.google.com/group/joomla-dev-cms/browse_thread/thread/1382dc6f4af56278#msg_9b95648941ef6fa7
>> for this development)
>>
>> My submission can be found at:
>> http://joomlacode.org/gf/project/joomla/tracker/?action=TrackerItemEdit&tracker_item_id=18384
>>
>> Things to watch out for a version-dependent features and clumsy implementations.
>
> *) why bother with the version check in queryBatch?  why not just do
> begin->commit always?

To be honest, I hadn't touched that function.  That's entirely
inherited from the MySQL one, but you're right, for Postgres there's
no need to check the version.

> *) no ability to delete?

The driver has to match that of the MySQL one in Joomla, so I couldn't
implement functionality which wouldn't be used by every driver.  The
only thing I added which differed was a concatentation operator which
I put in for future implementation as I believe it will be necessary
if they're going to adapt the existing codebase to support multiple
database systems.

> *) looks like typo on line 713

Yes, that's just left over from me quickly trying to tidy up my
comments as I couldn't get the proper version from the site.  Please
ignore it as it isn't in my actual submission.

Thanks Merlin!

Thom

Re: PostgreSQL driver for Joomla review

From
Alban Hertroys
Date:
On 20 Oct 2009, at 14:02, Thom Brown wrote:

> If anyone has a moment, could they review the PostgreSQL driver I
> wrote for Joomla's next major release?  The developers at Joomla have
> listened to the persistent noise created about only having MySQL as an
> option and are now accepting submissions for alternative database
> systems. (see
http://groups.google.com/group/joomla-dev-cms/browse_thread/thread/1382dc6f4af56278#msg_9b95648941ef6fa7
> for this development)
>
> My submission can be found at:
> http://joomlacode.org/gf/project/joomla/tracker/?action=TrackerItemEdit&tracker_item_id=18384
>
> Things to watch out for a version-dependent features and clumsy
> implementations.


You miss a keyword in your query in renameTable; it should be " RENAME
TO ", not " TO ".

Wouldn't it be convenient to have an EXPLAIN ANALYSE version of explain
()? Maybe with a boolean parameter?

In insertObject() you have a query "SELECT $keyName AS 'id' FROM
$table'" - That line contains two syntax errors: 'id' (You probably
meant "id") and $table' (spurious trailing quote).

Regards,
Alban Hertroys

--
Screwing up is the best way to attach something to the ceiling.


!DSPAM:737,4addf36611682031315238!



Re: PostgreSQL driver for Joomla review

From
Thom Brown
Date:
2009/10/20 Alban Hertroys <dalroi@solfertje.student.utwente.nl>:
> You miss a keyword in your query in renameTable; it should be " RENAME TO ",
> not " TO ".

Thanks for spotting that.  I've made my amendments for next submit.

> Wouldn't it be convenient to have an EXPLAIN ANALYSE version of explain()?
> Maybe with a boolean parameter?

The problem is, I don't see where they're using this function, so if
they're using it to EXPLAIN an INSERT, UPDATE or DELETE statement,
ANALYZE would execute it... unless I roll it back straight after.

> In insertObject() you have a query "SELECT $keyName AS 'id' FROM $table'" -
> That line contains two syntax errors: 'id' (You probably meant "id") and
> $table' (spurious trailing quote).

Again, you're right and well spotted. :)  Fixed both of those.

Cheers for helping out Alban! :D

Thom

Re: PostgreSQL driver for Joomla review

From
Csaba Nagy
Date:
Hi Thom,

Sorry for the delay, I got sick in the meantime. I see that others
already did some review, I will do a quick one too, later maybe I'll
actually try it out... so after a quick review:

* on line 218, the " ENCODING '$DBname')" part feels wrong, you probably
want hardcoded UTF8 encoding there ?
* as Merlin already commented, transactions are always safe in postgres,
this is no mysql ;-)
* again, as Merlin commented, getTableList is getting the data bases,
which doesn't make sense, but maybe you actually wanted to get the
tables - in this case you don't have a typo but you need to change the
query ;-)

If I'll get some time I'll test it too, but likely not this week...

Cheers,
Csaba.


On Tue, 2009-10-20 at 15:28 +0200, Thom Brown wrote:
> 2009/10/20 Reid Thompson <reid.thompson@ateb.com>:
> > your attachment contains this...
> >
> > <?xml version="1.0" encoding="iso-8859-1"?>
> > <!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN"
> >         "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
> > <html xmlns="http://www.w3.org/1999/xhtml" xml:lang="en" lang="en">
> >  <head>
> >  <title>403 - Forbidden</title>
> >  </head>
> >  <body>
> >  <h1>403 - Forbidden</h1>
> >  </body>
> > </html>
> >
>
> Erk.. that's weird.  I got that too even after being logged in.  I'm
> not sure how anyone can review it if no-one has access to it.
>
> I've attached my working version which differs only slightly to
> conform with coding-styles required by Joomla.
>
> Apologies
>
> Thom


Re: PostgreSQL driver for Joomla review

From
Thom Brown
Date:
2009/10/21 Csaba Nagy <nagy@ecircle-ag.com>:
> Hi Thom,
>
> Sorry for the delay, I got sick in the meantime. I see that others
> already did some review, I will do a quick one too, later maybe I'll
> actually try it out... so after a quick review:
>
> * on line 218, the " ENCODING '$DBname')" part feels wrong, you probably
> want hardcoded UTF8 encoding there ?
> * as Merlin already commented, transactions are always safe in postgres,
> this is no mysql ;-)
> * again, as Merlin commented, getTableList is getting the data bases,
> which doesn't make sense, but maybe you actually wanted to get the
> tables - in this case you don't have a typo but you need to change the
> query ;-)
>
> If I'll get some time I'll test it too, but likely not this week...

Thanks for the feedback Csaba,

Yes, I will change the function which uses transactions, and also
replace any conditional UTF8 statements with hard-coded ones.

The getTableList function...erm... not sure what I was thinking there.
 How about:

select tablename from pg_tables where schemaname='public';

As for testing it, you won't be able to with the existing Joomla code
base as most of the queries will fail as they've been terribly
written, and that's because MySQL lets you get away with it for some
reason.  I imagine the only way to test it is to either get a copy of
modified Joomla from me (although it still needs work), or create a
test app which uses that class, but you will need the parent JDatabase
class from Joomla too... and not sure how deep that rabbit hole goes
as that will also in turn inherit from another class.

Thanks again.

Thom