Development

#9306 (Request for refactoring of sfDoctrineBuildTask)

You must first sign up to be able to contribute.

Ticket #9306 (new enhancement)

Opened 2 years ago

Request for refactoring of sfDoctrineBuildTask

Reported by: arialdomartini Assigned to: Jonathan.Wage
Priority: minor Milestone:
Component: sfDoctrinePlugin Version: 1.4.8
Keywords: Cc:
Qualification: Unreviewed

Description

I wanted to extend sfDoctrineBuildTask, then I created a new class extending sfDoctrineBuildTask. I wanted to rewrite the code of "--and-load" command option when I discovered that all the command options, in sfDoctrineBuildTask, are coded in the same method: this forces to rewrite it all.

The actual code is something like that:

  if ($options['option1'])
    {
       // Here the code for option1
    }
  if ($options['option2'])
    {
       // Here the code for option2
    }

This prevents from just overwriting one single option.

I propose this approach:

  if ($options['option1'])
    {
       $this->doOption1();
    }
  if ($options['option2'])
    {
       $this->doOption2()
    }

Doing this, anyone can extend a task this way:

 class MyTask extends sfDoctrineBuildTask
 {
    function doOption1()
    {
       // My implementation
    }
 }

Another reason why sfDoctrineBuildTask should be refactored is the static coupling between sfDoctrineBuildTask and other tasks, which prevent developers from substituting the tasks implementations used by sfDoctrineBuildTask.

For example, I would like to use my implementation of sfDoctrineDataLoadTask, used by sfDoctrineBuildTask.

Unfortunately, sfDoctrineBuildTask gets sfDoctrineDataLoadTask instance with this code:

  protected function execute($arguments = array(), $options = array())
    if (count($options['and-load']) || count($options['and-append']))
    {
      $task = new sfDoctrineDataLoadTask($this->dispatcher, $this->formatter);
      [...]

A better approach is with Factory Methods:

  protected function getDataLoadTask($dispatcher, $formatter) {
    return new sfDoctrineDataLoadTask($dispatcher, $formatter);
  }
  protected function execute($arguments = array(), $options = array())
    if (count($options['and-load']) || count($options['and-append']))
    {
      $task = $this->getDataLoadTask($this->dispatcher, $this->formatter);
      [...]

With this approach is very simple to substitute the sfDoctrineDataLoadTask implementation, for example with this code:

 class MyTask extends sfDoctrineBuildTask
 {
    function getDataLoadTask($dispatcher, $formatter)
    {
       return new MydataLoadTask($dispatcher, $formatter);
    }
 }

I can contribute with a refactored sfDoctrineBuildTask.
Anyone interested?