Design an ApplicationController implementation together

[Ezku]

Yes, I think that would be nice if possible.

Alright. I modified them:

interface IInputRule
{
    /**
     * Validate DataObject. Return true for success. Report errors to Logger.
     * @param    object    DataObject
     * @param    object    ILogger
     * @return    boolean    is valid
     */
    public function validate(DataObject $data, ILogger $logger);
}

class InputController implements IInputRule
{
    (...)
}

class InputValidator implements IInputRule
{
    (...)
} 

How is the Application Controller, Input Controller and Flow Controller related to each other? --By just looking at the code, I think the responsibilities of the Flow Controller overlap with the Input Controller.

As my code says, I intended the InputController to be an InputValidator+InputFilterChain facade. It's not an actual flow Controller as it doesn't even implement IHandler, but I couldn't come up with a more suitable name. So my IC is meant to be used by another Controller, and FlowController and InputController most certainly don't overlap.

For clarity, here's FlowController modified to my InputController:

class FlowController implements IHandler
{
    /**
     * @var    object    IHandler
     */
    protected $success = NULL;
    /**
     * @var    object    IHandler
     */
    protected $failure = NULL;
    /**
     * @var    object    IInputRule
     */
    protected $validator = NULL;
    
    public function __construct(IHandler $success, IHandler $failure)
    {
        $this->success = $success;
        $this->failure = $failure;
        $this->validator = new InputController;
    }
    
    public function execute(Request $request, Response $response)
    {
        $logger = new Logger;
        if ($this->validator->validate($request, $logger))
        {
            $this->success->execute($request, $response);
        }
        else
        {
            $this->request->set('errors', $logger->getMessages());
            $this->failure->execute($request, $response);
        }
    }
    
    public function addFilter($filter)
    {
        $this->validator->addFilter($filter);
    }
    
    public function addRule($rule)
    {
        $this->validator->addRule($rule);
    }
} 

Please don't take me for someone who's for the FlowController approach, I don't have an opinion here - I'm just trying to accommodate.

I repeat, my InputController isn't a flow controller. If you want InputController to have those abilities - I don't - then you should rename mine and have your class use it: the input filtering and validation part definitely needs to be a in separate class. Actually I'd be glad to have suggestions for a new name, as the current one is admittedly confusing.

[Dr Livingston]

Umm...

...
public function execute(Request $request, Response $response)
    {
        $logger = new Logger;
        if ($this->validator->validate($request, $logger))
        {
            $this->success->execute($request, $response);
        }
        else
        {
            $this->request->set('errors', $logger->getMessages());
            $this->failure->execute($request, $response);
        }
    } 
... 

Instead of passing in both the Request and Response objects, and again having to pass these over later to a success or failure object, wouldn't it be better to pass in a Factory instead, which already has the Request and Response objects?

So you can still continue to execute either success or failure... You don't then have the overhead of having to pass around the Request and Response objects no? ie

public function execute( $factory )
    {
        $logger = new Logger;
        if ($this->validator->validate($request, $logger))
        {
// what does success do? I see no class method for this 'success' ??
            $this->factory->onSuccess();
        }
        else
        {
            $this->request->set('errors', $logger->getMessages());
            $this->factory->onFailure();
        }
    } 

Unless of course, the Request and Response are required by the FlowController it's self? Thoughts...

[Ezku]

Unless of course, the Request and Response are required by the FlowController it's self?

They are - IHandler defines the execute method parameters, so if you don't want to go changing that - well, no good. You could, OTOH, rig it like this:

    public function execute(Request $request, Response $response)
    {
        $this->request = $request;
        $this->response = $response;
        $logger = new Logger;
        
        if ($this->validator->validate($request, $logger))
        {
            $this->factory->onSuccess();
        }
        else
        {
            $this->request->set('errors', $logger->getMessages());
            $this->factory->onFailure();
        }
    } 

I don't quite get the point, but there you go anyway.

what does success do? I see no class method for this 'success' ??

Success and Failure are both IHandlers that are passed to the FlowController as its constructor parameters.

[wdeboer]

Any place where I can get the latest version of this project? I am using PHP 4.

[kyberfabrikken]

Dr Livingston > That would mean that FlowController was the only controller which didn't implement the IHandler interface. However, we might want to replace the execute(Request $request, Response $response) with execute(DataSpace $context) making $context a registry for request/response. The danger is that this may become a MagicContainer.
It would solve a problem I ran into, with the following line in FlowController :

$this->request->set('errors', $logger->getMessages()); 

It would make sense to put the logger in the context-object, rather than modify the request. (what if you had a field named 'errors' ?)

Any place where I can get the latest version of this project? I am using PHP 4.

I'm afraid that there are several version currently.

There is one version here and another one here

See my comparison of the two in post #75

[Ezku]

However, we might want to replace the execute(Request $request, Response $response) with execute(DataSpace $context) making $context a registry for request/response. The danger is that this may become a MagicContainer.

It would solve a problem I ran into, with the following line in FlowController :

$this->request->set('errors', $logger->getMessages()); 

It would make sense to put the logger in the context-object, rather than modify the request. (what if you had a field named 'errors' ?)

My thoughts exactly. I've been thinking about bringing in a Context object, but haven't been able to conjure anything solid.

This, I feel, is important - what do others think about it?

[kyberfabrikken]

class InputValidator
{
(...)
    public function validate(DataObject $data, ILogger $logger)
    {
        $errors = 0;
        foreach ($this->rules as $rule)
        {
            $error = $rule->validate($data);
            if (!empty($error))
            {
                $logger->log($error);
                ++$errors;
            }
        }
        return ($errors == 0);
    }
} 

Any particular reason why you don't implement validate() as :

class InputValidator
{
(...)
    public function validate(DataObject $data, ILogger $logger)
    {
        $valid = TRUE;
        foreach ($this->rules as $rule)
        {
            $valid = $rule->validate($data, $logger) && $valid;
        }
        return $valid;
    }
} 

If rules and InputValidator are to share the same interface (IInputRule), the rules should also take a Logger as argument.

Off Topic:


btw. I still think IRule is way cooler than IInputRule.

[wdeboer]

Yes, the Context object seems reasonable to have.

[Ezku]

Any particular reason why you don't implement validate() as: (...)

Lack of brain capacity.

If rules and InputValidator are to share the same interface (IInputRule), the rules should also take a Logger as argument.

They do. You were looking at an older version.

Off Topic:

-- IRule is way cooler than IInputRule.

Sure is , but at this stage we have to be a bit more specific in our naming conventions. Calling it IRule instead of IInputRule would mean we'd have a corresponding IFilter, which, you'll surely realize, just won't do. "Input" explicitly communicates what the interface is related to.

[arborint]

Dr Livingston > That would mean that FlowController was the only controller which didn't implement the IHandler interface. However, we might want to replace the execute(Request $request, Response $response) with execute(DataSpace $context) making $context a registry for request/response. The danger is that this may become a MagicContainer.
It would solve a problem I ran into, with the following line in FlowController :

$this->request->set('errors', $logger->getMessages()); 

It would make sense to put the logger in the context-object, rather than modify the request. (what if you had a field named 'errors' ?)

I think the Context object is looking more like a Service Locator which is a MagicContainer but a good kind. Going to a global registry like a Service Locator can certainly simplify the code. A Service Locator will probably lead us toward Dependency Injection which is fine but is a pretty fundamental change. How do we let someone implementing a framework from this code choose any of the these options?

[arborint]

If rules and InputValidator are to share the same interface (IInputRule), the rules should also take a Logger as argument.

On the subject of Logger I have a couple of comments. First I don't think the primitive Rule and Filter classes should take a Logger because if you want to use them standalone it seems overkill to have an object to hold one error. If the Validator and FilterChain use the then it makes more sense.

I think the reason kyberfabrikken started using the Logger is because he wasn't involved in the the InputController code which itself is a Logger (the errors are held in each Parameter). My problem with the Logger is that it isn't a response to the question: how will the error strings be accessed and used? For example, a Form Controller wants to redisplay the values with associated error messages. It may also want to highlight the inputs with errors. The problem with the Logger as implemented is that we loose any association of the error to a specific parameter. You can add that to the Logger, but then you are back to the InputController (but without the other Parameter info all together in a Value Object).

[Ezku]

I think the Context object is looking more like a Service Locator which is a MagicContainer but a good kind. Going to a global registry like a Service Locator can certainly simplify the code. A Service Locator will probably lead us toward Dependency Injection which is fine but is a pretty fundamental change.

I found this article by Martin Fowler about IoC patterns. Something to build upon if anyone's as ignorant of these concepts as I am.

[kyberfabrikken]

First I don't think the primitive Rule and Filter classes should take a Logger because if you want to use them standalone it seems overkill to have an object to hold one error. If the Validator and FilterChain use the then it makes more sense.

I don't see the harm - the calling object could pass a reference to itself. On the contrary I think the Visitor-approach is superior, since it's more flexible than returning an array.

There has never been any attempt at passing a logger to the Filter class btw.

The problem with the Logger as implemented is that we loose any association of the error to a specific parameter. You can add that to the Logger, but then you are back to the InputController (but without the other Parameter info all together in a Value Object).

The rules could log more verbose message if you prefer. Or the parameter could implement ILogger and pass itself.
You seem to be making an assumption here that a rule applies to a single parameter btw, which isn't always true.

While we're at the details-part, it seems that DataSource vs. DataSpace is a tie with 2 vs. 2. I wouldn't rule out HashMap completely, and it could be a compromise. Does anybody have anything in particular against HashMap ?

Once we settle on the name, I would like to update the cvs (frontcontroller code) with a DataSpace/DataSource/HashMap and accordingly update Request, which has some serious issues in the current implementation.

A Service Locator will probably lead us toward Dependency Injection which is fine but is a pretty fundamental change. How do we let someone implementing a framework from this code choose any of the these options?

We can't do both. It's either ServiceLocator or DependencyInjection. While the latter is interesting, I think it's a bit of an overkill. A ServiceLocator is much simpler to implement.

I found this article by Martin Fowler about IoC patterns. Something to build upon if anyone's as ignorant of these concepts as I am.

Watch the section ServiceLocatorVsDependencyInjection

[arborint]

I don't see the harm - the calling object could pass a reference to itself. On the contrary I think the Visitor-approach is superior, since it's more flexible than returning an array.

As I said, I don't mind it at the Validator level, it just doesn't make sense at the Rule level where there is just one error message. If you want to use a Rule standalone you just want the error string back.

The rules could log more verbose message if you prefer. Or the parameter could implement ILogger and pass itself.
You seem to be making an assumption here that a rule applies to a single parameter btw, which isn't always true.

It's not the verbosity that matters. Also at this point each rule is tied to a specific parameter so we can implement match rules. This was Overrunner's change passing a DataSource rather than a value to the Rule.

My point is that the error messages are already logged individually by each InputControllerParameter. Each InputControllerParameter is a Logger. That is where you want the error messages because they are associated with other parameter info like the value.

While we're at the details-part, it seems that DataSource vs. DataSpace is a tie with 2 vs. 2. I wouldn't rule out HashMap completely, and it could be a compromise. Does anybody have anything in particular against HashMap ?

It is currently DataSpace in the inputcontroller0.5 code so someone would have to change it to DataSource at this point. My focus is not on naming at this point.

We can't do both. It's either ServiceLocator or DependencyInjection. While the latter is interesting, I think it's a bit of an overkill. A ServiceLocator is much simpler to implement.

ServiceLocator certainly simplifies things, but I still haven't seen the need for anything beyond Request/Response. The only reason the Context came up is to support the Logger.

[Ezku]

My point is that the error messages are already logged individually by each InputControllerParameter. Each InputControllerParameter is a Logger. That is where you want the error messages because they are associated with other parameter info like the value.

In my opinion, the InputControllerParameter part here is really awkward. With them in, the whole InputController feels really clumsy in implementation - but you have a point in saying that with a single rule you'd ideally want it to just return the possible error. I'm looking into a fix that would enable us to do just that while retaining both the Logger and interchangeability.

About relating error messages to fields - we could have the Rule classes log an Error, that hosts conflicting field names and error messages. Simple and doable.

Would you mind posting the latest version of your approach?

[arborint]

In my opinion, the InputControllerParameter part here is really awkward. With them in, the whole InputController feels really clumsy in implementation - but you have a point in saying that with a single rule you'd ideally want it to just return the possible error. I'm looking into a fix that would enable us to do just that while retaining both the Logger and interchangeability.

About relating error messages to fields - we could have the Rule classes log an Error, that hosts conflicting field names and error messages. Simple and doable.

Would you mind posting the latest version of your approach?

For me the InputControllerParameter makes building a Controller from the InputController classes really easy. Each parameter has this Value Object that contains everything about the param/field. The Model can initialize them, the View can display them, but it keeps everything uncoupled. It really makes building general or custom Page and Form Controllers very easy. And that is the point of these controllers is to move the non-specific stuff into the common controler code.

It's funny, but clumsy is my reaction to the Logger. It takes organized information and dumps it into a single hopper. You are now proposing turning it into a DataSource by adding field information which is just copying the InputControllerParameter.

[kyberfabrikken]

Each parameter has this Value Object that contains everything about the param/field. The Model can initialize them, the View can display them, but it keeps everything uncoupled. It really makes building general or custom Page and Form Controllers very easy.

What the InputControllerParameter really is, is a configuration. This could be dealt with by using a loader/factory to create the concrete FormControllers.

After Ezku had some work done on it, I really like the FlowController-design.

It's funny, but clumsy is my reaction to the Logger. It takes organized information and dumps it into a single hopper. You are now proposing turning it into a DataSource by adding field information which is just copying the InputControllerParameter.

That's true, but if we don't have the InputControllerParameter in the first place, there isn't really any duplication ?

(...) but you have a point in saying that with a single rule you'd ideally want it to just return the possible error. I'm looking into a fix that would enable us to do just that while retaining both the Logger and interchangeability.

Even without the Logger-class I don't think it's a good idea to have seperate interfaces for the Rules and the Validator. Returning an array from Validator, but only a single message from the Rule is an interface mismatch.

I have updated the code I posted earlier with ezku's suggestions, which makes it much cleaner, so I thought I would post it here for the sake of comparing it against the StateMachine design.
I have added a context-object in the same go, though I'm still a bit lukewarm about that. I have also made some changes to the Request-object, which had some serious issues. I'd like some feedback on that, since I'm quite happy with that and think it should go into the frontcontroller-package (skeleton-thread).

[Ezku]

What the InputControllerParameter really is, is a configuration. This could be dealt with by using a loader/factory to create the concrete FormControllers.

I agree.

I really like the FlowController-design.

Yeah, it seems to make a nice tool for a one-page form implementation.

Even without the Logger-class I don't think it's a good idea to have seperate interfaces for the Rules and the Validator. Returning an array from Validator, but only a single message from the Rule is an interface mismatch.

That's exactly what I meant by retaining interchangeability. A bad choice of words?

I have added a context-object in the same go, though I'm still a bit lukewarm about that.

Yeah, the implications aren't exactly clear to me yet. So there's a Logger in the Context - that makes it an application-wide class. Where else does it get used, besides input validation? Is it a means to exchange data between layers in general? If so, shouldn't it be more like DataSpace?

I have also made some changes to the Request-object, which had some serious issues. I'd like some feedback on that, since I'm quite happy with that and think it should go into the frontcontroller-package (skeleton-thread).

I don't quite understand why you did this:

    function & getInstance() {
        static $instance = Array();
        if (count($instance) == 0) {
            $instance[0] =& new Request();
        }
        return $instance[0];
    } 

and not:

    function & getInstance() {
        static $instance = NULL;
        if (!isset($instance)) {
            $instance =& new Request();
        }
        return $instance;
    } 

Other than that, I don't know... I kinda liked your previous version with the data choosed based on wanted method. Well, perhaps this is cleaner after all.

You are now proposing turning it into a DataSource by adding field information which is just copying the InputControllerParameter.

Well, not quite. As kyberfabrikken said, the InputControllerParameters part is a configuration matter and can be dealt with in other ways. The benefit in doing it the other way is not having to instance loads of filterchains and validators, but one of each will suffice. A lot cleaner that way, isn't it?

[kyberfabrikken]

I don't quite understand why you did this:

There's a bug/limitation of php4, preventing you from storing a reference ín a static variable. It's a workaround.

Other than that, I don't know... I kinda liked your previous version with the data choosed based on wanted method. Well, perhaps this is cleaner after all.

The thing is, that even if the method is POST, you may want to access the $_GET data. For example, the applicationcontroller may be concerned with the POST data, while the requestmapper is looking at the GET data. (a very actual problem).
So a POST and GET request aren't equal to each other. The POST extends the GET and adds something additional to it.

[kyberfabrikken]

Yeah, (the FlowController-design) seems to make a nice tool for a one-page form implementation.

Just to be clear - It can handle as many states as need be. Just replace one of the ServerPages with a new FlowController. It can even jump back to a previous state. Anything that the statemachine can do, actually.

The FormController on the other hand is limited to a single form.

[Ezku]

There's a bug/limitation of php4, preventing you from storing a reference ín a static variable. It's a workaround.

Ah. You should document those in the code!

The thing is, that even if the method is POST, you may want to access the $_GET data. For example, the applicationcontroller may be concerned with the POST data, while the requestmapper is looking at the GET data.

I got a bit lost there before; this should be pretty obvious. It's just a matter of how you do it, and I can't think of a better way. Um, what about $_FILES et al?

Just to be clear - It can handle as many states as need be. Just replace one of the ServerPages with a new FlowController. It can even jump back to a previous state. Anything that the statemachine can do, actually.

I'm such an idiot. Indeed! That makes the prospects seem all the more enticing.

[arborint]

I have also made some changes to the Request-object, which had some serious issues.

I think we have opposing "serious issues" but as long as it supports get() you can use yours and I'll use mine. Your new one uses $_REQUEST and doesn't escape, nor do I see anywhere in the code where we need getInstance().

Well, not quite. As kyberfabrikken said, the InputControllerParameters part is a configuration matter and can be dealt with in other ways. The benefit in doing it the other way is not having to instance loads of filterchains and validators, but one of each will suffice. A lot cleaner that way, isn't it?

I see it as encaptsulation, not configuration. Actually having filterchains and validators in each parameter was kyberfabrikken's design. I originally just used a single filterchain and validator which I prefer.

[Overunner]

Kyber: Why have you applied the singleton pattern for the request class, and not for the response and errorLogger?

Ragarding the Input Controller, which is basically an request filter/validator (as it is now atleast)...in what parts of the framework should that class be used?

[Ezku]

Kyber: Why have you applied the singleton pattern for the request class, and not for the response and errorLogger?

I didn't get that part either. Rather, why exactly does the Request need to be a singleton?

Ragarding the Input Controller, which is basically an request filter/validator (as it is now atleast)...in what parts of the framework should that class be used?

Input validation can be used wherever you want it. This could be either at the ApplicationController stage (which we're now designing to) or the Action itself. Take a look at the FlowController, I think it's an excellent example of what you can achieve with very simple design. It chooses one of the two passed IHandlers based on whether the Request validates or not. FlowController is an IHandler in itself, so it's possible to chain an arbitrary number of these together to create a multi-page form. Pretty neat

Perhaps we could have an ApplicationController that creates states and transitions by means of FlowController chaining. I'll look into it tomorrow.

[kyberfabrikken]

(...) nor do I see anywhere in the code where we need getInstance().

Kyber: Why have you applied the singleton pattern for the request class, and not for the response and errorLogger?

No, the Request might not need to be a singleton, but I figured that since it is in fact a unique global, we might aswell show this by implementing it that way. I won't object if you want to remove it.

Response and errorLogger aren't singletons. You could make local copies. Could be useful for composite views.

Your (Request) uses $_REQUEST and doesn't escape

I didn't put much thought into removing the escaping. It's a fix, and since I have magic_quotes turned off anyway, I didn't use it. Perhaps it belongs in a filter. Or a regular compatibility-patch. (I think WACT does it this way).

Edit:


It springs to my mind that if the Request-class does strip slashes, it actually should be a singleton, to prevent stripping from happening twice.

I'm a bit in doubt whether the class should refer $_REQUEST or $_GET|$_POST (depending on method).
As of now - If you don't care if the params come from GET or POST, you can just use $request->get('param') (like you would use $_REQUEST without a Request-class). On the other hand if you're specifically interested in the POST-data, you can use $request->POST->get('param').
If we let the default getter select from $_GET|$_POST you don't have a I-dont-care-about-the-protocol way of getting a param.
Both designs would make sense, but I made it that way, because that's how native php works.