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

Hey Bharat,

I think at this point we're both on the same page regarding how to handle
the general case of class files.  And, at least for myself, I can say that
I feel pretty sure about it.  Awesome.

The part I feel less sure about is how we handle the Gallery-specific stuff
(event, block, etc.).  It seems there are one of two general paths to
follow:
- make them basically the same as the general case.
- make them different.

-----

SAME AS GENERAL CASE

Pros:
- Follows PSR-0 auto-loading standards.
- Allows our event-running routines to just look for classes and let
auto_load work its magic.
- Gives modules the flexibility of overriding each other's
events/installers/etc.

Cons:
- Gives modules the flexibility of overriding each
other's events/installers/etc. (!)
- Doesn't provide any division between generic and Gallery-specific stuff.
- Slow.  Finding event classes is O(n^2) since Kohana::find_file() is
agnostic to the module name and will search every module for TagEvent until
it's found.
- Requires capitalizing the module name when used for the class name.
- Requires *knowing* the capitalization, too.  You're not fond of adding a
line to module.info (more dev overhead), and I'm not fond of capitalizing
the module directory name (seems out of sync with "classes", "system",
"view", "vendor", "orm", etc.).
- May screw up how our REST API works (we at least need to be careful how
we map a resource called "item_tags" to its class).

All this makes me wonder if we can do better...

-----

CONSIDER AS DIFFERENT CASE

There are already a few precedents in Kohana where we bypass auto_load()
and run find_file() directly:
- finding views (ref system/classes/Kohana/View.php)
- finding media
- finding vendor classes

So, why not do the same for us?  Make a new subdir in the module directory
(I propose "gallery"), then make our event-running routines call
find_file() directly.

Examples:
  /modules/tag/gallery/event.php ==> class: tag_event
  /modules/tag/gallery/rest.php ==> classes: tag_rest, tags_rest,
tag_item_rest, tag_items_rest, item_tags_rest

How we get them:
  $paths = Kohana::find_file("gallery", "event", null, true);
  $paths = Kohana::find_file("gallery", "rest", null, true);

The last "true" argument means it will search *every* module and return an
array.  We do this once, cache the result, do a "require" for each found
path, and then use "class_exists($class, false)" when running the events to
skip auto-loading.

Pros:
- Nicely divides generic and Gallery-specific stuff.
- Faster.  Finding event classes is O(n).
- Caching the list means we can be even faster, as running the second event
of its type (e.g. page_bottom() after page_top()) only requires searching
the classes we've actually found.
- Leaves module names used in these special classes lowercase and still
allows underscores.  This probably helps things like the REST API.
- Requires knowing that we left it lowercase, but simply forcing this to be
same as the module directory name erases both of our previous concerns.
- Does not actually break strict K3 conventions.
- Removes the ability of modules to override each other's
events/installers/etc.

Cons:
- Removes the ability of modules to override each other's
events/installers/etc. (still technically can, but it'd involve a few
not-so-great practices)
- No longer follows PSR-0 autoloading standards. (but we're not generically
auto-loading things, so I'm not sure we care)
- Adds a little complexity to our event-running loops, but really not too
much.
- Means there's another place for devs to look for code, but this is
already the case with views anyway.

Here's my first pass at what this would do to our tag directory tree:

tag
|-- classes
|   |-- Controller
|   |   |-- Admin
|   |   |   `-- Tags.php
|   |   |-- Tag.php
|   |   |-- TagName.php
|   |   `-- Tags.php
|   |-- Model
|   |   `-- Tag.php
|   |-- Tag
|   |   |-- Controller
|   |   |   |-- Admin
|   |   |   |   `-- Tags.php
|   |   |   |-- Tag.php
|   |   |   |-- TagName.php
|   |   |   `-- Tags.php
|   |   |-- Model
|   |   |   `-- Tag.php
|   |   `-- Tag.php
|   `-- Tag.php
|-- gallery
|   |-- block.php
|   |-- event.php
|   |-- installer.php
|   |-- rest.php      (note: all five rest classes defined here)
|   |-- rss.php
|   |-- task.php
|   `-- theme.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



On 21 March 2013 00:30, Bharat Mediratta <[email protected]> wrote:

>
>
>
> On Wed, Mar 20, 2013 at 4:16 PM, Shad Laws <shad-xpYdmXCiSuZWk0Htik3J/[email protected]> wrote:
>
>> Hey Bharat,
>>
>> So, here's my pass at it.  Tag 3.0.x has 17 class files, and
>> correspondingly Tag 3.1.x has 17 class files and 17 extension files (34
>> total).
>>
>> tag
>> |-- classes
>> |   |-- Controller
>> |   |   |-- Admin
>> |   |   |   `-- Tags.php
>> |   |   |-- Tag.php
>> |   |   |-- TagName.php
>> |   |   `-- Tags.php
>> |   |-- Model
>> |   |   `-- Tag.php
>> |   |-- Tag                   (**1**)
>> |   |   |-- Controller
>> |   |   |   |-- Admin
>> |   |   |   |   `-- Tags.php
>> |   |   |   |-- Tag.php
>> |   |   |   |-- TagName.php
>> |   |   |   `-- Tags.php
>> |   |   |-- Model
>> |   |   |   `-- Tag.php
>>  |   |   |-- Tag.php
>> |   |   |-- TagBlock.php      (**2**)
>> |   |   |-- TagEvent.php
>> |   |   |-- TagInstaller.php
>> |   |   |-- TagRest
>>
>
> So you did TagRest here instead of Tag/Rest- this was inconsistent in mine.
>
>
>> |   |   |   |-- Item.php
>> |   |   |   |-- ItemTags.php
>> |   |   |   |-- Items.php
>> |   |   |   `-- Tags.php
>>
>
> You're missing a few rest handlers, but that's just an artifact of the
> modelling I assume.
>
>
>> |   |   |-- TagRest.php
>> |   |   |-- TagRss.php
>> |   |   |-- TagTask.php
>> |   |   `-- TagTheme.php
>> |   |-- Tag.php
>> |   |-- TagBlock.php
>> |   |-- TagEvent.php
>> |   |-- TagInstaller.php
>> |   |-- TagRest
>> |   |   |-- Item.php
>>  |   |   |-- Items.php
>> |   |   `-- Tags.php
>> |   |-- TagRest.php
>> |   |-- TagRss.php
>> |   |-- TagTask.php
>> |   `-- TagTheme.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
>>
>> Notes (as annotated above)
>> 1. All actual class code goes in this directory, and all thin
>> extension-enabling files are outside it.  This makes it easy to browse
>> code, as you can navigate straight here and ignore everything else.  Also,
>> note that the *exact* name of this directory is not super important,
>> provided that all the classes inside it follow its name (and that it
>> doesn't set a precedent for other devs that ends up causing name
>> collisions).
>>
>
> Agreed.
>
>
>>
>> 2. It's for these files that the exact name "Tag" is important, as
>> Gallery needs to know to look for TagEvent instead of TAGEvent.  So, we can
>> add a line to module.info.  BTW, I'm starting to come around to the
>> little-endian way, as alphabetizing them groups them together.  Of course,
>> there are other options, too: they could be Tag_Event (which extends
>> Tag_Tag_Event), Module_TagEvent (which extends Tag_Module_TagEvent), etc.,
>> but I'm inclined to think the simplest one is just TagEvent (which extends
>> Tag_TagEvent).
>>
>
> PHP is case insensitive when it comes to class names so "new TagEvent",
> "new TAGEvent" and "new tagEvent" are all the same.  But when it comes to
> filenames this will be a problem.   But again, we can easily fix that with
> an opinionated framework.  Our opinion will be that your module directory
> name (which is your module id) is exactly the same as your prefix.
>
> Here are some example of module directory names and the corresponding file
> in which we'd look for their event handler:
>
>   modules/Tag/classes/TagEvent.php
>   modules/PDF/classes/PDFEvent.php
>   modules/foo/classes/fooEvent.php
>
> Agreed?  If you agree with that then I think we are now in perfect
> agreement.
>
> -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 ]