[G3/Pg] Some news

[Bharat Mediratta]

Romain LE DISEZ wrote:
> I'm working on it. I finally found a place where Kohana convert a
> boolean to an integer, it's very strange, I will propose a patch. This
> way the diff for Gallery will be smallest and more coherent.
>
> I'm leaving today for a two weeks trip so don't expect to see a commit
> before three or four weeks.

No hurry.  Enjoy your trip!

------------------------------------------------------------------------------
__[ g a l l e r y - d e v e l ]_________________________

[ list info/archive --> http://gallery.sf.net/lists.php ]
[ gallery info/FAQ/download --> http://gallery.sf.net ]

[Romain LE DISEZ-5]

Hi all,

Le mercredi 24 juin 2009 à 17:26 -0700, Bharat Mediratta a écrit :
> How about leaving the field as a BOOLEAN and fixing all the occurrences
> of 1/0 to true/false?  Upon some reflection, I think that's a fair
> change for us to make in Gallery3 because it makes it clear to us that
> we're dealing with a bool.  This is a discipline that we should probably
> have.  If that works for you, then make that change in your repo and
> I'll pull it into Gallery3.

As I said before leaving, I was working on it. I think it's ready now. I
pushed it in my repo. Some explications to fully understand the diff :

1/ MySQL doesn't know what is a BOOL (that's terific but it's true)
   When creating a BOOLEAN column, MySQL create a TINYINT(1). So, the
   value of the bool is an integer between 0 and 127. There is now way
   to deal with that

2/ Kohana convert boolean to integer

3/ Kohana consider a BINARY(1) as a BOOLEAN (but not a TINYINT(1), can't
   understand why...)


So, the solution is :

1/ Patch Kohana (system/libraries/drivers/Database.php) to works well
   with boolean (I will propose this modification upstream soon)

2/ Change :
    - access::ALLOW   to TRUE,
    - access::DENY    to FALSE,
    - access::UNKNOWN to NULL
   I also create access::INHERIT (NULL) to clarify the differences
   between NULL in access_intent and in the cache

3/ Convert the columns (view_X, view_full_X, ...) to BINARY(1)

I also changed the definition of items.view_X to be nullable. It must be
nullable because in access::_update_access_view_cache() these columns
are set to null (step 2).


With this modification, it should be easiest to make it compatible with
other RDBMS. The code is also more clear I think (I replaced all
occurences of "null" and "0" by INHERIT/UNKNOWN and FALSE).


I hope it is clear and complete but I'm really tired so it's probably
not. I let you look at the code and I wait for comments. I will write
the code for upgrader after you approved the modification.

Greetings.


------------------------------------------------------------------------------
Enter the BlackBerry Developer Challenge  
This is your chance to win up to $100,000 in prizes! For a limited time,
vendors submitting new applications to BlackBerry App World(TM) will have
the opportunity to enter the BlackBerry Developer Challenge. See full prize  
details at: http://p.sf.net/sfu/Challenge
__[ g a l l e r y - d e v e l ]_________________________

[ list info/archive --> http://gallery.sf.net/lists.php ]
[ gallery info/FAQ/download --> http://gallery.sf.net ]

[Romain LE DISEZ-5]

Le 19 juil. 09 à 01:16, Romain LE DISEZ a écrit :

> 1/ Patch Kohana (system/libraries/drivers/Database.php) to works well
>   with boolean (I will propose this modification upstream soon)


The ticket : http://dev.kohanaphp.com/issues/1855


------------------------------------------------------------------------------
Enter the BlackBerry Developer Challenge  
This is your chance to win up to $100,000 in prizes! For a limited time,
vendors submitting new applications to BlackBerry App World(TM) will have
the opportunity to enter the BlackBerry Developer Challenge. See full prize  
details at: http://p.sf.net/sfu/Challenge
__[ g a l l e r y - d e v e l ]_________________________

[ list info/archive --> http://gallery.sf.net/lists.php ]
[ gallery info/FAQ/download --> http://gallery.sf.net ]

[Bharat Mediratta]

I looked over:
http://github.com/rledisez/gallery3/commit/719c59e0402464a0e2b14915f6d10218ff5d4729

This seems like a reasonable change to me.  It's strictly superior to
what we have currently in terms of readability so I think you're making
your point about portability leading to improved code.  But I suspect
that's also because you've put a lot of time and care into the change
(which I considerably appreciate).

If you can write the upgrade code, then I suspect I can pull it in
as-is.  Please make sure that the unit tests continue to pass with MySQL.

If you want to go above and beyond, you can update kohana/README in the
gallery3-vendor branch with a reference to the ticket you filed, so that
we can track our change to the Kohana library when we do updates.

If you need help with the upgrader code, please let me know.

Romain LE DISEZ wrote:

> Hi all,
>
> Le mercredi 24 juin 2009 à 17:26 -0700, Bharat Mediratta a écrit :
>> How about leaving the field as a BOOLEAN and fixing all the occurrences
>> of 1/0 to true/false?  Upon some reflection, I think that's a fair
>> change for us to make in Gallery3 because it makes it clear to us that
>> we're dealing with a bool.  This is a discipline that we should probably
>> have.  If that works for you, then make that change in your repo and
>> I'll pull it into Gallery3.
>
> As I said before leaving, I was working on it. I think it's ready now. I
> pushed it in my repo. Some explications to fully understand the diff :
>
> 1/ MySQL doesn't know what is a BOOL (that's terific but it's true)
>    When creating a BOOLEAN column, MySQL create a TINYINT(1). So, the
>    value of the bool is an integer between 0 and 127. There is now way
>    to deal with that
>
> 2/ Kohana convert boolean to integer
>
> 3/ Kohana consider a BINARY(1) as a BOOLEAN (but not a TINYINT(1), can't
>    understand why...)
>
>
> So, the solution is :
>
> 1/ Patch Kohana (system/libraries/drivers/Database.php) to works well
>    with boolean (I will propose this modification upstream soon)
>
> 2/ Change :
>     - access::ALLOW   to TRUE,
>     - access::DENY    to FALSE,
>     - access::UNKNOWN to NULL
>    I also create access::INHERIT (NULL) to clarify the differences
>    between NULL in access_intent and in the cache
>
> 3/ Convert the columns (view_X, view_full_X, ...) to BINARY(1)
>
> I also changed the definition of items.view_X to be nullable. It must be
> nullable because in access::_update_access_view_cache() these columns
> are set to null (step 2).
>
>
> With this modification, it should be easiest to make it compatible with
> other RDBMS. The code is also more clear I think (I replaced all
> occurences of "null" and "0" by INHERIT/UNKNOWN and FALSE).
>
>
> I hope it is clear and complete but I'm really tired so it's probably
> not. I let you look at the code and I wait for comments. I will write
> the code for upgrader after you approved the modification.
>
> Greetings.
>
>

------------------------------------------------------------------------------
Enter the BlackBerry Developer Challenge  
This is your chance to win up to $100,000 in prizes! For a limited time,
vendors submitting new applications to BlackBerry App World(TM) will have
the opportunity to enter the BlackBerry Developer Challenge. See full prize  
details at: http://p.sf.net/sfu/Challenge
__[ g a l l e r y - d e v e l ]_________________________

[ list info/archive --> http://gallery.sf.net/lists.php ]
[ gallery info/FAQ/download --> http://gallery.sf.net ]

[Romain LE DISEZ-5]

Le lundi 20 juillet 2009 à 12:41 -0400, Bharat Mediratta a écrit :
> I looked over:
> http://github.com/rledisez/gallery3/commit/719c59e0402464a0e2b14915f6d10218ff5d4729
>
> ...
>
> If you can write the upgrade code, then I suspect I can pull it in
> as-is.  Please make sure that the unit tests continue to pass with MySQL.

I pushed the upgrade code. I checked again that unit tests pass and I
did some tests on a copy of a real installation. It works for me.

During the merge with my fork, take care that the modifications of
modules/gallery/models/item.php is not related to ACL. It just removes
an useless ORDER BY that cause problem to PgSQL.


> If you want to go above and beyond, you can update kohana/README in the
> gallery3-vendor branch with a reference to the ticket you filed, so that
> we can track our change to the Kohana library when we do updates.

If I understand well, I have to copy the files of Kohana I modified in
modified/. Then I "diff upstream/ modified/" and I check it is conform
to the patch I provided in the ticket #1855. If it is ok, I add the URL
of the ticket at the end of README and, finally, I commit and push.

Is it what I'm supposed to do ?


Greetings.


------------------------------------------------------------------------------
__[ g a l l e r y - d e v e l ]_________________________

[ list info/archive --> http://gallery.sf.net/lists.php ]
[ gallery info/FAQ/download --> http://gallery.sf.net ]

[Bharat Mediratta]

Romain LE DISEZ wrote:

> Le lundi 20 juillet 2009 à 12:41 -0400, Bharat Mediratta a écrit :
>> I looked over:
>> http://github.com/rledisez/gallery3/commit/719c59e0402464a0e2b14915f6d10218ff5d4729
>>
>> ...
>>
>> If you can write the upgrade code, then I suspect I can pull it in
>> as-is.  Please make sure that the unit tests continue to pass with MySQL.
>
> I pushed the upgrade code. I checked again that unit tests pass and I
> did some tests on a copy of a real installation. It works for me.
>
> During the merge with my fork, take care that the modifications of
> modules/gallery/models/item.php is not related to ACL. It just removes
> an useless ORDER BY that cause problem to PgSQL.

I've pushed this into the main repo.  Thanks, Romain-- this is quality
code.  I only cherrypicked the specific changes from your branch related
to this fix (216a21, 2282f9, 7c7d4c, and 350c1b) so I should not have
gotten anything extra that you've done in your branch.

>> If you want to go above and beyond, you can update kohana/README in the
>> gallery3-vendor branch with a reference to the ticket you filed, so that
>> we can track our change to the Kohana library when we do updates.
>
> If I understand well, I have to copy the files of Kohana I modified in
> modified/. Then I "diff upstream/ modified/" and I check it is conform
> to the patch I provided in the ticket #1855. If it is ok, I add the URL
> of the ticket at the end of README and, finally, I commit and push.
>
> Is it what I'm supposed to do ?

Actually in this case, all you have to do is update the README because
the actual changes will be in the gallery3 repo.  We only apply
programmatic changes in the vendor's "modified" dir.  I've gone ahead
and updated the README for you.

-Bharat

------------------------------------------------------------------------------
__[ g a l l e r y - d e v e l ]_________________________

[ list info/archive --> http://gallery.sf.net/lists.php ]
[ gallery info/FAQ/download --> http://gallery.sf.net ]

[Bharat Mediratta]

Bharat Mediratta wrote:

> Romain LE DISEZ wrote:
>> I pushed the upgrade code. I checked again that unit tests pass and I
>> did some tests on a copy of a real installation. It works for me.
>>
>> During the merge with my fork, take care that the modifications of
>> modules/gallery/models/item.php is not related to ACL. It just removes
>> an useless ORDER BY that cause problem to PgSQL.
>
> I've pushed this into the main repo.  Thanks, Romain-- this is quality
> code.  I only cherrypicked the specific changes from your branch related
> to this fix (216a21, 2282f9, 7c7d4c, and 350c1b) so I should not have
> gotten anything extra that you've done in your branch.

One problem with this is that when we run the upgrader, it's going to
change some of the default values set in install.sql.  Did you update
the install.sql file by hand?  The way to generate that file is by
running "php index.php package".  Here are some sample diffs when I run
that now:

 CREATE TABLE {access_caches} (
   `id` int(9) NOT NULL auto_increment,
   `item_id` int(9) default NULL,
-  `view_full_1` binary(1) NOT NULL default false,
-  `edit_1` binary(1) NOT NULL default false,
-  `add_1` binary(1) NOT NULL default false,
-  `view_full_2` binary(1) NOT NULL default false,
-  `edit_2` binary(1) NOT NULL default false,
-  `add_2` binary(1) NOT NULL default false,
+  `view_full_1` binary(1) NOT NULL default '0',
+  `edit_1` binary(1) NOT NULL default '0',
+  `add_1` binary(1) NOT NULL default '0',
+  `view_full_2` binary(1) NOT NULL default '0',
+  `edit_2` binary(1) NOT NULL default '0',
+  `add_2` binary(1) NOT NULL default '0',
   PRIMARY KEY  (`id`)


-INSERT INTO {access_intents} VALUES (1,1,1,1,0,0,1,1,0,0);
+INSERT INTO {access_intents} VALUES (1,1,'1','1','0','0','1','1','0','0');

Looks like it's treating the binary column as text for default values
there.  Will this be a problem for Postgres if I commit it this way?

-Bharat

------------------------------------------------------------------------------
__[ g a l l e r y - d e v e l ]_________________________

[ list info/archive --> http://gallery.sf.net/lists.php ]
[ gallery info/FAQ/download --> http://gallery.sf.net ]

[Romain LE DISEZ-5]

Le 23 juil. 09 à 20:27, Bharat Mediratta a écrit :

> Bharat Mediratta wrote:
>> Romain LE DISEZ wrote:
>>> I pushed the upgrade code. I checked again that unit tests pass  
>>> and I
>>> did some tests on a copy of a real installation. It works for me.
>>>
>>> During the merge with my fork, take care that the modifications of
>>> modules/gallery/models/item.php is not related to ACL. It just  
>>> removes
>>> an useless ORDER BY that cause problem to PgSQL.
>>
>> I've pushed this into the main repo.  Thanks, Romain-- this is  
>> quality
>> code.  I only cherrypicked the specific changes from your branch  
>> related
>> to this fix (216a21, 2282f9, 7c7d4c, and 350c1b) so I should not have
>> gotten anything extra that you've done in your branch.
>
> One problem with this is that when we run the upgrader, it's going to
> change some of the default values set in install.sql.  Did you update
> the install.sql file by hand?  The way to generate that file is by
> running "php index.php package".

Yes, I updated it with my little hands. I knew that this file was  
generated,
but I wasn't aware of this command.

> Here are some sample diffs when I run
> that now:
>
> CREATE TABLE {access_caches} (
>   `id` int(9) NOT NULL auto_increment,
>   `item_id` int(9) default NULL,
> -  `view_full_1` binary(1) NOT NULL default false,
> -  `edit_1` binary(1) NOT NULL default false,
> -  `add_1` binary(1) NOT NULL default false,
> -  `view_full_2` binary(1) NOT NULL default false,
> -  `edit_2` binary(1) NOT NULL default false,
> -  `add_2` binary(1) NOT NULL default false,
> +  `view_full_1` binary(1) NOT NULL default '0',
> +  `edit_1` binary(1) NOT NULL default '0',
> +  `add_1` binary(1) NOT NULL default '0',
> +  `view_full_2` binary(1) NOT NULL default '0',
> +  `edit_2` binary(1) NOT NULL default '0',
> +  `add_2` binary(1) NOT NULL default '0',
>   PRIMARY KEY  (`id`)
>
>
> -INSERT INTO {access_intents} VALUES (1,1,1,1,0,0,1,1,0,0);
> +INSERT INTO {access_intents} VALUES  
> (1,1,'1','1','0','0','1','1','0','0');
>
> Looks like it's treating the binary column as text for default values
> there.  Will this be a problem for Postgres if I commit it this way?
>
> -Bharat

It's fine, PgSQL has its own install.sql.

Greetings.


------------------------------------------------------------------------------
__[ g a l l e r y - d e v e l ]_________________________

[ list info/archive --> http://gallery.sf.net/lists.php ]
[ gallery info/FAQ/download --> http://gallery.sf.net ]