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

Hey Bharat et alia,

After sleeping on it, looking at Kohana::find_file() and
Kohana::auto_load(), and thinking a bit more, I *think* I'm starting
to have a clearer idea of what to do.  What's less clear to me is how
to try and organize this email thread in a meaningful way, but let's
give it a shot :-).

Alright, it seems that we're really addressing two different questions here:
1. What should we do about class and file/dir names in general?
2. What should we do about class and file/dir names for
Gallery-overloaded classes (e.g. tag_event)?

Summary of where I personally stand right now:
1. I think I've figured out a pretty nice proposal and implementation,
and feel pretty sure of it.
2. My thoughts here are getting better, but not yet as refined as #1.

tl;dr: Sorry, no one-line summary for this one :-).  I used a bunch of
headings so hopefully it's easier to skim through, though.

----------

1. CLASS AND FILE/DIR NAMES IN GENERAL

Our starting point here is the K3 strict conventions, then trying to
figure out what we want to do and/or not do about them.  We can break
this up into several subquestions:
1a. What's annoying about the K3 strict conventions?
1b. How much overloading do we really need?
1c. What are the nuts and bolts of Kohana's auto-loading and how can
we tweak it?
1d. What's our end solution?


1a. K3 STRICT CONVENTION ANNOYANCES

I think the big part we both agree on here is doubling our file count.
 It adds a bunch of noise to our distribution package, our repo, and
to the learning curve of devs using Notepad and not Netbeans to get
one feature implemented.

Additionally, you bring up hierarchy.  This I see as less of a
concern, as I think what it really boils down to is adding one subdir
while still keeping all the code in one place.  In other words, we're
not asking devs to look in both modules/foo/classes *and*
modules/foo/classes/Foo, because they do all of their work in the
latter and leave the former totally empty.

1b. ACTUAL OVERLOADING REQUIREMENTS

While Kohana is in principle designed for near-infinite levels of
overloading, I think we both agree this isn't entirely necessary.  I'd
argue that four would be *more* than sufficient for us.  In order from
least to most significant:
1. Kohana
2. Gallery
3. Module
4. Theme

In fact, Kohana has more-or-less already followed this convention: all
classes in system, cache, database, and orm use the same Kohana
subdirectory and class prefix.  Isn't that convenient?

1c. KOHANA'S AUTO-LOADING

Kohana::auto_load() is a relatively thin and simple function.  It
parses PHP's auto-load request, then calls Kohana::find_file() which
does all the dirty work of traversing module directories, caching
paths, etc.  Kohana::find_file() is also more generic in the sense
that it can look for files in view, vendor, etc.  Example:
Kohana::auto_load("Foo_View") => Kohana::find_file("classes",
"Foo_View").

Trying to hack into find_file() sounds really messy, but replacing the
auto_load() function seems relatively clean and straightforward.  We
can do it in our bootstrap, still have it call Kohana::find_file(),
and leave the system directory unmodified.  I like this approach.

The only drawback to this approach is that, since auto_load() by
design has no idea about our module list, it makes it difficult to
implement the near-infinite levels of overloading without all of the
doubled files everywhere.  However, if we restrict ourselves to a
fixed list of four levels, this problem disappears and the
implementation becomes rather elegant.

1d.  END SOLUTION

Alright, so based on this, here's my new proposal:
1. Class code goes in modules/[module]/classes/[Layer] or
themes/[theme]/classes/[Layer], where Layer is Kohana, Gallery,
Module, or Theme.
2. The relationship between a class name and its dir/file names
maintains strict K3 conventions.  So,
modules/foo/classes/Module/Foo.php has a class called Module_Foo.
3. We write our own auto_load().  If the searched-for class starts
with "Kohana_", "Gallery_", "Module_", or "Theme_", then call
find_file("Foo", "classes") and require($path) exactly as before with
no changes.  If not, search "classes/Theme", "classes/Module",
"classes/Gallery", and "classes/Kohana" in that order until something
is found.  If found, do an eval("class Foo extends [Layer]_Foo {}"),
effectively implementing all of the useless doubled files on the fly.
This eval() call is similar to the K2 implementation of the _Core
suffix.
4. No doubled files - hooray!  In fact, I'd go so far as to propose
that we strip the doubled files from the Kohana code to keep our
package small, consistent, and easier to browse (note that we already
have a precedent of trimming vendor directories...).

Some sidenotes on this proposal:
1. This does not break any K3 naming conventions, and does not
introduce any new hypothetical edge case name conflicts.
2. In principle, a module/theme could implement code in more than one
layer.  This might actually be nice for complicated themes (e.g.
Greydragon currently requires a module that goes with its theme).
3. Since we search for prefixes *with* the underscore/slash, we avoid
collisions with classes that have the same name (e.g. the Gallery and
Module helpers).
4. Kohana::auto_load() *does* actually implement namespaces, although
many Kohana sites I've perused seem to indicate it's a
less-than-mature implementation.  Based on this, and the fact that
it's another complicated thing for devs to understand, I vote that we
continue to not use them.

I drafted up what our auto_load() would look like:

  public static function auto_load($class, $directory="classes") {
    // This array definition should probably go somewhere else...
    $layers = array("Theme", "Module", "Gallery", "Kohana");

    $file = str_replace("_", DIRECTORY_SEPARATOR, $class);

    if (($directory != "classes") || (in_array(strstr($file,
DIRECTORY_SEPARATOR, true)), $layers) {
      // Either not in classes or we have one of the layers as a
prefix - search for exact match.
      // This is the same as Kohana::auto_load().
      if ($path = Kohana::find_file($directory, $file)) {
        // Found - load it and return that class is found.
        require $path;
        return true;
      }
    } else {
      // Search for a match with one of the layers added as a prefix,
then extend it.  This
      // essentially implements all of the doubled files typically
required by Kohana 3 on the fly.
      foreach ($layers as $layer) {
        if ($path = Kohana::find_file($directory, $layer .
DIRECTORY_SEPARATOR . $file)) {
          // Found - load it.
          require $path;

          // Extend to class without $layer and add "abstract" if needed.
          $base_class = new ReflectionClass("{$layer}_$class");
          $extension = $base_class->isAbstract() ? "abstract " : "";
          $extension .= "class $class extends {$layer}_$class {}";
          eval($extension);

          // Return that class is found.
          return true;
        }
      }
    }

    // Nothing found.
    return false;
  }

The more I think about this solution, the more I really like it...

----------

2. CLASS AND FILE/DIR NAMES FOR GALLERY-OVERLOADED STUFF

It seems that we should pick one of two basic approaches:
- Uniquify the names for each module/theme, then let them fall into
the general case for file/dir/class naming structure. (e.g. tag's
events are in "TagEvent" or "EventTag").
- Keep the names generic, but make them a special case and modify the
auto_load to be aware of this (e.g. tag's events are in "Event").

My vote is for the first approach - it seems easier for us to
implement, looks more robust, requires fewer exceptions and special
cases, and isn't too different from the G3.0.x precedent.  The second
approach would require a bunch of special things and exceptions to
rules to avoid collisions, which sounds messy.  Judging by your last
email, it sounds like you're probably on the same page with me here
:-).

So, the remaining questions are:
2a. Should we group them together in a subdirectory?
2b. Where is it defined that tag is called Tag, pdf is called PDF, and
tag albums is called TagAlbums?  Or, should we avoid having to define
this?
2c. Do we need to make it possible to overload them?
2d. Should the name be big-endian or little-endian?

2a. SUBDIRECTORY GROUPING

On the plus side this might be a nice grouping; on the minus side it
adds a piece of hierarchy.  For the examples below, I chose "G3" as a
potential subdirectory name.

Example: we want to make a two-file module foo.  We need:
  /modules/foo/module.info
And one of something like:
  /modules/foo/classes/Module/EventFoo.php
  /modules/foo/classes/Module/G3/EventFoo.php

Alone, neither of these options seem overwhelmingly more appealing
than the other to me.  However, I wonder if choosing a subdirectory to
group them in might help with...

2b. DEFINING OR NOT DEFINING THE MODULE'S CLASS PREFIX

If we have to define it, a logical place seems to be the module.info
file.  This doesn't seem too bad, and means we wouldn't have to alter
any other auto-loading code.  It also might have some other hidden
advantages (e.g. something like the current $theme->url()
implementation).  However, I wonder how hard it would be to avoid
needing this in the first place.

One way to avoid it is to rename the module directories with the
correct capitalization scheme.  Then, module directory = class prefix.
 Our example above would be /modules/Foo/classes/Module/EventFoo.php.
The only inconsistency here is that, with the exception of things in
"classes", all other directory names are lowercase and are free to use
underscores if desired.  So while it'd work, I think it's a bit
awkward.

Another way to avoid it is to use a subdirectory.  Instead of looking
specifically for classes/Module/EventFoo.php, we just look for
classes/Module/G3/Event*.php.  I haven't entirely thought through the
nuts'n'bolts of this implementation (i.e. how we search for a wildcard
yet maintain an imposed module/theme ordering), but in principle it
should be able to work while keeping strict K3 naming convention
compliance.

2c. TO OVERLOAD OR NOT TO OVERLOAD

>From a functional requirements standpoint, I think the answer is no:
we should never *need* to overload the event class of a module.

However, I think actually enforcing that isn't a good idea.  This
would make them follow a different naming convention, break strict K3
compliance, etc.  I'm personally a fan of letting them fall into the
same general case as everything else, and think it makes easier both
on us and module devs.  It also keeps all the code in one place, which
is nice.

2d. BIG/LITTLE-ENDIAN

>From an implementation standpoint, this is no big deal: it is equally
as easy to have "EventFoo" as it is "FooEvent".  I think it's purely a
question of aesthetics.

Pros of little-endian:
- Similar to standard English adjective/noun order (although there's a
treasure trove of adverbs abound, adjectives aplenty, and languages
galore that don't do this... and yes, I'm quite proud of myself for
that self-referential statement ;-).
- If we choose not to have a subdirectory to group them in, alpha
order still groups them together in the directory listing

Pros of big-endian:
- Similar to the fact that subdirectories go first in class names
(e.g. "Module_EventFoo", "Module_Controller_Admin_Foo")

My vote is big-endian, but my opinion isn't very strong here.  If you
feel strongly otherwise, that works too.

----------

Alright, I *think* I've touched on all the points in the previous
email chain.  And if not, I've left it attached below anyway.

Thoughts?

Thanks for reading!
Shad


On 19 March 2013 01:26, Bharat Mediratta <[email protected]> wrote:
>
> On Mon, Mar 18, 2013 at 3:54 PM, Shad Laws <shad-xpYdmXCiSuZWk0Htik3J/[email protected]> wrote:
>>
>> Thanks for the great reply, too!  You make some good points, and I should
>> probably extract myself from K3-land and sleep on it before figuring out an
>> equally-good reply, but I suspect the overall tone will be this: while I
>> certainly agree that having fewer (relatively meaningless) files and a
>> simpler structure for devs to understand is nice, I worry that this could be
>> a slippery slope.  For example, why not just reimplement K2's autoloader and
>> get total backward compatibility?
>
> Good point - let's do it!  Haha, kidding.  I want to be *similar* to Kohana
> 3, but it doesn't have to be exact.  I think that it's a minor and
> human-understandable change to consider the module name as part of the auto
> load path.
>>
>> One off-the-cuff, not-totally-thought-through idea: if the major issue is
>> (understandably) the doubling of files and the noise it brings to our repo
>> and to devs trying to get one feature made, what if we just implemented that
>> in the autoloader?  This is similar to what K2 did (used eval to extend the
>> _Core, etc).  We could keep the K3-compatible structure and naming, but
>> replace the empty files with just enough magic to keep everything still
>> looking more-or-less like K3.
>
> We could, but then we'd still have the problem that we're adding an extra
> level of depth. For example, then all of the tag classes would be in:
>
>   modules/tag/classes/Tag/Event.php
>   modules/tag/classes/Tag/Installer.php
>   modules/tag/classes/Tag/Task.php
>   ...
>
> modules/tag/classes would be largely empty, but the classes/Tag subdir has
> to exist so that Tag_Event is consistent, and then our magic autoloader
> would be creating modules/tag/classes/Event.php to extend Tag_Event for us.
> That magic would be even more confusing since there's no obvious way to
> identify that it's happening.  Whereas with what I'm proposing it'd be:
>
>   modules/tag/classes/Event.php
>   modules/tag/classes/Installer.php
>   modules/tag/classes/Task.php
>   ...
>
> And if you're looking for Tag_Event then you're looking in
> modules/tag/classes/Event which is a little bit magic, but is also a fairly
> simple rule.
>
> My solution is definitely unorthodox.  But I'm not convinced that the extra
> file-level complexity is worth the code duplication.  Who knows, we may be
> able to convince Woody to take it upstream...
>
>>
>> Take care,
>> Shad
>>
>> Sent from my Swiss Army Phone
>>
>> On Mar 18, 2013 11:28 PM, "Bharat Mediratta" <[email protected]> wrote:
>>>
>>>
>>> Once again, great writeup.  Let me make some counter-points below.
>>>
>>> On Mon, Mar 18, 2013 at 11:24 AM, Shad Laws <shad-xpYdmXCiSuZWk0Htik3J/[email protected]> wrote:
>>>>
>>>> Re: the naming of the gallery-imposed helpers, I like the idea of
>>>> flattening the hierarchy a bit and using the now-unique filenames as a
>>>> reason why we don't need the "Module" name stuck in there.  However,
>>>> in sync with K3-style, I think it'd be better to be "bigendian-like"
>>>> with our filenames and use prefixes rather than suffixes to specify
>>>> type.  Here's my slightly-changed counter-proposal:
>>>>   class [Module]_[Type][Method] {
>>>>     // do something
>>>>   }
>>>> then transparently extend it with:
>>>>   class [Type][Module] extends [Module]_[Type][Module] {}
>>>>
>>>> So:
>>>>   modules/tag/classes/Tag/EventTag.php
>>>> has:
>>>>   class Tag_EventTag { // cut'n'paste from current tag_event.php }
>>>> and is extended by:
>>>>   modules/tag/classes/EventTag.php
>>>> which has:
>>>>   class EventTag extends Tag_EventTag {}
>>>
>>>
>>> Can you give me an example of the big-endianness in K3?  Even if K3 has a
>>> different style, I'm a big believer that class names should be human
>>> understandable.  "It's an EventTag" makes a lot less sense to me than "it's
>>> a TagEvent".  So I'm still leaning towards:
>>>
>>> modules/tag/classes/TagEvent.php:
>>> class TagEvent { ... }
>>>
>>> In a perfect world I'd like to even drop the "Tag" bit but since we lack
>>> namespacing, that's not possible.  Came to that same conclusion with K2,
>>> which is why it's "tag_event" there, too :-)
>>>
>>> I also think it's too hierarchical which leads me to the next point...
>>>
>>>>
>>>> Re: to overload or not to overload, that is the question.  Hamlet
>>>> aside, I've spent a decent amount of time now acquainting myself with
>>>> this stuff and staring at one-too-many K2 and K3 directory trees, and
>>>> my feeling is this: allow overloading of 100% of classes.
>>>>
>>>> My reason is this: the list of "rules" for module devs to follow is
>>>> short and has zero exceptions.  In fact, I'd argue that it's easier
>>>> than K2 and it's MY_, _Core, _Driver, uppercase
>>>> controller/library/model and lowercase helper, etc.  It's a bummer
>>>> that we double the file count, but the extra files can be 100%
>>>> systematically generated... and in fact I'm already working on a
>>>> script to do it for us.
>>>
>>>
>>> I agree that K2's approach is sucky.  No argument there.  The interesting
>>> thing about K3 is that they removed a lot of the overloading magic - all you
>>> have to do is understand the order of the cascading filesystem and the rest
>>> you can get by following the code.
>>>
>>> But the problem is that most module devs don't want to understand this.
>>> With G3 we're aiming to make it really simple for a dev to write something
>>> useful.  With G3.0.x you can write a useful module with two files, the
>>> module.info and an _event.php file.  Making a class overloadable adds yet
>>> another file to the mix and the file is merely an abstraction point - it
>>> adds zero value to the module dev.  It only adds value to future module
>>> devs.  So from a "I want to get this feature done" perspective it's just
>>> noise.  And the cumulative effect of that noise will be another 70+ files
>>> (guessing based on your other comment) in our codebase whose filenames are
>>> very similar to the files that we really care about - the ones that actually
>>> contain the code.
>>>
>>> That might still be worthwhile if people actually overloaded stuff that
>>> often - but the truth is that they really don't.
>>>
>>> $ cd gallery3-contrib
>>> $ find 3.0/modules/ -name 'MY_*' | awk -F/ '{print $NF}' | sort | uniq -c
>>> maste
>>>       1 MY_Form_Input.php
>>>       2 MY_Item_Model.php
>>>       1 MY_ORM_MPTT.php
>>>       1 MY_Theme_View.php
>>>       2 MY_access.php
>>>       1 MY_embedlinks_block.php
>>>       1 MY_embedlinks_theme.php
>>>       1 MY_gallery_graphics.php
>>>       6 MY_item.php
>>>       1 MY_search.php
>>>       1 MY_url.php
>>>
>>> We don't do that much overloading.  Some of the overloading we do is
>>> probably quite dangerous (ORM_MPTT, Item_Model, access).  It's a bad smell
>>> that we have to do this in the first place.
>>>
>>> Finally, the fact that we can systematically generate these helper files
>>> with a script is a sign that we're headed in the wrong direction towards
>>> more boilerplate instead of less.  We want to bridge that line between too
>>> much unnecessary code (boilerplate) and too little (magic).  I think the way
>>> to do that is to be very careful about what type of overloading we allow.
>>>
>>>>
>>>> My first sketch of "the rules":
>>>>
>>>> 1. Put all "real" code in modules/[module]/classes/[Module].  All file
>>>> and directory names should be capitalized and have no underscores.
>>>>
>>>> 2. The class name is a direct "/" => "_" search-and-replace job with
>>>> the path in classes (e.g.
>>>> modules/tag/classes/Gallery/Controllers/Albums.php is
>>>> Gallery_Controllers_Albums).
>>>>
>>>> 3. If you want to transparently extend something (K2's MY_
>>>> functionality), do it directly with the name of the other "real" class
>>>> from the base module (e.g. modules/gallery/classes/Gallery/View.php
>>>> has "class Gallery_View extends Kohana_View").
>>>>
>>>> 4. If you want to extend something with a new name, do it with the
>>>> name of the other already-extended class (e.g.
>>>> modules/gallery/classes/Gallery/Controller/Albums.php has "class
>>>> Gallery_Controller_Albums extends Controller_Items").
>>>>
>>>> 5. For every file you have in modules/[module]/classes/[Module], make
>>>> one of the same name and path in modules/[module]/classes that has
>>>> "class [Something] extends [Module]_[Something]".  Or, run the script
>>>> I'm working on to do it for you.
>>>>
>>>> This makes it easy to know that 100% of "real" code is in the [Module]
>>>> subdirectory.  If we only allow some stuff to be overloaded and not
>>>> others, we break this.  Based on my (limited) experience, this
>>>> actually makes it harder to browse code.  It'd also make it harder to
>>>> auto-generate (and build unit tests for, of course) the
>>>> transparent-extension-enabling files.
>>>>
>>>
>>> How about we try going a little bit deeper here and we assume that the
>>> name of the module is accounted for by the class loader.  How about these
>>> rules:
>>>
>>> 1) Put all code into modules/[MODULE]/classes/[TYPE].  eg, tag_event =>
>>> modules/tag/classes/Event.php containing "class Tag_Event"
>>>
>>> 2) To access a class like X_Y, the autoloader looks in
>>> modules/X/classes/Y.php which allows us to incorporate the class name.
>>>
>>> 3) The class search order for X_Y_Z would be:
>>> modules/X/classes/Y/Z
>>> modules/classes/X/Y/Z
>>>
>>> This allows the following:
>>> - our directory structure is very flat, there's only one place for the
>>> code
>>> - each module can provide its own implementation, for example you'd
>>> write:
>>>
>>>     modules/gallery/classes/Controller/Albums.php:
>>>     class Gallery_Controller_Albums {}
>>>
>>>   then if you want to overload it in the tag module you'd do:
>>>
>>>     modules/tag/classes/Controller/Albums.php:
>>>     class Tag_Controller_Albums extends Gallery_Controller_Albums {}
>>>
>>>   and when you do "new Controller_Albums" it'll pull
>>> Tag_Controller_Albums first, which will trigger the equivalent of "new
>>> Gallery_Controller_Albums" which will pull the rest.
>>>
>>> Pros
>>> - A lot less files!
>>> - Easier for devs to understand
>>> - I *think* that it's backwards compatible with the existing Kohana
>>> modules that we've already integrated
>>>
>>> Cons:
>>> - it's us, doing our own thing, again.  sigh
>>> - we have to extend the class loader.  Is that hard in this case?  dunno
>>>
>>>
>>> thoughts?
>>> -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 ]