Re: Gallery's K3 class name/dir/file conventions (was "Capitalization")

I agree with lots of what I see here, so I'm going to address a few
specific things inline.


>  2) The standard approach isn't O(n^2) unless I'm severely
>> misunderstanding - if you try to load a class like Controller_Admin_Tags
>> it'd expand to modules/*/classes/Controller/Admin/Tags.php so it'd do N
>> file_exists calls, not N^2. It should be the same for TagEvent, etc.
>>
>
> You're right - to find TagEvent, it's O(n).  However, Gallery events are
> run by finding the event files of *all* modules, and searching for each of
> them in each directory, which is O(n^2).
>
> (Sidenote: writing a module-name-sensitive fild_file(), e.g. one that
> looks only in "tag" for "TagEvent," removes the possibility of it being
> extended by another module)
>

Ah ok I see what you mean - it's N find_file() calls across M modules to
load the event class.  And if the event handler doesn't exist, then it's a
little heavier.  Kohana really should optimize this case away with a cache,
although we're also getting a benefit from the filesystem cache.  The
second round of these file_exists() calls should never touch disk.

I agree that this *might* be bad, but if we're going to complicate the
codebase to account for it somebody has to run benchmarks first.  At this
stage, we're better off aiming for simplicity and optimizing later if and
when we find out that it's a real issue.  Before we release, we can move
stuff around as much as we want.



> I think that, upon reflection, I like the general idea of leaving them as
> normal K3 classes that follow the rules.  But, there are still two things
> that bug me:
> - Running "event" and "theme" functions seems kinda costly, and doesn't
> really get optimized the second time around.  For example, if our foo
> module doesn't have a theme file, we'll still search event module for it
> when running page_bottom() even after we couldn't find it when we ran
> page_top().
>

This is *might* turn out to be a significant performance hit but I highly
doubt it.  And if it turns out to be, it's something that could be
trivially optimized with a small cache.


> - Being opinionated for the naming of REST API resources seems important,
> but something about enforcing this everywhere else doesn't sit right.  I
> feel like we shouldn't *have* to be opinionated here.
>

Let me turn that around - why would we *not* be opinionated wherever it's
useful?  There's value in saying "Oh, the event handlers are all in
modules/foo/classes/FooEvent.php" - the simplicity in having only one way
to do things greatly reduces cognitive load.


The solution I proposed yesterday addresses these, but obviously does so at
> a cost.  Here's another possibility:
> - Put all of our special files into a separate subdir or two in classes (I
> added a "Rest" subdir, but it could be dumped into the main "G3" too).
> - Allow the files in "G3" to have any prefix they want (TagEvent,
> TAGEvent, or TagFooEvent all work).
> - Write our own routine to find and run event/theme/etc files.  It
> essentially runs a wildcard version of Kohana::find_file() to look for
> classes/G3/*Event.php, caches the result, then requires them (as needed -
> e.g. usually not all needed for Block).  This does mean that we're
> kinda/sorta implementing our own find_file() function, but after staring at
> it for awhile, I don't think it'd be too bad.  And, I think it'd be worth
> the payoff, especially considering the frequency with which we call event
> and theme functions.
>
> This means:
> - All classes stay in the classes dir and are strictly PSR-0 compliant.
> - The classes play nice with everything else in the classes directory, and
> can still override each other (e.g. class G3_FooEvent extends
> Tag_G3_TagEvent).
> - We still separate generic stuff from Gallery-specific stuff.
> - They can have the prefix TagAlbums, Tag, or PDF as devs wish without
> having to specify it or follow rules that specify it for them.
> - We can load the files ourselves using our own, optimized routine.
>

I'm a little mixed on this.  Some thoughts:
- I can't tell if I like the separate subdir or not.  Frankly, that implies
that there's something to separate from, but many modules can do everything
they need to do with just and event handler and a block, etc.
- We avoid using "G3" in the code.  This is a holdover from Gallery 2 which
had a bit of an identity crisis in the code.  It's always "Gallery" with no
version number.
- I don't like the fact that files in the G3 dir can have any prefix -
people will invariably do stupid stuff with that.  There's no need for that
flexibility, IMO
- I don't want to write our own find_file if we can avoid it - it'd be nice
if anything we did to optimize things could be pushed upstream
- If we don't care about overloading these files, we can avoid using
find_file altogether, for example:

foreach (Module::active() as $module) {
  $class = "{$module->name}Event";
  if (!class_exists($class)) {
    $event_file = sprintf("modules/%s/classes/%sEvent.php", $module->name,
ucfirst($module->name));
    if (file_exists($event_file)) {
      require_once($event_file);
    }
  }
  if (class_exists($class) && method_exists($class, $function)) {
    call_user_func_array(array($class, $function), $args);
  }
}

That's a very straightforward adaptation of what we're doing for K2.  I
don't recommend it though - not until we determine that it's an actual
performance hit to use the K3 approach.

I hear what you're saying, but I'm not swayed by the performance or opinion
arguments.  I could be swayed towards a less opinionated stance, but only
if we can find some use cases where that flexibility really adds value...

-Bharat

>

------------------------------------------------------------------------------
Everyone hates slow websites. So do we.
Make your web apps faster with AppDynamics
Download AppDynamics Lite for free today:
http://p.sf.net/sfu/appdyn_d2d_mar

__[ 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 ]