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

Two quick updates to my thinking:
- While event and theme are always called on all modules simultaneously,
that's not the case for the others.  So, the function I sketched out should
be tweaked to *search* all modules at once, but if we only ask for one, it
should only load the one we're looking for.
- The rest files are unique in that they're not 1:1 with modules.  Rather
than merge them together as I suggested before (which doesn't seem great
and would break registry_rest), maybe a subdir for them makes more sense.
 This would give a tree more like this:

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
|   |   |-- item_tags.php
|   |   |-- tag.php
|   |   |-- tags.php
|   |   |-- tag_item.php
|   |   `-- tag_items.php
|   |-- 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 11:58, Shad Laws <shad-xpYdmXCiSuZWk0Htik3J/[email protected]> wrote:

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