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

Hey Bharat,

Thanks for the detailed analysis.  I love the creativity - this is not a
> direction I would have come up with, but it makes a lot of sense.  I'm not
> sold on it, though because it adds some inconsistencies:
>

Thanks!  After writing it up and then sleeping on it, I'm not quite sold on
it either.  You know how it is sometimes when you need to flesh out a
prototype a bit before realizing it isn't as great as you originally
thought?  :-)

*But,* it did get me thinking in some interesting directions, and I wonder
if I can't pull some of that back into our more normal case.


> 1) Having a new/separate location for PHP classes doesn't make a lot of
> sense unless you understand the internals of Kohana::find_file, etc.
>

True.  One could argue "they're a bit different"... but one could also
argue "so what?!" :-)


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

3) Capitalization is now different for the files under modules/*/gallery
> (we could change that of course)
>

Furthermore, we can't even say "all classes are capitalized, all others
not" like the rest of Kohana.  Same as #1 - we probably shouldn't do it.

 4) Files under modules/*/gallery/rest have underscores in them, which has
> a significant meaning now in K3 so that'd be confusing
>

Yah, true.  While the media and views do, too, they aren't classes.  Same
as #3 and #4 - we probably shouldn't do that.


> I think your approach is creative, but I think it breaks too many of the
> existing conventions.  You convinced me with your original argument - let's
> be consistent and have simple rules.
>

Sounds good - let's keep these classes in the classes directory and,
accordingly, follow the rules of the other stuff there.


> Regarding the case sensitivity, I agree with you that it'd be nicer to
> have lowercase module names.  Mapping those to filenames is pretty
> straightforward and we don't have to employ any fallbacks - remember, in an
> opinionated system you're supposed to have opinions!  With creative use of
> ucfirst and split we can do simple transforms on module names into expected
> filenames.  Some examples:
>
> foo -> Foo -> modules/foo/classes/FooEvent.class
> foo-bar -> FooBar -> modules/foo-bar/classes/FooBarEvent.class
>
> That doesn't seem too complicated.
>
> I propose that we *try* to do it the K3 way first.  If we hit too many
> problems, I'm open to adding a new convention, but I'd rather do that as a
> last resort to keep things simple and consistent.
>
> What do you think?
>

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

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.

It'd look something like this:

tag
|-- classes
|   |-- Controller
|   |   |-- Admin
|   |   |   `-- Tags.php
|   |   |-- Tag.php
|   |   |-- TagName.php
|   |   `-- Tags.php
|   |-- G3                      (I chose G3 to avoid confusion with Gallery)
|   |   |-- Rest
|   |   |   |-- ItemTags.php    (this is strictly translated as the
"item_tags" REST resource)
|   |   |   |-- Tag.php
|   |   |   |-- Tags.php
|   |   |   |-- TagItem.php
|   |   |   `-- TagItems.php
|   |   |-- TagBlock.php        (the "Tag" prefix must be unique, but has
no other requirements)
|   |   |-- TagEvent.php
|   |   |-- TagInstaller.php
|   |   |-- TagRss.php
|   |   |-- TagTask.php
|   |   `-- TagTheme.php
|   |-- Model
|   |   `-- Tag.php
|   |-- Tag
|   |   |-- Controller
|   |   |   |-- Admin
|   |   |   |   `-- Tags.php
|   |   |   |-- Tag.php
|   |   |   |-- TagName.php
|   |   |   `-- Tags.php
|   |   |-- G3
|   |   |   |-- Rest
|   |   |   |   |-- ItemTags.php
|   |   |   |   |-- Tag.php
|   |   |   |   |-- Tags.php
|   |   |   |   |-- TagItem.php
|   |   |   |   `-- TagItems.php
|   |   |   |-- TagBlock.php
|   |   |   |-- TagEvent.php
|   |   |   |-- TagInstaller.php
|   |   |   |-- TagRss.php
|   |   |   |-- TagTask.php
|   |   |   `-- TagTheme.php
|   |   |-- Model
|   |   |   `-- Tag.php
|   |   `-- Tag.php
|   `-- Tag.php
|-- media
|   `-- tag.css
|-- module.info
|-- tests
|   |-- Tag_Item_Rest_Test.php
|   |-- Tag_Rest_Test.php
|   |-- Tag_Test.php
|   `-- Tags_Rest_Test.php
`-- views
    |-- admin
    |   `-- tags.html.php
    `-- tag
        |-- block.html.php
        `-- cloud.html.php


Thoughts?

Take care,
Shad

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