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

Sidenote: maybe optimizing around call_user_func_array isn't really worth
it anyway.  For example, the dev's of Kohana's ORM module had the same
discussion, did some tests, and came to the conclusion the savings was so
teeny that the clean code base was a bigger win:
http://dev.kohanaframework.org/issues/2803


On 21 March 2013 14:51, Shad Laws <shad-xpYdmXCiSuZWk0Htik3J/[email protected]> wrote:

> A quick sketch of what the gallery-functioning-running code might look
> like...
>
> In Module.php:
>
>   Module::$gallery_func_modules = array();
>
>   function run_gallery_func($type, $function, &$data=null, $name=null) {
>     // If we haven't already done it, find the files and load them.
>     if (!isset(Module::$gallery_func_modules[$type])) {
>       if ($found = Kohana::find_file("gallery", $type, "php", true)) {
>         foreach ($found as &$result) {
>           // Load the file.
>           require $result;
>           // Extract the module name from the full path.
>           $result = substr(strrchr("/", substr($result, 0, - 13 -
> strlen($type))), 1);
>         }
>       }
>       // Cache the array of module names (which may be empty if nothing
> was found).
>       Module::$gallery_func_modules[$type] = $found;
>     }
>
>     // If $name is specified, only run one class named $name_$type
>     if ($name) {
>       $class = "{$name}_{$type}";
>       if (class_exists($class, false) && method_exists($class, $function))
> {
>         return call_user_func_array(array($class, $function), $data);
>       }
>       return null;
>     }
>
>     // Run all classes named *_$type
>     foreach (Module::$gallery_func_modules[$type] as $module) {
>        $class = "{$module}_{$type}";
>       if (method_exists($class, $function)) {
>         call_user_func_array(array($class, $function), $data);
>       }
>     }
>   }
>
>   // Example: to run a gallery event (ref. module::event())
>   Module::run_gallery_func("event", $function, &$data);
>
>   // Example: to run a rest function (ref. rest::resolve()) and return the
> result
>   return Module::run_gallery_func("rest", $function, &$data, "item_tags");
>
> This isn't done yet (e.g. no optimization around call_user_func_array for
> small numbers of arguments like module::event(), etc.), but I think it
> implements by general idea.
>
> 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 ]