Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RFC] redesign steps #46

Merged
merged 20 commits into from
May 27, 2013
Merged

[RFC] redesign steps #46

merged 20 commits into from
May 27, 2013

Conversation

craue
Copy link
Owner

@craue craue commented Apr 17, 2013

No description provided.

@craue craue mentioned this pull request Apr 17, 2013
@daFish
Copy link
Contributor

daFish commented Apr 18, 2013

Wow. This is a huge refactoring and I'll try to test it out in the coming days.

@Abhoryo
Copy link
Contributor

Abhoryo commented Apr 22, 2013

@craue Do you really intend to implement this new design? I really want to start with this new design.
Do you think there are more updates to perform or you just wait issues?

@craue
Copy link
Owner Author

craue commented Apr 22, 2013

@Abhoryo: I plan to make this the next major version, yes. But it's still WIP and I wouldn't recommend to really use it yet, especially not in production. There might be bugs, things may change. You can, of course, give it a try and leave some feedback. ;)

@Abhoryo
Copy link
Contributor

Abhoryo commented Apr 22, 2013

This is for production (end of may) but only for back office for the moment.
I'll use this version tonight.

@craue
Copy link
Owner Author

craue commented Apr 22, 2013

@Abhoryo: You should take 1.1.2 then.

@Abhoryo
Copy link
Contributor

Abhoryo commented Apr 24, 2013

Define the steps skipping like this is great.
Plus I see that we can now define a different FormType for each step. So we can manage easily different entities on each page ?

@daFish
Copy link
Contributor

daFish commented Apr 24, 2013

Ok, took the evening to play with the PR and it work's very well already. I was able to convert my flows without any real pain. The definition of skipping a step using the loadStepsConfig()-method is really nice. But I'm not sure it even works when you try to define the skipping of the very first step. The closure is evaluated to true which was right but the step wasn't skipped at all. Is there a check missing somewhere?

Oh, and maybe one addition: The method getName() of the flow should return the same value as the form type and to make it easy you could propose:

public function getName()
{
    return $this->formType->getName();
}

But I'm not sure how this behaves when you bind multiple form types to a flow as @Abhoryo noted.

@craue
Copy link
Owner Author

craue commented Apr 24, 2013

@daFish: Good to know. :)

Skipping the first step should work as expected. I admit there are not many tests yet, but there's even one for this behavior (Demo1FlowTest). How does your failing setup look like?

As there can be one form type per step with this PR, I wouldn't add a default implementation for getName. But the upgrade guide at least mentions to return the same value as the (only) form type does to have the same validation groups.

@Abhoryo: Are you referring to #42? If so, I'd guess that this is still not possible. But I didn't try it.

@daFish
Copy link
Contributor

daFish commented Apr 25, 2013

@craue My setup looks like:

public function loadStepsConfig()
{
    return array(
        array(
            'label' => 'First step',
            'type'  => $this->formType,
            'skip'  => function ($currentStepNumber, $formData) {
                return $currentStepNumber == 1 && null !== $formData->getId()
            }
        )
    );
}

Maybe my problem lies somewhere else. Must investigate.

@craue
Copy link
Owner Author

craue commented Apr 25, 2013

@daFish: Indeed, the closure is not even evaluated. Will fix that.

@Abhoryo
Copy link
Contributor

Abhoryo commented Apr 25, 2013

@craue Indeed, the fact that you can choose differents fromType doesn't means that the bundle can handle different entity. My bad.

@craue
Copy link
Owner Author

craue commented Apr 25, 2013

@daFish: Should be fixed now.

@craue
Copy link
Owner Author

craue commented Apr 25, 2013

@daFish: But in that closure, you'd have to use $currentStepNumber > 0 (or don't check for the current step number at all in this case) to skip that step for subsequent steps as well. Otherwise that wouldn't work.

@daFish
Copy link
Contributor

daFish commented Apr 25, 2013

@craue Thanks for the fix. I'll try it out tonight and I guess I'd stumbled upon the issue too. ;)

@daFish
Copy link
Contributor

daFish commented Apr 25, 2013

@craue Works. 👍

@daFish
Copy link
Contributor

daFish commented Apr 29, 2013

Is it already possible to define one class per step? We now have the EmptyStep-class but I couldn't find any evidence we can already do this.

@craue
Copy link
Owner Author

craue commented Apr 29, 2013

@daFish: Not intended. Do you still see a need for this? I can imagine that having one class per step could be more difficult to overview the flow, compared to using loadStepsConfig. Also, when setting up skipped steps (e.g. depending on the current step number), it wouldn't be possible to reuse a single step class for different positions in two flows (e.g. step 1 in flow A, step 3 in flow B).

@Abhoryo
Copy link
Contributor

Abhoryo commented Apr 29, 2013

For one class per step.
I've a current case. I create a profile member with a formflow and in the third step, I want to add images to this profile.

In my form type, I've add my non-mapped fields:

$builder->add('_title','text', ['mapped' => false]);
$builder->add('_file','file', ['mapped' => false]);

In the template, add a submit button and disable the form validation on the next button

        <button type="submit" name="{{ flow.getFormTransitionKey() }}" value="add">
            {{- 'add' | trans -}}
        </button>
        <script type="text/javascript">
        $(document).ready(function() {
            $('.craue_formflow_button_last').attr('formnovalidate', 'formnovalidate');
        });
        </script>

In my Flow I've add a transition:

    public function isAddTransition()
    {
        if ($this->request->isMethod('POST') && $this->getRequestedTransition() == self::TRANSITION_ADD) {
            return true;
        }

        return false;
    }

In my controller, I handle the transition and the image entity.

$profile = new Profile;
// form of the current step
$form = $flow->createForm($profile);
if ($flow->isValid($form)) {
    $flow->saveCurrentStepData();

    switch ($flow->getCurrentStep()) {
        case 3:
            if (null !== $file = $request->files->get('addProfile')['_file']) {
                // move the upload file and get the new path <new_path>

                $images = $storage->get($flow->getStepDataKey().'_images');

                $images[] = array(
                    'path' => <new_path>,
                    'title' => $request->request->get('addProfile')['_title']),
                );

                $storage->set($flow->getStepDataKey().'_images', $images);
            }
            break;
    }

    if ($flow->isAddTransition() || $flow->nextStep()) {
        // form for the next step
        $form = $flow->createForm($profile);
    } else {
        // Flow finished

        // Persist the profile entity

        // Persist the images with the profile entity
        $images = $storage->get($flow->getStepDataKey().'_images');
        foreach ($images as $image) {
            $image = New Image;
            $image->setProfile($profile);
            // Persist the image
        }

        // redirect
    }
}

@daFish
Copy link
Contributor

daFish commented Apr 29, 2013

@craue I mainly asked out of curiosity since I saw the class.

@craue
Copy link
Owner Author

craue commented Apr 29, 2013

@daFish: Which way of setting up steps would you prefer, and why? There's nothing finalized yet. So we can discuss which way is more useful.

@craue
Copy link
Owner Author

craue commented Apr 29, 2013

@Abhoryo: Would it make any difference if there would be one class per step definition? I don't see an advantage for that code.

@daFish
Copy link
Contributor

daFish commented Apr 29, 2013

@craue: As this bundle is aimed on form flows only I'd leave it the way it is simply because anything else would be oversized. If the bundle was a generic workflow-engine it would make sense to have separated steps.
I'd leave it as it is because it works well IMHO.

@Abhoryo
Copy link
Contributor

Abhoryo commented Apr 29, 2013

My code is more about perform the same step to generate others entities.

@craue
Copy link
Owner Author

craue commented Apr 30, 2013

@Abhoryo: What do you mean? I don't understand.

@Abhoryo
Copy link
Contributor

Abhoryo commented Apr 30, 2013

In the third step, when I click on the Add button, the Add transition is called so the flow stay on the same step but I store the data posted. Plus I've disabled the validation of the form on the next button.

@Abhoryo
Copy link
Contributor

Abhoryo commented May 3, 2013

Nothing, I was just about the different entities for a flow.

@havvg havvg mentioned this pull request May 17, 2013
@havvg havvg mentioned this pull request May 17, 2013
craue added a commit that referenced this pull request May 27, 2013
@craue craue merged commit 6e70ceb into master May 27, 2013
@craue craue deleted the wip-redesign-steps branch May 27, 2013 14:36
@craue
Copy link
Owner Author

craue commented May 27, 2013

Merged everything. Will tag 2.0.0 later.

@craue
Copy link
Owner Author

craue commented May 27, 2013

Done. Thank you guys, especially @havvg.

@Abhoryo
Copy link
Contributor

Abhoryo commented May 28, 2013

I'm just migrate to this version.
It seems that it is not necessary to define the formType as a service if you use the approach B.

@craue
Copy link
Owner Author

craue commented May 28, 2013

@Abhoryo: Depends. If you need to inject other services into your form types, you'd define them as services, tag them and use their alias for the type option.

@Abhoryo
Copy link
Contributor

Abhoryo commented May 28, 2013

Ok, I understand.
Thanks, my whole project is ok after the migration and the step skpping feature is great.

@craue craue changed the title [RFC][WIP] redesign steps [RFC] redesign steps Apr 4, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants