Check previousStep for null

[fritzmg]

Currently a click on the back button within the checkout module might not be recognized if the value of the button is empty for whatever reason, e.g. if you adjusted the template to

<button type="submit" class="submit previous button" name="previousStep"></button>

for example, or if the previousLabel variable is empty for whatever reason. A click on the back button in this case will instead be interpreted as a click on the confirm/next button. This PR changes that by always checking whether the back button was used - by way of checking if Input::post returns null or not.

[aschempp]

not sure why an empty label would be valid?

[fritzmg]

not sure why an empty label would be valid?

The following would be valid and would lead to the same issue:

<button type="submit" class="submit previous button" name="previousStep"><?= $this->previousLabel ?></button>

This sends an empty string for the previousStep POST variable.

[aschempp]

there's a few other places where we do this comparison, could you maybe fix them too? I quickly searched for Input::post() and it found our action buttons for example.

[fritzmg]

Hm, not sure which places these are? Which action buttons do you mean? It only really matters in the checkout module where there is a back button I think.

[fritzmg]

/reminder @aschempp

[fritzmg]

Rebased to 2.9 now.

[aschempp]

I checked this again. Isotope does not generate <button> elements, but <input type="submit">. So I am assuming you changed the markup yourself? You should add <button value="1"> to make sure the button actually submits an input, that should solve the problem, shouldn't it?

[fritzmg]

It does not matter if you use <button> or <input> - and that was not really the point anyway. This issue is not an issue that occurs out of the box with Isotope, only when certain changes were made (intentionally or unintentionally) - see the initial description. The check should always be a null check in this case for defensive programming, otherwise you can end up in a situation where the Back button makes a purchase instead.

[qzminski]

I have approved this change. If the "go back" button is submitted and has no value attribute, then it gets an empty string:

If we hit the "next step" button, then the previousStep is not even in the $_POST, so Input::post() would return null.

[aschempp]

Confirming this with Symfony forms as well: https://github.com/symfony/symfony/blob/4ce4e5ed87acc87d90d7f516b14289c4f49139ef/src/Symfony/Component/Form/SubmitButton.php#L45

I'll update the PR to include all the other places where Isotope does that check.

[aschempp]

Thank you @fritzmg