2

I have a block which outputs a form. The form action is generated dinamically using Mage::getUrl. The form is always posted to exactly the same url, from where it was rendered. I process the form's data inside the block's corresponding class file.

Problem: code review guys tells me, that the processing of the form should be inside the controller whose action ultimately displays the block, and that my solution is bad practice. I understand his argument, but on the other and, I think that a block class file is a perfect place to accomplish this, so that I don't have flood the controller of my page with logic, that is specific to the block. In fact, I feel that having the page know how to process a contained block is bad practice, since blocks are meant to be reusable withouth so much of a hassle?

So... can some senior magento people help me put things into order? Which one is good practice and why? Or is there a third solution that would be preferable?

3

I think Fabian has a good point and any heavy lifting of data should be done by a model.

I would like to add to that however that they way Magento works is generally catching the post of a page in the Controller, sorting out the data and then calling models to process that data.

Although this might not strictly be the way MVC dictates it, it is however how Magento does it and for keeping your code readable and logical for the next developer to work on I would like to advice you to follow this example.

Let's take a simple contact form that is stored in the database as an example. We will have a controller and model in this case. I'll just post the controller code below. The index action displays the form but will post to the postAction where all the data will be handled. After catching the data and validating it we just use the model to persist the data to the database.

In my opinion (correct me if I'm wrong) this is the common way how Magento extensions deal with post data from forms.

class [Namespace]_[Module]_indexController extends Mage_Core_Controller_Front_Action
{
    public function indexAction()
    {
        $this->loadLayout()
            ->renderLayout();
    }

    public function postAction()
    {
        $data = $this->getRequest()->getPost();

        // validate the data
        $validated = true;
        if (!Zend_Validate::is($data['name'], 'NotEmpty')) 
        {
            Mage::getSingleton('customer/session')->addError('Please fill out name');
            $validated = false;
        }
        if (!Zend_Validate::is($data['email'], 'NotEmpty')) 
        {
            Mage::getSingleton('customer/session')->addError('Please fill out email');
            $validated = false;
        }
        if (!Zend_Validate::is($data['message'], 'NotEmpty')) 
        {
            Mage::getSingleton('customer/session')->addError('Please fill out message');
            $validated = false;
        }

        // save the data
        if (!$validated)
        {
            $model = Mage::getModel('[model]/someclass');
            $model->setData(array(
                'sender_name' => $data['name'],
                'sender_email' => $data['email'],
                'message' => $data['message'],
            ));

            try 
            {
                $model->save();
            } 
            catch(Exception $e)
            {
                Mage::getSingleton('customer/session')->addError("{$e}");
                $this->_redirect('[module]/index/index');
            }

            Mage::getSingleton('customer/session')->addSuccess('Succesfully saved');
        }

        $this->_redirect('[module]/index/index');
    }
}
| improve this answer | |
2

I think you both are right :-)

Flooding controllers is bad

Flooding controllers with lots of code is bad pratice too, because code in controller is not reusable and you want short code pieces to be easy readable.

flooding view classes is bad too

The problem with putting everything in the block is bad, because the class is intended to show things and information. You don't expect the code there, so finding it is hast.

In a block class should only be code for the view (MVC)

put everything in a model

so the solution is to put everything in a model.

But with all things in software architecture, patterns, etc there are many ways to solve problems, achieve readability and findability...

| improve this answer | |

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.