Update FormFlow.php

[robhunt3r]

Includes a flag to determine if we should expire the flow on a POST/PUT request, so we can create a flow from a POST/PUT request like this workflow

Load Route We do some stuff, for example in AngularJS Once finished our stuff, we submit data to the same controller (or another controller). That controller, handles the data and creates a flow according to submitted data. Flow is rendered.

This workflow won't work right now, because every POST/PUT request expires the flow.

[coveralls]

Coverage decreased (-0.3%) to 99.485% when pulling ba9ec03cfa04dae9e9500747b9ca2a9c55e2e8d1 on robhunt3r:patch-1 into 03b0604cfc7de8d890add83b3afd1b45bde1e533 on craue:master.

[coveralls]

Coverage decreased (-0.3%) to 99.485% when pulling ba9ec03cfa04dae9e9500747b9ca2a9c55e2e8d1 on robhunt3r:patch-1 into 03b0604cfc7de8d890add83b3afd1b45bde1e533 on craue:master.

[coveralls]

Coverage decreased (-0.3%) to 99.485% when pulling ba9ec03cfa04dae9e9500747b9ca2a9c55e2e8d1 on robhunt3r:patch-1 into 03b0604cfc7de8d890add83b3afd1b45bde1e533 on craue:master.

[craue]

I wonder why there's no stored data available to the flow, which in turn causes the flow to expire. Could you elaborate on this?

[coveralls]

Coverage decreased (-0.3%) to 99.485% when pulling ba9ec03cfa04dae9e9500747b9ca2a9c55e2e8d1 on robhunt3r:patch-1 into 03b0604cfc7de8d890add83b3afd1b45bde1e533 on craue:master.

[robhunt3r]

@craue I don't understand what do you mean?

This is my controller, if you want to see my workflow

    public function formAction(Request $request)
    {
        $product = new Product();
        if($category = $request->get('category')) {
            $product->setCategory($category);
        }

        $flow = $this->container->get('product_flow');
        $flow->bind($product);
        $form = $flow->createForm();

        if($category || $flow->isValid($form)) {
            $flow->saveCurrentStepData($form);

            if ($flow->nextStep()) {
                $form = $flow->createForm();
            } else {
                $data = $flow->getDataManager()->load($flow);
                $this->manager()->save($product, $data);
                $flow->reset();

                return $this->redirectToRoute('success');
            }

            return $this->render('form.html.twig',array(
                'form'      => $form->createView(),
                'flow'      => $flow,
            ));
        }

        return $this->render('preindex.html.twig',array('flow' => $flow));
    }

You may wonder why I need flow in my preindex template. Well, I need to render some things, for example, how many steps do I have, and that only can be retrieved from flow, but I actually don't render any form in there, just a few angularjs things, like a category selector, which is needed in order to create the form later. Actually there are a lot more conditions before rendering the form, but I've tried to simplify my code to be readable.

[craue]

I mean !$this->dataManager->exists($this). How are you able to work with the flow when there's no data stored? It seems odd to add code for a workaround instead of having a clean workflow which, admittedly, I still don't understand. E.g. due to if($category || $flow->isValid($form)) { the flow is processed even with invalid data as long as a category is set.

[robhunt3r]

It won't be processed even with invalid data, as $category will be null always but the first time (when we set the data to the product that has the form), then, it's always empty as the POST request won't have any 'category' stuff.

The workaround is because before the commit that came with the POST expiration, our flows were working perfectly, now, they're expired...

We may be confused and wrong about how to make our flow (in this particular case, with a POST request), in that case I don't know how to proceed.

About !$this->dataManager->exists($this) well, data is stored into flow before the first POST that sets a category into a product, and then our flow begins to work.

I've also tried to create the flow ONLY after the post, so no flow is created before we submit our angularjs stuff, and it's expired too. EG.

    public function formAction(Request $request)
    {
        $product = new Product();
        if($category = $request->get('category')) {
            $product->setCategory($category);
        }

        if($category || $product->getCategory()) {

             $flow = $this->container->get('product_flow');
             $flow->bind($product);
             $form = $flow->createForm();
             if($flow->isValid($form)) {
                 $flow->saveCurrentStepData($form);

                 if ($flow->nextStep()) {
                     $form = $flow->createForm();
                 } else {
                     $data = $flow->getDataManager()->load($flow);
                     $this->manager()->save($product, $data);
                     $flow->reset();

                     return $this->redirectToRoute('success');
                 }
            }

            return $this->render('form.html.twig',array(
                'form'      => $form->createView(),
                'flow'      => $flow,
            ));
        }

        return $this->render('preindex.html.twig',array());
    }

[craue]

Would it work to create the flow with a GET request? Sending a POST before the flow's storage has been initialized seems to be the issue here.

[robhunt3r]

Using a GET request won't work as desired. Using GET, we get querystring ?category=142, which triggers the if($category || $flow->isValid($form)) every single time, bypassing an invalid form, and obviously isn't good, that's why we need to use POST request.

The bug is also expiring forms in other controller actions, as if we send the POST request to formRenderAction from formAction even if they have distinct routes.

And all that, is why I thought using a flag for expiring the form is needed in our case, and as it is harmless I made the PR, if you don't set it to false it will work the normal way.

[craue]

@Nairebis, do you have an opinion on this?

[craue]

@robhunt3r, I thought about using GET only for the first request to properly start the flow. You don't need to send the category parameter on subsequent POST requests if it would otherwise break the logic.

I'm still not sure if the issue is in the bundle or in your code handling the flow, so I'm not yet convinced that the bundle needs another option.

Would it help to change your action to this? (Edit: Never mind, that's silly, since the instance id is not set when save will be called.)

public function formAction(Request $request) {
    $product = new Product();
    $flow = $this->container->get('product_flow');

    if ($category = $request->get('category')) {
        $product->setCategory($category);
        $flow->getDataManager()->save($flow, array());
    }

    if ($category || $product->getCategory()) {
        $flow->bind($product);
        $form = $flow->createForm();
        ...

If not, could you set up a reasonable test (or project) that passes with your introduced $expiresOnRequest being false and fails otherwise?

[robhunt3r]

I will upload a test on Monday, as I am leaving the office now and it's holydays until monday, and I don't have the project at home.

[SiliconEngine]

I'm not sure I completely have a handle on how things are being used here, but it sounds like the form is initialized through a POST request? But the expiration detection won't trigger unless it already has an instance ID generated (the creation sets the newInstance flag, which blocks the expiration action).

robhunt3r mentioned needing "flow in my preindex template". I'm thinking that might be generating the instance ID somehow, and then perhaps bind() isn't getting called, which is what initializes the storage slot. Then POSTing into the form unknown to the session triggers the expiration, which is what it should do.

Assuming the above is what's really going on, I can see three ways to handle it, depending on what makes sense with what robhunt3r is doing:

1) Have robhunt3r call bind() in his preindex template, which would initialize the session.

2) Move the instance slot initialization. Again, without knowing the code flow, it's hard to know if this makes sense.

3) If moving the slot initialization or calling bind() would create other problems, then a manual initializeInstance() call might make sense.

Of course, #2 is attractive, since that wouldn't require any client code changes. Just casually looking at the code, the slot initialization could be done in determineInstanceId() where it's actually generated. I think the reason I didn't do it that way in the first place was: 1) it architecturally made sense to do it in bind() where all the rest of the magic happens, and 2) I was thinking it might create unused slots in the session if instance IDs were casually created multiple times, which I wasn't sure about.

[robhunt3r]

@craue Functional test https://github.com/robhunt3r/flowTest

Load data fixtures to have some data.

There are two URL

http://localhost/app_dev.php/create [GET] Here you have the angularjs pre-create stuff, that will post to the next url http://localhost/app_dev.php/create [POST] Here you handle the flow stuff.

There's also http://localhost/app_dev.php/noextra, in that URL, try to submit the form without filling any input, you will get a This value is not valid. error, if you then fillt he data an try to submit again, you will have the expired flow error. This is even worst than the first case (because first case is more particular for me), because when disabling HTML5 validation, you can submit the form and return the errors... as it's been POSTed and BINDed before, expiration is true... That must be fixed.

In that URL i'm also trying to recreate another error I got randomly in my project, but I will open another PR if I recreate it successfuly.

@Nairebis ping.

[craue]

With these changes, the create flow works for me:

diff --git a/src/AppBundle/Controller/DefaultController.php b/src/AppBundle/Controller/DefaultController.php
index 4264e8a..8d33a3e 100644
--- a/src/AppBundle/Controller/DefaultController.php
+++ b/src/AppBundle/Controller/DefaultController.php
@@ -76,7 +76,7 @@
             $flow->saveCurrentStepData($form);

             if($flow->nextStep()) {
-
+                $form = $flow->createForm();
             } else {
                 $em = $this->getDoctrine()->getManager();
                 $em->persist($formData);
diff --git a/src/AppBundle/Entity/Vehicle.php b/src/AppBundle/Entity/Vehicle.php
index 05ab26e..2c4006c 100644
--- a/src/AppBundle/Entity/Vehicle.php
+++ b/src/AppBundle/Entity/Vehicle.php
@@ -32,7 +32,7 @@
     /**
      * @var integer
      */
-    private $price;
+    private $price = 0;

     /**
      * @var \AppBundle\Entity\Category
diff --git a/src/AppBundle/Resources/views/preCreate.html.twig b/src/AppBundle/Resources/views/preCreate.html.twig
index f9d93d6..6f1a701 100644
--- a/src/AppBundle/Resources/views/preCreate.html.twig
+++ b/src/AppBundle/Resources/views/preCreate.html.twig
@@ -24,6 +24,8 @@

 {% block render_form %}
     <form method="POST" name="testForm" action="">
+        <input type="hidden" name="{{ flow.getInstanceKey() }}" value="{{ flow.getInstanceId() }}" />
+        <input type="hidden" name="{{ flow.getFormStepKey() }}" value="{{ flow.getCurrentStepNumber() }}" />
         <select name="product" class="form-control">
             <option value="1">Product 1</option>
             <option value="2">Product 2</option>

[robhunt3r]

Great, so I just need to add theese two lines

<input type="hidden" name="{{ flow.getInstanceKey() }}" value="{{ flow.getInstanceId() }}" /> <input type="hidden" name="{{ flow.getFormStepKey() }}" value="{{ flow.getCurrentStepNumber() }}" />

I can't set $price = 0; because there are many other fields like this, with relations, so no default value can be used.

[craue]

The issue is that this custom form was sent without these fields identifying the current flow instance. Even better if this also fixes your real project. :smirk:

There's still something odd after a validation error occured since the instance and step field have no value which in turn leads to the "flow expired" message again when submitting.

I just had to set a non-null value for the price to avoid an error when persisting in order to make the example code work.

I did't take a look at the noextra stuff yet.

[robhunt3r]

Okay I will try, about the noextra stuff, I will upload "soon" (I'm very very busy right now) a full example, it does some weird stuff, but I "fixed" in my project removing some angular/javascript and stuff.

[craue]

@robhunt3r, any news?

[robhunt3r]

@craue sorry we were so busy at work because project needs to launch asap and I forgot about this, I will postit so tomorrow can test and give you feedback.