Re: [Phrame-devel] MVC sample application

[Jason S. <jsw...@ya...>]

--- Tony Bibbs <to...@to...> wrote:
> Again, this is not meant to be a jab at Phrame. I have talked to Arnold 
> and I'm hoping his work, the work attached and contributions from folks 
> like Jason can be quickly be merged into the 'best-of-all-worlds' to 
> quickly produce a production quality implementation.  Again, a good 
> portion of my motiviation is time related with respects to another 
> project so any feedback you can give in the short term would be appreciated.

Notes listed here as I came across them:

require_once('config.php');
require is a directive, not a function, so
require_once 'config.php'; 
is better form, mentioned in the PEAR coding conventions as acceptable either
way, but I have been dinged in professional writing elsewhere for including the
parens.

$_CONF is a bad name for a variable.  It looks like a superglobal, but clearly
is note.  It also ad-hears to the PEAR coding conventions definititions of a
define, not a global variable.

If you have these constants defined:
// General configuration settings
define('_BASE_URL', 'baseurl');
define('_MVC_BASE', 'dirbase');
then why do you need:
$_CONF['site_url'] = 'http://www.example.com/MVC/contactmanager';
$_CONF['path_mvc'] = '/path/to/MVC/';

in Controller::Controller()
       $_SESSION['MVC_message'] = '';
Should 'MVC_message' also be a define?  I tend to like to use defines for all
"magic" indexes, that way there is less risk of me mis-typing an assignment
index.

I would move the $configData initialization into the config.php script.  This
makes the "bootstrap" (index.php) file more understandable, and it is after
all, configuration, which seems like it belongs in the "config" script.   BTW,
I initialize a global db connection in my application_setup.php files (the
equivalent of your config.php file) so this does not seem out of place to me. 
(DB detour:
I actually insulate my developers from knowing the database passwords by
creating a function in our standard php library that returns a connected
database object from a parameter specifying the type of connection they want.)

The discussion of the $var[foo][bar] vs $var = array('foo'=>array('bar'=>...));
came up on the Phrame list earlier.  I said I was ambivilent then, but looking
at it:
// Models
$configData[_MODELS]['validate'][_NAME] = 'ValidateContact';
$configData[_MODELS]['validate'][_FORWARDS]['showAgain'][_TARGET] = 'edit';
$configData[_MODELS]['validate'][_FORWARDS]['showAgain'][_TYPE] = 'view';
$configData[_MODELS]['validate'][_FORWARDS]['saveIt'][_TARGET] = 'saveContact';
$configData[_MODELS]['validate'][_FORWARDS]['saveIt'][_TYPE] = 'model';

$configData[_MODELS] = array(
    'validate' => array(
        _NAME  	  => 'ValidateContact',
        _FORWARDS => array(
            'showAgain' => array(_TARGET => 'edit',        _TYPE => 'view'),
            'saveIt'    => array(_TARGET => 'saveContact', _TYPE => 'model')
            )
        )
    );

Some part of the programmer in me hates seeing the _MODELS and _FORWARDS and
'string keys' repeated over and over :)  I guess it would be easier for a
newcomer to php to understand perhaps?  Everyone is free to setup the array
however they like, so it does not matter much.

It is now 11:00PM and my brain is starting to get fuzzy.  Rather than continue
line by line, I jumped back and tried to get a picture of the whole
refactoring.  One major problem I see is the removal of Actions from the
design.  I admit many of my actions are relatively trivial pieces of code that
only interact with a single model, and could thus be capture as a function of
that model rather than a separate class.  And I also can see potential
elimination of the Forms and just access the superglobals directly.  

However, I do have a number of instances where my actions access a number of
models simultaneously, and I believe your refactoring would make this very hard
to implement.  For instance, I have a 3 step "wizard".  Each step is a separate
"Action" class under frame.  Each action interacts with three different models.
 The main model dealing with the database, a second model tied mainly to the
$_SESION tracking the inputs between the steps (the "wizard" model) and a user
model to track security.  I am not sure how this would be refactored under your
MVC.

Maybe it is just me being used to seeing things in a "Phrame" format, but I am
uncomfortable seeing things labeled "delete" and "saveContact" as models.  I
see these as being methods of a Contact model, implemented through delete and
save actions.

My last initial concern is also related to changing too much from the original
Struts implementation.  Prior to discovering Phrame, I had found Struts and had
purchased a book on the subject to better understand the MVC pattern.  If I
ever decide to migrate any of my developers or projects from PHP to Java, I see
some advantage to having the PHP/Phrame familiar developers having and easier
time migrating to Java/Struts.  Perhaps this is a very minor concern for you,
but it did at least occur to me.

It looks like you have put a lot of work and thought into your redesign.  I
agree with your sentiment that we should be able to combine all our efforts and
produce and outstanding MVC framework.

Regards,

Jason

__________________________________________________
Do you Yahoo!?
Yahoo! Platinum - Watch CBS' NCAA March Madness, live on your desktop!
http://platinum.yahoo.com