craue / CraueFormFlowBundle

Multi-step forms for your Symfony project.
MIT License
736 stars 118 forks source link

What really is the purpose of $estimatedCurrentStepNumber #313

Closed tomchkk closed 6 years ago

tomchkk commented 6 years ago

I'd like to better understand the purpose of the $estimatedCurrentStepNumber variable given to a skip option callable in FormFlow::loadStepsConfig(). It isn't explained throughly in the documentation and, after some research, I can't say I really see the point of it.

The docs mention it briefly and exemplify it as follows:

protected function loadStepsConfig() {
    return array(
        1 => array(
            'label' => 'wheels',
            'form_type' => 'MyCompany\MyBundle\Form\CreateVehicleStep1Form',
        ),
        2 => array(
            'label' => StepLabel::createCallableLabel(function() { return 'engine'; })
            'form_type' => 'MyCompany\MyBundle\Form\CreateVehicleStep2Form',
            'form_options' => array(
                'validation_groups' => array('Default'),
            ),
            'skip' => function($estimatedCurrentStepNumber, FormFlowInterface $flow) {
                return $estimatedCurrentStepNumber > 1 && !$flow->getFormData()->canHaveEngine();
            },
        ),
        3 => array(
            'label' => 'confirmation',
        ),
    );
}

From this example I infer that it's purpose - in this case - is to ensure that the step is only actually evaluated for skipping at the correct occurrence - i.e.: when the current step is not step 1 or, more generally, when the current step is not the previous step. My thought process is as follows:

If $estimatedCurrentStepNumber is 1 or less, we won't skip the step; if it's greater than 1, we will evaluate it for skipping according to other criteria.

But then, referring to the CraueFormFlowBundle Demo and its source code, the skip config for CreateTopicFlow disproves my theory because, on line 37, the skip function for step 3 also tests the $estimatedCurrentStepNumber against 1 - i.e.:

...

return array(
  array(
    'label' => 'basics',
    'form_type' => $formType,
  ),
  array(
    'label' => 'comment',
    'form_type' => $formType,
  ),
  array(
    'label' => 'bug_details',
    'form_type' => $formType,
    'skip' => function($estimatedCurrentStepNumber, FormFlowInterface $flow) {
      return $estimatedCurrentStepNumber > 1 && !$flow->getFormData()->isBugReport();
    },
  ),
  array(
    'label' => 'confirmation',
  ),
);

...

I realise that the $estimatedCurrentStepNumber variable name itself tells me that its value is an estimation, but in my testing I cannot find any predictable correlation between this variable's value and the actual current step value. Because of this, in my current project, I tried not using $estimatedCurrentStepNumber at all in my skip callables, and I cannot see any differences to the correctness of my flow implementation.

So it appears to me that testing the value of $estimatedCurrentStepNumber doesn't really serve any purpose at all. I would be very interested to learn otherwise though.

Thanks

craue commented 6 years ago

You're right that it's only an estimation while determining the final value. And you're also right that it's meant to avoid executing the rest of the function when it's not relevant yet within the current progress of the flow. But it's also useful to avoid accessing invalid (or not yet given) data.

In the demo code you mentioned, the category (e.g. if it's a bug report) is set in step 1, so calling $flow->getFormData()->isBugReport() would only make sense when step 1 is done already. The goal is to only enable step 3 if the category has been set to "bug report" in step 1.

tomchkk commented 6 years ago

Thanks, @craue, for explaining that.