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

Update FormFlow.php #235

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Update FormFlow.php #235

wants to merge 1 commit into from

Conversation

robhunt3r
Copy link

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.

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
Copy link

Coverage Status

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

@coveralls
Copy link

Coverage Status

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

@coveralls
Copy link

Coverage Status

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

@craue
Copy link
Owner

craue commented Mar 22, 2016

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
Copy link

Coverage Status

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

@robhunt3r
Copy link
Author

@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
Copy link
Owner

craue commented Mar 22, 2016

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
Copy link
Author

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
Copy link
Owner

craue commented Mar 22, 2016

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
Copy link
Author

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
Copy link
Owner

craue commented Mar 23, 2016

@Nairebis, do you have an opinion on this?

@craue
Copy link
Owner

craue commented Mar 23, 2016

@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
Copy link
Author

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
Copy link
Contributor

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
Copy link
Author

@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
Copy link
Owner

craue commented Apr 21, 2016

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
Copy link
Author

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
Copy link
Owner

craue commented Apr 21, 2016

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. 😏

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
Copy link
Author

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
Copy link
Owner

craue commented May 25, 2016

@robhunt3r, any news?

@robhunt3r
Copy link
Author

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants