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

Hey Bharat,

Likewise, responding inline (and snipping a bit)...

On 23 March 2013 00:58, Bharat Mediratta <[email protected]> wrote:

>
> 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.
>

Hmm, good point.  If we're going to replace something, we should benchmark
first to prove it's worth the trouble.  That's a good argument for making
our system work as-is with Kohana's built-in functions.

Speaking of good points, I went back and looked at Kohana::find_file()
again.  I seemed to be under the impression that it only cached *found*
paths, but upon second read, that doesn't appear to be true.  That's yet
another reason why working around it probably wouldn't make a huge
difference.  It turns out the authors of Kohana have spent more time than I
have thinking about this... :-)


- 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.
>

If we're sticking with Kohana's functions and not making a wildcard-savvy
loader, then this conversation is kinda moot, but for fun...

<pseudo-philosophical tangent>
I feel like there are three different cases here:
1. Not require a rule. (make the wildcard-savvy loader)
2. Require a rule. (be opinionated and enforce one way to do it)
3. Require a rule and make you define it. (extra line in module.info)

While there are certainly arguments why 2 is better than 3, in the general
case, it seems like 1 could often be better than 2, no?
</pseudo-philosophical tangent>

 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.
>

Personally, I do.  To me, it has a similar significance to Controller or
Model, in the sense that the names of their functions have special meanings
outside of the class itself.  This is not the case, for example, for the
Tag.php helper, which is why it shouldn't be in any subdir.



> - 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.
>

Ah, gotcha.  We can find something else.  I picked it mainly because it was
short :-)



> - 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
>

These disappear if we just use the Kohana loaders, as they don't allow
wildcards.



>  - 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.
>

Aye, I agree.  In fact, even simply keeping a local list of which modules
do and don't have a certain type of file (Event, Theme, etc) would be
largely duplicating what find_file() already does.


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...
>

I think by now I'm convinced of all points you just made, with the sole
exception being the separate directory for Gallery-specific stuff.  I think
it gives a meaningful distinction between it and the other stuff.  Also,
it'd give us the added benefit of being able to change our mind at a later
date should someone decide to build an even-more-optimized,
wildcard-capable loader.

On a semi-related note, how do we feel about underscores in "views" names?
 I like the idea of adding a teeny bit of hierarchy like you did with the
Tag example (certainly no more than 1 level), and that removes a decent
number of them, but not all.  Example: is
/modules/gallery/views/admin/advanced_settings.php fine?  My vote is yes,
personally...

Take care,
Shad



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