Thoughts about the Identify change in talmdal_dev

[Bharat Mediratta]

I'm reading through a diff now while I head back from Disneyland.  Here
are some stream of conciousness notes:

- user_theme.php is gone?  where did that functionality go?  Ah, that's
moved into gallery_theme.php.  ok.

- identify.php configures the driver.. will that be configured via a
hook so that we can switch via the admin interface?

- in admin_users.php, why not just call <? if (user::is_writable()): ?>
in the view instead of precaching the result?  I try to avoid passing
boolean settings into the view where possible to give the view more
flexibility.  This applies in all the various places where you do that

- Big +1 for switching all the ORM calls over to using the helpers.  IMO
it'd be nice to cherrypick those into master early to lessen the size of
your branch

- Breaking the separation from REST_Controller is good, but it really
shouldn't be done here; it's making your change very large.  Can you
stage that separately and cherrypick it up to master?

- Not sure what users::get_site_list() is yet

- Naming.  Most of the group_Core and user_Core methods call Identity
methods of the same name but I notice that group_Core::groups() calls
list_groups().  Consistency here is really important in that it'll make
it much easier to figure out the flow.  I suggest that you rename
group_Core::groups() to group_Core::list().  Please fix up similar
descrepancies (I didn't notice any but I didn't look too hard)

- I'm not 100% happy with how we pull form rules from the model.  You're
making it no worse, just commenting that this is something that I'm
still not pleased with.  I think we should just keep the rules in the
form generation code.  Thoughts?

- user_Core::cookie_locale() needs phpdoc

- Why does user_core::load_uesr() call Identity::instance() ?

- Why not just roll user:: and group:: helpers under a single identity::
helper to make naming more obvious?

- Why does Identity_Core have multiple instances?  Seems like we should
just have the active one.  Is this a copy/paste issue from the template
driver you used?

- Identity_Driver::guest() has a "@todo consider caching" which is not
relevant.  You probably need to closely review *all* phpdoc for
cut/paste artifacts

- Why is User_Definition::$hashed_password a write only field?

- User_Definition::avatar_url() doesn't seem like it's a function of the
driver to me. Is that because it was in the model?

- What's Group_Definition::uncloaked() ?

- Why does Identity_Gallery_Driver::guest() return a Gallery_User
wrapped around the ORM instead of just the ORM itself?  It seems like an
unnecessary adherence to interfaces here.


In general this looks ok to me, but I'm a little worried about the extra
overhead of having the User_Definition class.  Do we really need that?
Why don't we just have all the various user / group implementations
support the fields natively and be done with it?









------------------------------------------------------------------------------
Come build with us! The BlackBerry(R) Developer Conference in SF, CA
is the only developer event you need to attend this year. Jumpstart your
developing skills, take BlackBerry mobile applications to market and stay
ahead of the curve. Join us from November 9 - 12, 2009. Register now!
http://p.sf.net/sfu/devconference
__[ 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 ]

[Tim Almdal]

> - identify.php configures the driver.. will that be configured 
> via a
> hook so that we can switch via the admin interface?

At the moment, it is configured in the config/identity.php.  At the moment, I was trying to keep things simple.

> - in admin_users.php, why not just call <? if 
> (user::is_writable()): ?>
> in the view instead of precaching the result?  I try to 
> avoid passing
> boolean settings into the view where possible to give the view more
> flexibility.  This applies in all the various places where 
> you do that

Probably, just a style approach, I try to keep the method calls in the controller, its changable.

> - Breaking the separation from REST_Controller is good, but it really
> shouldn't be done here; it's making your change very 
> large.  Can you
> stage that separately and cherrypick it up to master?

Shouldn't be an issue, I could do that.  it was more of an issue of lets get it working, them we can fine tune the approach.

> - Naming.  Most of the group_Core and user_Core methods 
> call Identity
> methods of the same name but I notice that group_Core::groups() calls
> list_groups().  Consistency here is really important in 
> that it'll make
> it much easier to figure out the flow.  I suggest that you rename
> group_Core::groups() to group_Core::list().  Please fix up 
> similardescrepancies (I didn't notice any but I didn't look too hard)

Well can't call the method list as php compiler gets confused between the method list() and list(x, y) = array();  so I had to fudge the name, I can go back and see if the I can clean that up , but at the moment, the Identity class also has a list_user, which is the other reason I have the discrepancy.

> - I'm not 100% happy with how we pull form rules from the 
> model.  You're
> making it no worse, just commenting that this is something that I'm
> still not pleased with.  I think we should just keep the 
> rules in the
> form generation code.  Thoughts?

Problem is this. Different drivers might have different rules about what's acceptable.  The form generation is a Gallery responsibility, the acceptable values are a driver responsibility.

> - Why does user_core::load_uesr() call Identity::instance() ?

We need to call Identity::instance() early in the initialization process, because this will force the load of Identity.php, which loads all the classes, including User_Definition and the derived User class.  Without doing this, we can't load the user class from the session.  The user implementation class is part of the driver file and can't be found by the hierarchical search.

> - Why not just roll user:: and group:: helpers under a single 
> identity::helper to make naming more obvious?

I was trying to minimized changes to the existing code base.  There is a lot of code that expects user and group helper classes and I wasn't ready to make that level of change unless we decided that this is the way to go forward.

> - Why does Identity_Core have multiple instances?  Seems 
> like we should
> just have the active one.  Is this a copy/paste issue from 
> the template
> driver you used?

Probably and this applies to your other comment as well.  Some of this stuff is fairly labour intensive and until we move beyond the "prototype stage", I didn't focus any time on really cleaning up the code, i just wanted to make it work from a proof of concept.  If we go forward, I will definitely try to catch all the copy/paste artifacts before merging.

> - Not sure what users::get_site_list() is yet

Where did you see that, I tried looking in my branch on github and didn't see that 

Tim

----- Original Message -----
From: Bharat Mediratta <bharat@...>
Date: Sunday, October 11, 2009 7:56 pm
Subject: Thoughts about the Identify change in talmdal_dev
To: Tim Almdal <tnalmdal@...>
Cc: "gallery-devel@..." <gallery-devel@...>

>
> I'm reading through a diff now while I head back from
> Disneyland.  Here
> are some stream of conciousness notes:
>
> - user_theme.php is gone?  where did that functionality
> go?  Ah, that's
> moved into gallery_theme.php.  ok.
>
> - identify.php configures the driver.. will that be configured
> via a
> hook so that we can switch via the admin interface?

>
> - in admin_users.php, why not just call <? if
> (user::is_writable()): ?>
> in the view instead of precaching the result?  I try to
> avoid passing
> boolean settings into the view where possible to give the view more
> flexibility.  This applies in all the various places where
> you do that
>
> - Big +1 for switching all the ORM calls over to using the
> helpers.  IMO
> it'd be nice to cherrypick those into master early to lessen the
> size of
> your branch
>
> - Breaking the separation from REST_Controller is good, but it really
> shouldn't be done here; it's making your change very
> large.  Can you
> stage that separately and cherrypick it up to master?
>
> - Not sure what users::get_site_list() is yet
>
> - Naming.  Most of the group_Core and user_Core methods
> call Identity
> methods of the same name but I notice that group_Core::groups() calls
> list_groups().  Consistency here is really important in
> that it'll make
> it much easier to figure out the flow.  I suggest that you rename
> group_Core::groups() to group_Core::list().  Please fix up
> similardescrepancies (I didn't notice any but I didn't look too hard)
>
> - I'm not 100% happy with how we pull form rules from the
> model.  You're
> making it no worse, just commenting that this is something that I'm
> still not pleased with.  I think we should just keep the
> rules in the
> form generation code.  Thoughts?
>
> - user_Core::cookie_locale() needs phpdoc
>
> - Why does user_core::load_uesr() call Identity::instance() ?
>
> - Why not just roll user:: and group:: helpers under a single
> identity::helper to make naming more obvious?
>
> - Why does Identity_Core have multiple instances?  Seems
> like we should
> just have the active one.  Is this a copy/paste issue from
> the template
> driver you used?
>
> - Identity_Driver::guest() has a "@todo consider caching" which
> is not
> relevant.  You probably need to closely review *all* phpdoc for
> cut/paste artifacts
>
> - Why is User_Definition::$hashed_password a write only field?
>
> - User_Definition::avatar_url() doesn't seem like it's a
> function of the
> driver to me. Is that because it was in the model?
>
> - What's Group_Definition::uncloaked() ?
>
> - Why does Identity_Gallery_Driver::guest() return a Gallery_User
> wrapped around the ORM instead of just the ORM itself?  It
> seems like an
> unnecessary adherence to interfaces here.
>
>
> In general this looks ok to me, but I'm a little worried about
> the extra
> overhead of having the User_Definition class.  Do we really
> need that?
> Why don't we just have all the various user / group implementations
> support the fields natively and be done with it?
>
>
>
>
>
>
>
>
>

Website: http://www.timalmdal.com

------------------------------------------------------------------------------
Come build with us! The BlackBerry(R) Developer Conference in SF, CA
is the only developer event you need to attend this year. Jumpstart your
developing skills, take BlackBerry mobile applications to market and stay
ahead of the curve. Join us from November 9 - 12, 2009. Register now!
http://p.sf.net/sfu/devconference
__[ 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 ]

[Tim Almdal]

Forgot to answer these questions:

> - Why is User_Definition::$hashed_password a write only field?

Cause when I looked at the code, it was always converted into the password field, it was only used by g2 import and I couldn't find any place that it was accessed, just set.  Beyond that, I wasn't sure what it was for.

> 
> - User_Definition::avatar_url() doesn't seem like it's a 
> function of the
> driver to me. Is that because it was in the model?

Yes.  G3 is expecting that method and it is user related, so it was put in the driver interface as something the identity management needed to provide to G3.
> 
> - What's Group_Definition::uncloaked() ?

There was a couple places with the driver implementation I wanted to access the user object directly and  not through User_Definition.  I can't remember w/o looking at the code if I just added it to the Group_Definition because I needed it, or I it was just in case (because the User_Definition had one, the Group_Definition should get one.)  I'll check and remove it if its not required.
> 
> - Why does Identity_Gallery_Driver::guest() return a Gallery_User
> wrapped around the ORM instead of just the ORM itself?  It 
> seems like an
> unnecessary adherence to interfaces here.

Because the default implementation of Identity understands and works with an ORM object, doesn't mean that the next implementation understands or uses the Kohana ORM object.  By having the Identity abstraction apply to the users and groups as well, we insure that developers implement all the required methods that G3 needs to function.  This IMO, will reduce problems down the road for developers and us as we try to support integrations.

----- Original Message -----
From: Bharat Mediratta <bharat@...>
Date: Sunday, October 11, 2009 7:56 pm
Subject: Thoughts about the Identify change in talmdal_dev
To: Tim Almdal <tnalmdal@...>
Cc: "gallery-devel@..." <gallery-devel@...>

>
> I'm reading through a diff now while I head back from
> Disneyland.  Here
> are some stream of conciousness notes:
>
> - user_theme.php is gone?  where did that functionality
> go?  Ah, that's
> moved into gallery_theme.php.  ok.
>
> - identify.php configures the driver.. will that be configured
> via a
> hook so that we can switch via the admin interface?
>
> - in admin_users.php, why not just call <? if
> (user::is_writable()): ?>
> in the view instead of precaching the result?  I try to
> avoid passing
> boolean settings into the view where possible to give the view more
> flexibility.  This applies in all the various places where
> you do that
>
> - Big +1 for switching all the ORM calls over to using the
> helpers.  IMO
> it'd be nice to cherrypick those into master early to lessen the
> size of
> your branch
>
> - Breaking the separation from REST_Controller is good, but it really
> shouldn't be done here; it's making your change very
> large.  Can you
> stage that separately and cherrypick it up to master?
>
> - Not sure what users::get_site_list() is yet
>
> - Naming.  Most of the group_Core and user_Core methods
> call Identity
> methods of the same name but I notice that group_Core::groups() calls
> list_groups().  Consistency here is really important in
> that it'll make
> it much easier to figure out the flow.  I suggest that you rename
> group_Core::groups() to group_Core::list().  Please fix up
> similardescrepancies (I didn't notice any but I didn't look too hard)
>
> - I'm not 100% happy with how we pull form rules from the
> model.  You're
> making it no worse, just commenting that this is something that I'm
> still not pleased with.  I think we should just keep the
> rules in the
> form generation code.  Thoughts?
>
> - user_Core::cookie_locale() needs phpdoc
>
> - Why does user_core::load_uesr() call Identity::instance() ?
>
> - Why not just roll user:: and group:: helpers under a single
> identity::helper to make naming more obvious?
>
> - Why does Identity_Core have multiple instances?  Seems
> like we should
> just have the active one.  Is this a copy/paste issue from
> the template
> driver you used?
>
> - Identity_Driver::guest() has a "@todo consider caching" which
> is not
> relevant.  You probably need to closely review *all* phpdoc for
> cut/paste artifacts
>
> - Why is User_Definition::$hashed_password a write only field?
>
> - User_Definition::avatar_url() doesn't seem like it's a
> function of the
> driver to me. Is that because it was in the model?
>
> - What's Group_Definition::uncloaked() ?
>
> - Why does Identity_Gallery_Driver::guest() return a Gallery_User
> wrapped around the ORM instead of just the ORM itself?  It
> seems like an
> unnecessary adherence to interfaces here.
>
>
> In general this looks ok to me, but I'm a little worried about
> the extra
> overhead of having the User_Definition class.  Do we really
> need that?
> Why don't we just have all the various user / group implementations
> support the fields natively and be done with it?
>
>
>
>
>
>
>
>
>

Website: http://www.timalmdal.com

------------------------------------------------------------------------------
Come build with us! The BlackBerry(R) Developer Conference in SF, CA
is the only developer event you need to attend this year. Jumpstart your
developing skills, take BlackBerry mobile applications to market and stay
ahead of the curve. Join us from November 9 - 12, 2009. Register now!
http://p.sf.net/sfu/devconference
__[ 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 ]

[Tim Almdal]

> - Not sure what users::get_site_list() is yet
Do you mean gallery_block::get_site_list?  It returns the language block for the sidebar.  I will probably implement this separately.

----- Original Message -----
From: Bharat Mediratta <bharat@...>
Date: Sunday, October 11, 2009 7:56 pm
Subject: Thoughts about the Identify change in talmdal_dev
To: Tim Almdal <tnalmdal@...>
Cc: "gallery-devel@..." <gallery-devel@...>

>
> I'm reading through a diff now while I head back from
> Disneyland.  Here
> are some stream of conciousness notes:
>
> - user_theme.php is gone?  where did that functionality
> go?  Ah, that's
> moved into gallery_theme.php.  ok.
>
> - identify.php configures the driver.. will that be configured
> via a
> hook so that we can switch via the admin interface?
>
> - in admin_users.php, why not just call <? if
> (user::is_writable()): ?>
> in the view instead of precaching the result?  I try to
> avoid passing
> boolean settings into the view where possible to give the view more
> flexibility.  This applies in all the various places where
> you do that
>
> - Big +1 for switching all the ORM calls over to using the
> helpers.  IMO
> it'd be nice to cherrypick those into master early to lessen the
> size of
> your branch
>
> - Breaking the separation from REST_Controller is good, but it really
> shouldn't be done here; it's making your change very
> large.  Can you
> stage that separately and cherrypick it up to master?
>
> - Not sure what users::get_site_list() is yet
>
> - Naming.  Most of the group_Core and user_Core methods
> call Identity
> methods of the same name but I notice that group_Core::groups() calls
> list_groups().  Consistency here is really important in
> that it'll make
> it much easier to figure out the flow.  I suggest that you rename
> group_Core::groups() to group_Core::list().  Please fix up
> similardescrepancies (I didn't notice any but I didn't look too hard)
>
> - I'm not 100% happy with how we pull form rules from the
> model.  You're
> making it no worse, just commenting that this is something that I'm
> still not pleased with.  I think we should just keep the
> rules in the
> form generation code.  Thoughts?
>
> - user_Core::cookie_locale() needs phpdoc
>
> - Why does user_core::load_uesr() call Identity::instance() ?
>
> - Why not just roll user:: and group:: helpers under a single
> identity::helper to make naming more obvious?
>
> - Why does Identity_Core have multiple instances?  Seems
> like we should
> just have the active one.  Is this a copy/paste issue from
> the template
> driver you used?
>
> - Identity_Driver::guest() has a "@todo consider caching" which
> is not
> relevant.  You probably need to closely review *all* phpdoc for
> cut/paste artifacts
>
> - Why is User_Definition::$hashed_password a write only field?
>
> - User_Definition::avatar_url() doesn't seem like it's a
> function of the
> driver to me. Is that because it was in the model?
>
> - What's Group_Definition::uncloaked() ?
>
> - Why does Identity_Gallery_Driver::guest() return a Gallery_User
> wrapped around the ORM instead of just the ORM itself?  It
> seems like an
> unnecessary adherence to interfaces here.
>
>
> In general this looks ok to me, but I'm a little worried about
> the extra
> overhead of having the User_Definition class.  Do we really
> need that?
> Why don't we just have all the various user / group implementations
> support the fields natively and be done with it?
>
>
>
>
>
>
>
>
>

Website: http://www.timalmdal.com

------------------------------------------------------------------------------
Come build with us! The BlackBerry(R) Developer Conference in SF, CA
is the only developer event you need to attend this year. Jumpstart your
developing skills, take BlackBerry mobile applications to market and stay
ahead of the curve. Join us from November 9 - 12, 2009. Register now!
http://p.sf.net/sfu/devconference
__[ 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]

Tim Almdal wrote:
>> - identify.php configures the driver.. will that be configured
>> via a
>> hook so that we can switch via the admin interface?
> At the moment, it is configured in the config/identity.php.  At the
> moment, I was trying to keep things simple.

Understood.  But remember also that when the switch happens, you'll need
to take some actions.  Look at admin_ldap in modules/ldap/controllers in
bharat_dev on gallery3-contrib to see the actions I took.

http://github.com/gallery/gallery3-contrib/blob/bharat_dev/modules/ldap/controllers/admin_ldap.php

>> - in admin_users.php, why not just call <? if
>> (user::is_writable()): ?>
>> in the view instead of precaching the result?  I try to
>> avoid passing
>> boolean settings into the view where possible to give the view more
>> flexibility.  This applies in all the various places where
>> you do that
> Probably, just a style approach, I try to keep the method calls in the
> controller, its changable.

The style I'm aiming for is to provide objects to the views to give the
views flexibility.  If we consider write-ability as a function of a user
object, then it would make sense to just provide the object to the view
and ask <? if ($user->is_writeable()): ?>

But in this case, write-ability is a function of the Identity driver, so
 it seems like user::is_writeable() is attaching the function to the
wrong domain.  It should either be $user->is_writeable() or
identity::is_writeable() because it operates over the whole driver.

Either way, I still prefer to put this stuff down in the view and not
pass a bunch of booleans down from the controller.

>> - Naming.  Most of the group_Core and user_Core methods
>> call Identity
>> methods of the same name but I notice that group_Core::groups() calls
>> list_groups().  Consistency here is really important in
>> that it'll make
>> it much easier to figure out the flow.  I suggest that you rename
>> group_Core::groups() to group_Core::list().  Please fix up
>> similardescrepancies (I didn't notice any but I didn't look too hard)
> Well can't call the method list as php compiler gets confused between
> the method list() and list(x, y) = array();  so I had to fudge the name,
> I can go back and see if the I can clean that up , but at the moment,
> the Identity class also has a list_user, which is the other reason I
> have the discrepancy.

I think that if we eventually move to having an identity helper which
covers all of this stuff then that issue goes away.  So I'm ok with the
discrepancies for now (but this task isn't over until we fix up all of
those issues).

>> - I'm not 100% happy with how we pull form rules from the
>> model.  You're
>> making it no worse, just commenting that this is something that I'm
>> still not pleased with.  I think we should just keep the
>> rules in the
>> form generation code.  Thoughts?
> Problem is this. Different drivers might have different rules about
> what's acceptable.  The form generation is a Gallery responsibility, the
> acceptable values are a driver responsibility.

Yes, but I highly doubt that we're going to support having our UI allow
editing of data in another datastore.  LDAP, for instance-- you'd never
have Galley updating the directory.  The main few implementations that
we're going to see are LDAP (and flavors thereof), OpenID and BasicAuth
of some kind.  Let's take the easy way out here and make editing *only*
applicable to the Gallery store.

>> - Why does user_core::load_uesr() call Identity::instance() ?
> We need to call Identity::instance() early in the initialization
> process, because this will force the load of Identity.php, which loads
> all the classes, including User_Definition and the derived User class.
>  Without doing this, we can't load the user class from the session.  The
> user implementation class is part of the driver file and can't be found
> by the hierarchical search.

Got it.  Please add a comment explaining this so it doesn't get
"optimized" away.

>> - Why not just roll user:: and group:: helpers under a single
>> identity::helper to make naming more obvious?
> I was trying to minimized changes to the existing code base.  There is a
> lot of code that expects user and group helper classes and I wasn't
> ready to make that level of change unless we decided that this is the
> way to go forward.

Ok, this is part of the follow-on cleanup then.

>> - Why does Identity_Core have multiple instances?  Seems
>> like we should
>> just have the active one.  Is this a copy/paste issue from
>> the template
>> driver you used?
> Probably and this applies to your other comment as well.  Some of this
> stuff is fairly labour intensive and until we move beyond the "prototype
> stage", I didn't focus any time on really cleaning up the code, i just
> wanted to make it work from a proof of concept.  If we go forward, I
> will definitely try to catch all the copy/paste artifacts before merging.

Excellent.

>> - Why is User_Definition::$hashed_password a write only field?
> Cause when I looked at the code, it was always converted into the
> password field, it was only used by g2 import and I couldn't find any
> place that it was accessed, just set.  Beyond that, I wasn't sure what
> it was for.
>>
>> - User_Definition::avatar_url() doesn't seem like it's a
>> function of the
>> driver to me. Is that because it was in the model?
> Yes.  G3 is expecting that method and it is user related, so it was put
> in the driver interface as something the identity management needed to
> provide to G3.

Makes sense.  The Gallery3 driver doesn't have a specific avatar_url
because we use Gravatar, but other stores (eg: OpenID, LDAP) might.

>> - What's Group_Definition::uncloaked() ?
> There was a couple places with the driver implementation I wanted to
> access the user object directly and  not through User_Definition.  I
> can't remember w/o looking at the code if I just added it to the
> Group_Definition because I needed it, or I it was just in case (because
> the User_Definition had one, the Group_Definition should get one.)  I'll
> check and remove it if its not required.

IMO, this smacks of "dereferencing pointers".  If we have to deal with
the uncloaked object then I think we're doing something wrong.

>> - Why does Identity_Gallery_Driver::guest() return a Gallery_User
>> wrapped around the ORM instead of just the ORM itself?  It
>> seems like an
>> unnecessary adherence to interfaces here.
> Because the default implementation of Identity understands and works
> with an ORM object, doesn't mean that the next implementation
> understands or uses the Kohana ORM object.  By having the Identity
> abstraction apply to the users and groups as well, we insure that
> developers implement all the required methods that G3 needs to function.
>  This IMO, will reduce problems down the road for developers and us as
> we try to support integrations.

Ok.  I'm not 100% enthused about adding a level of abstraction that we
don't know for sure that we need.. but we can always refactor it away if
we need to.

I'm going to do another pass over your branch now.

-Bharat

------------------------------------------------------------------------------
Come build with us! The BlackBerry(R) Developer Conference in SF, CA
is the only developer event you need to attend this year. Jumpstart your
developing skills, take BlackBerry mobile applications to market and stay
ahead of the curve. Join us from November 9 - 12, 2009. Register now!
http://p.sf.net/sfu/devconference
__[ 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 ]

[Tim Almdal]

>> - What's Group_Definition::uncloaked() ?
> There was a couple places with the driver implementation I wanted to
> access the user object directly and  not through User_Definition.  I
> can't remember w/o looking at the code if I just added it to the
> Group_Definition because I needed it, or I it was just in case (because
> the User_Definition had one, the Group_Definition should get one.)  I'll
> check and remove it if its not required.

IMO, this smacks of "dereferencing pointers".  If we have to deal with
the uncloaked object then I think we're doing something wrong.  

I've changed the name to _uncloaked, adding the underscore to indicate that it should be treated as private.  And added comments to explain that it is more because PHP doesn't have the concept of friend class, internal, or package level modifiers.  I guess the other alternative is to make the actual internal user/group object public.  We only need it when we call ORM::add() when we are adding a user to a group.

I'll bag the writable variable in the views as well

> Got it.  Please add a comment explaining this so it doesn't get
> "optimized" away.

k, consider it done, thought i had but must have lost it somewhere.

----- Original Message -----
From: Bharat Mediratta <bharat@...>
Date: Wednesday, October 14, 2009 11:58 am
Subject: Re: Thoughts about the Identify change in talmdal_dev
To: Tim Almdal <tnalmdal@...>
Cc: "gallery-devel@..." <gallery-devel@...>

> Tim Almdal wrote:
> >> - identify.php configures the driver.. will that be
> configured
> >> via a
> >> hook so that we can switch via the admin interface?
> > At the moment, it is configured in the
> config/identity.php.  At the
> > moment, I was trying to keep things simple.
>
> Understood.  But remember also that when the switch
> happens, you'll need
> to take some actions.  Look at admin_ldap in
> modules/ldap/controllers in
> bharat_dev on gallery3-contrib to see the actions I took.
>
> http://github.com/gallery/gallery3-
> contrib/blob/bharat_dev/modules/ldap/controllers/admin_ldap.php
> >> - in admin_users.php, why not just call <? if
> >> (user::is_writable()): ?>
> >> in the view instead of precaching the result?  I try to
> >> avoid passing
> >> boolean settings into the view where possible to give the
> view more
> >> flexibility.  This applies in all the various places
> where
> >> you do that
> > Probably, just a style approach, I try to keep the method
> calls in the
> > controller, its changable.
>
> The style I'm aiming for is to provide objects to the views to
> give the
> views flexibility.  If we consider write-ability as a
> function of a user
> object, then it would make sense to just provide the object to
> the view
> and ask <? if ($user->is_writeable()): ?>
>
> But in this case, write-ability is a function of the Identity
> driver, so
>  it seems like user::is_writeable() is attaching the
> function to the
> wrong domain.  It should either be $user->is_writeable() or
> identity::is_writeable() because it operates over the whole driver.
>
> Either way, I still prefer to put this stuff down in the view
> and not
> pass a bunch of booleans down from the controller.
>
> >> - Naming.  Most of the group_Core and user_Core methods
> >> call Identity
> >> methods of the same name but I notice that
> group_Core::groups() calls
> >> list_groups().  Consistency here is really important in
> >> that it'll make
> >> it much easier to figure out the flow.  I suggest that
> you rename
> >> group_Core::groups() to group_Core::list().  Please fix
> up
> >> similardescrepancies (I didn't notice any but I didn't look
> too hard)
> > Well can't call the method list as php compiler gets confused
> between> the method list() and list(x, y) = array();  so I
> had to fudge the name,
> > I can go back and see if the I can clean that up , but at the
> moment,> the Identity class also has a list_user, which is the
> other reason I
> > have the discrepancy.
>
> I think that if we eventually move to having an identity helper which
> covers all of this stuff then that issue goes away.  So I'm
> ok with the
> discrepancies for now (but this task isn't over until we fix up
> all of
> those issues).
>
> >> - I'm not 100% happy with how we pull form rules from the
> >> model.  You're
> >> making it no worse, just commenting that this is something
> that I'm
> >> still not pleased with.  I think we should just keep the
> >> rules in the
> >> form generation code.  Thoughts?
> > Problem is this. Different drivers might have different rules about
> > what's acceptable.  The form generation is a Gallery
> responsibility, the
> > acceptable values are a driver responsibility.
>
> Yes, but I highly doubt that we're going to support having our
> UI allow
> editing of data in another datastore.  LDAP, for instance--
> you'd never
> have Galley updating the directory.  The main few
> implementations that
> we're going to see are LDAP (and flavors thereof), OpenID and
> BasicAuthof some kind.  Let's take the easy way out here
> and make editing *only*
> applicable to the Gallery store.
>
> >> - Why does user_core::load_uesr() call Identity::instance() ?
> > We need to call Identity::instance() early in the initialization
> > process, because this will force the load of Identity.php,
> which loads
> > all the classes, including User_Definition and the derived
> User class.
> >  Without doing this, we can't load the user class from
> the session.  The
> > user implementation class is part of the driver file and can't
> be found
> > by the hierarchical search.
>
> Got it.  Please add a comment explaining this so it doesn't get
> "optimized" away.
>
> >> - Why not just roll user:: and group:: helpers under a single
> >> identity::helper to make naming more obvious?
> > I was trying to minimized changes to the existing code
> base.  There is a
> > lot of code that expects user and group helper classes and I wasn't
> > ready to make that level of change unless we decided that this
> is the
> > way to go forward.
>
> Ok, this is part of the follow-on cleanup then.
>
> >> - Why does Identity_Core have multiple instances?  Seems
> >> like we should
> >> just have the active one.  Is this a copy/paste issue
> from
> >> the template
> >> driver you used?
> > Probably and this applies to your other comment as well. 
> Some of this
> > stuff is fairly labour intensive and until we move beyond the
> "prototype> stage", I didn't focus any time on really cleaning
> up the code, i just
> > wanted to make it work from a proof of concept.  If we go
> forward, I
> > will definitely try to catch all the copy/paste artifacts
> before merging.
>
> Excellent.
>
> >> - Why is User_Definition::$hashed_password a write only field?
> > Cause when I looked at the code, it was always converted into the
> > password field, it was only used by g2 import and I couldn't
> find any
> > place that it was accessed, just set.  Beyond that, I
> wasn't sure what
> > it was for.
> >>
> >> - User_Definition::avatar_url() doesn't seem like it's a
> >> function of the
> >> driver to me. Is that because it was in the model?
> > Yes.  G3 is expecting that method and it is user related,
> so it was put
> > in the driver interface as something the identity management
> needed to
> > provide to G3.
>
> Makes sense.  The Gallery3 driver doesn't have a specific
> avatar_urlbecause we use Gravatar, but other stores (eg: OpenID,
> LDAP) might.
>
> >> - What's Group_Definition::uncloaked() ?
> > There was a couple places with the driver implementation I
> wanted to
> > access the user object directly and  not through
> User_Definition.  I
> > can't remember w/o looking at the code if I just added it to the
> > Group_Definition because I needed it, or I it was just in case
> (because> the User_Definition had one, the Group_Definition
> should get one.)  I'll
> > check and remove it if its not required.
>
> IMO, this smacks of "dereferencing pointers".  If we have
> to deal with
> the uncloaked object then I think we're doing something wrong.
>
> >> - Why does Identity_Gallery_Driver::guest() return a Gallery_User
> >> wrapped around the ORM instead of just the ORM itself?  It
> >> seems like an
> >> unnecessary adherence to interfaces here.
> > Because the default implementation of Identity understands and works
> > with an ORM object, doesn't mean that the next implementation
> > understands or uses the Kohana ORM object.  By having the
> Identity> abstraction apply to the users and groups as well, we
> insure that
> > developers implement all the required methods that G3 needs to
> function.>  This IMO, will reduce problems down the road
> for developers and us as
> > we try to support integrations.
>
> Ok.  I'm not 100% enthused about adding a level of
> abstraction that we
> don't know for sure that we need.. but we can always refactor it
> away if
> we need to.
>
> I'm going to do another pass over your branch now.
>
> -Bharat
>

Website: http://www.timalmdal.com

------------------------------------------------------------------------------
Come build with us! The BlackBerry(R) Developer Conference in SF, CA
is the only developer event you need to attend this year. Jumpstart your
developing skills, take BlackBerry mobile applications to market and stay
ahead of the curve. Join us from November 9 - 12, 2009. Register now!
http://p.sf.net/sfu/devconference
__[ 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 ]