PW 3.0.210 broke InputfieldForm.getErrors() with error cache

[da²]

Hello,

@ryan I tried to use in PW 3.0.210 and 3.0.218 a class I wrote in PW 3.0.200 for managing forms, and it's not working anymore.

When I manually set an error on a field, $form->getErrors() returns no error cause it's returning an empty cached array generated with $form->process().

A simple example:

<?php
use ProcessWire\InputfieldForm;
use function ProcessWire\modules;

/** @var InputfieldForm $form */
$form = modules('InputfieldForm');

$form->add($form->InputfieldText->attr('id+name', 'foobar'));
$form->add($form->InputfieldSubmit->attr('name', 'submit_subscribe')->val('Subscribe'));

if ($form->isSubmitted('submit_subscribe')) {
    if ($form->process()) {

        // Simulating a callback method doing extra form validation (for example requesting a URL to verify data).
        $fieldInError = $form->getChildByName('foobar');
        $fieldInError->error("test error");

        echo "<u>form->getErrors():</u> " . count($form->getErrors()) . "<br>"; // BUG: getErrors() == 0
        echo "fieldInError->getErrors(): " . count($fieldInError->getErrors()) . "<br>"; // this is correct, == 1
        echo "form->getErrorInputfields(): " . count($form->getErrorInputfields()) . "<br>"; // this is correct, == 1

        echo $form->render();

    } else {
        echo "<h3>There were errors, please fix</h3>";
        echo $form->render();
    }
} else {
    echo $form->render();
}
?>

$form->getErrors() returns 0 cause it's returning the cache. But even if you call $form->getErrors(clear: true) you are stuck with cache forever, look at InputfieldForm.getErrors():

public function getErrors($clear = false) {
        if($this->errorCache !== null) return $this->errorCache;
        $errors = parent::getErrors($clear);
        $this->errorCache = $clear ? null : $errors;
        return $errors;
    }

Cache cleaning is at line 3, but if we have cache we always stop at line 1.
Maybe we could fix it like this, but I don't get the purpose of this error cache so this code may be problematic in some cases:

public function getErrors($clear = false) {
  if($this->errorCache !== null && !$clear) return $this->errorCache;
  $errors = parent::getErrors($clear);
  $this->errorCache = $clear ? null : $errors;
  return $errors;
}

Actually the easiest fix is to use getErrorInputfields() from InputfieldWrapper, because it doesn't implement error cache.

-------------------------------------------

I also randomly found 2 things about errors being stored somewhere in PW. This cases should not happen but maybe it's worth reporting them.

1. If we don't call $form->render(); after setting the field in error, the error is stored and will be displayed on next page load. Demonstration:

   $form = modules('InputfieldForm');
   
   $form->add($form->InputfieldText->attr('id+name', 'foobar'));
   $form->add($form->InputfieldSubmit->attr('name', 'submit_subscribe')->val('Subscribe'));
   
   if ($form->isSubmitted('submit_subscribe')) {
       $form->getChildByName('foobar')->error("test error");
   } else echo $form->render();

   Click Subscribe, select url bar and hit enter, or open same url in another tab => you see the field in error.

2. If we do a redirection after setting the field in error, the error will also appear in PW admin:
    

   $form = modules('InputfieldForm');
   
   $form->add($form->InputfieldText->attr('id+name', 'foobar'));
   $form->add($form->InputfieldSubmit->attr('name', 'submit_subscribe')->val('Subscribe'));
   
   if ($form->isSubmitted('submit_subscribe')) {
       $form->getChildByName('foobar')->error("test error");
       $session->redirect($page->url);
   } else echo $form->render();

   Click Subscribe, navigate to admin => you see the red error notice:
   
   

-------------------------------------------

And the last thing, getErrors() have different behavior depending if we use $form->process(); or $form->processInput($input->post);.

• Here with process(), getErrors() returns 0. This is caused by process() calling getErrors(clear:false), and getErrors() creating this forever cache. 
   

  $form = modules('InputfieldForm');
  $form->add($form->InputfieldText->attr('id+name', 'foobar'));
  $form->add($form->InputfieldSubmit->attr('name', 'submit_subscribe')->val('Subscribe'));
  
  if ($form->isSubmitted('submit_subscribe')) {
      $form->process();
      $form->getChildByName('foobar')->error("test error");
      echo "<u>form->getErrors():</u> " . count($form->getErrors()) . "<br>"; // ZERO
      echo $form->render();
  
  } else {
      echo $form->render();
  }

   

• And with processInput() it returns 1.
   

  $form = modules('InputfieldForm');
  $form->add($form->InputfieldText->attr('id+name', 'foobar'));
  $form->add($form->InputfieldSubmit->attr('name', 'submit_subscribe')->val('Subscribe'));
  
  if ($form->isSubmitted('submit_subscribe')) {
      $form->processInput($input->post);
      $form->getChildByName('foobar')->error("test error"); // ONE
      echo "<u>form->getErrors():</u> " . count($form->getErrors()) . "<br>";
      echo $form->render();
  
  } else {
      echo $form->render();
  }

 

 

Thanks for reading, I hope it will be useful, I've lost my night on that trying to figure why my code wasn't working even if I used it tens of times on a PW 3.0.200 project. ?

[da²]

Up. @ryan Did you read this?

[matjazp]

Please open an issue report.

[da²]

Yes I'll open it if no answer from Ryan. ?

[ryan]

@da²

Quote

I tried to use in PW 3.0.210 and 3.0.218 a class I wrote in PW 3.0.200 for managing forms, and it's not working anymore.

This way of processing a form with the process() method was introduced in PW 3.0.205, so 3.0.210 would have been the first main/master version to support it. The method wasn't present in 3.0.200, so I'm thinking most likely you were using the processInput() method instead? (example here). But it may not matter, as the process() method is just a front-end to the processInput() method to provide a more convenient API usage, as it returns a true or false as to whether the form was processed without errors. But that distinction is significant for your example. 

It assumes processing the form is a one-time thing and that you aren't going to manually add errors to the form once it has successfully processed, as identifying errors is part of processing. In your case, you are doing a secondary validation manually, adding errors after the form has processed. That's new to me, but if you find it useful then I'm all for supporting it. 

With regard to "forever cache", the cache is a request cache, not a forever cache. It is a runtime memory-only cache for the one current request, as presumably the same exact form instance won't be processed multiple times in the same request (or at least shouldn't be). 

The reason for the cache value is that it has to recursively iterate through every single Inputfield in the form to ask each whether it had any errors. For a large form, it can be enough overhead to warrant caching that information, as the method may get called multiple times in the request and have to repeat the same task. But is it worthwhile enough? Maybe not, given your use case. I'll look into dropping it.

Quote

If we don't call $form->render(); after setting the field in error, the error is stored and will be displayed on next page load.

When you set an error, it will remain in the session until it is displayed. This is because redirects are often involved in forms, in order to prevent re-post by re-load. Form errors would be lost if they weren't remembered until used. 

Quote

And the last thing, getErrors() have different behavior depending if we use $form->process(); or $form->processInput($input->post);.

This is because the process() method determines if there are errors and returns true (no errors) or false (errors). The processInput() method does not tell you that information and so you'd have to check for errors manually afterwards. So your first example is checking for errors BEFORE you have manually added an error to it (which is what the process method does, and sets that cache), while your second example checks for errors AFTER you have manually added an error. This is why the results are different. 

Unrelated, but I recommend putting "<?php namespace ProcessWire;" (or any namespace) at the top of your template file, OR setting $config->templateCompile = false; in your /site/config.php file, just so that PW doesn't attempt to unnecessarily compile your template files. Maybe you are already doing this, but just mentioning it just in case. 

[da²]

OK, thanks for the explanations, I understand better your point of view.

1 hour ago, ryan said:

This way of processing a form with the process() method was introduced in PW 3.0.205, so 3.0.210 would have been the first main/master version to support it. The method wasn't present in 3.0.200, so I'm thinking most likely you were using the processInput() method instead?

Yes this project is 3.0.200 using processInput(), here is the class I used, it's basically just a wrapper over InputfieldForm. Since then, I updated this class, because I saw you updated it in the same way (to make InputfieldForm API easier).

abstract class TemplateFormManager
{
    /** @var Inputfield $form */
    private $form;
    private string $submitFieldName;

    function __construct(
        array $defaultValues = null,
        string $formAction = "./",
        string $method = FormMethod::POST,
        bool $showSubmit = true,
        string $submitFieldName = "submit_form"
    ) {
        $this->submitFieldName = $submitFieldName;

        $this->form = modules('InputfieldForm');
        $this->buildForm($this->form);
        $this->applyDefaultValues($defaultValues);
        if ($showSubmit)
            $this->addSubmit();
        $this->form->action = $formAction;
        $this->form->method = $method;
    }

    function render(): string
    {
        return $this->form->render();
    }

    function isSubmitted(): bool
    {
        $submitName = $this->form->method == FormMethod::POST ?
            wire()->input->post($this->submitFieldName) :
            wire()->input->get($this->submitFieldName);

        return $submitName != null;
    }

    function processInput()
    {
        $this->form->processInput(
            $this->form->method == FormMethod::POST ?
                wire()->input->post :
                wire()->input->get
        );
    }

    function getErrorCount(): int
    {
        return count($this->form->getErrors());
    }

    function setError(string $fieldName, string $error)
    {
        $field = $this->form->get($fieldName);

        if (!$field) {
            throw new InvalidArgumentException("Field '$fieldName' doesn't exist!");
        }

        $field->error($error);
    }

    /**
     * Get value of a Inputfield.
     */
    function __get($fieldName)
    {
        $field = $this->form->get($fieldName);

        if (!$field) {
            throw new InvalidArgumentException("Field '$fieldName' doesn't exist!");
        }

        return $field->attr('value');
    }

    function getField(string $fieldName): Inputfield
    {
        $field = $this->form->get($fieldName);

        if (!$field) {
            throw new InvalidArgumentException("Field '$fieldName' doesn't exist!");
        }

        return $field;
    }

    protected abstract function buildForm(Inputfield $form);

    private function applyDefaultValues(?array $values)
    {
        if ($values) {
            foreach ($values as $fieldName => $value) {

                $field = $this->form->get($fieldName);

                if (is_a($field, "ProcessWire\InputfieldCheckbox", true))
                    $field->checked($value);
                else
                    $field->attr('value', $value);
            }
        }
    }

    private function addSubmit()
    {
        $submit = modules('InputfieldSubmit');
        $submit->attr('name', $this->submitFieldName);
        $submit->value = "Valider";

        $this->form->add($submit);
    }
}

 

Next one is my current version (PW 3.0.210), you can look at the execute() method, this is where I use the callback method onValidate() to do extra validation in template file. In the previous version above it was coded in a function executeForm() outside of abstract form class.

On getErrorCount() method, I'm smiling at my comment "Strange method that gave me strange bugs, I just understand nothing..." ?

abstract class AbstractForm
{
    protected readonly InputfieldForm $form;
    protected readonly string $submitFieldName;

    public function __construct(
        array  $defaultValues = null,
        string $formAction = "./",
        string $method = FormMethod::POST,
        bool   $showSubmit = true,
        string $submitFieldName = "submit_form"
    )
    {
        $this->submitFieldName = $submitFieldName;

        /** @var InputfieldForm $form */
        $form = modules('InputfieldForm');
        $this->form = $form;

        $this->buildForm($this->form);
        if ($showSubmit)
            $this->addSubmit();
        $this->form->populateValues($defaultValues);
        $this->form->action = $formAction;
        $this->form->method = $method;
    }

    public function render(): string
    {
        return $this->form->render();
    }

    public function execute(
        callable $successCallback = null,
        callable $validateCallback = null,
    ): int
    {
        if ($this->isSubmitted()) {
            $this->form->process();

            // Check errors managed by form.
            $errorCount = $this->getErrorCount();
            if ($errorCount > 0) return $errorCount;

            // Do extra validation.
            if ($validateCallback) $validateCallback($this);

            // Check errors after our extra validation.
            $errorCount = $this->getErrorCount();
            if ($errorCount > 0) return $errorCount;

            if ($successCallback) $successCallback($this);
        }


        return 0;
    }

    /**
     * Strange method that gave me strange bugs, I just understand nothing...
     */
    public function getErrorCount(): int
    {
        return count($this->form->getErrorInputfields());
    }

    public function setError(string $fieldName, string $error): void
    {
        $field = $this->form->getChildByName($fieldName);

        if (!$field) {
            throw new InvalidArgumentException("Field '$fieldName' doesn't exist!");
        }

        $field->error($error);
    }

    /**
     * Get value of an Inputfield.
     */
    public function __get(string $fieldName): mixed
    {
        $value = $this->form->getValueByName($fieldName);

        if ($value === null)
            throw new InvalidArgumentException("Field '$fieldName' doesn't exist!");

        return $value;
    }

    public function setValue(string $fieldName, string $value): void
    {
        /** @noinspection PhpUnhandledExceptionInspection */
        $field = $this->form->get($fieldName);

        if (!$field)
            throw new Error("Field $fieldName it not in form.");

        $field->attr('value', $value);
    }

    protected abstract function buildForm(InputfieldForm $form): void;

    protected function isSubmitted(): bool
    {
        /** @noinspection PhpUnhandledExceptionInspection */
        return $this->form->isSubmitted($this->submitFieldName);
    }

    private function addSubmit(): void
    {
        try {
            /** @var InputfieldSubmit $submit */
            $submit = wire()->modules->get('InputfieldSubmit');
            $submit->attr('name', $this->submitFieldName);
            $submit->value = __("Valider", COMMON_TRANSLATION_DOMAIN); // Traduire dans _commonTranslations.php

            $this->form->add($submit);

        } catch (WirePermissionException $e) {
            throw new Error($e->getMessage());
        }
    }
}

I use it like that in a template:

$onValidate = function (AbstractForm $form): void {
  // Calling a remote API to valid some data

  // We got an error
  $form->setError('fieldName', "Don't do that please");
};

$onSuccess = function (AbstractForm $form): void {
    // Saving user and redirecting to another page
};

// Form default values
$currentValues = [
    'foo' => $bar
];

$form = new AccountEditionForm($currentValues);
$formErrorCount = $form->execute($onSuccess, $onValidate);

Twig::render('account-edition', [
    'form' => $form,
    'formErrorCount' => $formErrorCount
]);

 

1 hour ago, ryan said:

That's new to me, but if you find it useful then I'm all for supporting it. 

Thank you. Yes this is often necessary. I'm checking errors that are outside of pure form context.

Another user had this same issue since 3.0.210, I'm not alone doing this. ? 

 

1 hour ago, ryan said:

Unrelated, but I recommend putting "<?php namespace ProcessWire;" (or any namespace) at the top of your template file, OR setting $config->templateCompile = false; in your /site/config.php file, just so that PW doesn't attempt to unnecessarily compile your template files. Maybe you are already doing this, but just mentioning it just in case. 

Sometimes I add the namespace, but just to get cleaner imports. I didn't know there is a relation with template compilation, could you explain what's the problem of having my template compiled by PW?

I'm gonna add it in PHPStorm template file, so I don't forget it.

EDIT: in fact $config->templateCompile was already false by default.

[ryan]

@da² Looks like a good form class. I experimented with removing that cache in InputfieldForm, but found that it was worthwhile to keep as it gets called 5 or 6 times in page editor testing, and removing it adds more overhead than I'd like. So instead I updated it so that $form->getErrors(null) forces it to re-check all the fields, clearing/bypassing that internal cache. So in your form example, you'd call $form->getErrors(null) rather than $form->getErrors(), and then it should do what you were expecting. https://github.com/processwire/processwire/commit/b77d7f98c64edda4d49b2415e092e804e4c37e70

Quote

could you explain what's the problem of having my template compiled by PW?

There is no problem with it. It takes care of figuring out namespace issues and compiles a new version of the file that resolves them. Since you were using 'use' statements at the stop, I was figuring you didn't want it adding more namespace logic behind the scenes. But since you've got templateCompile=false; then you're all set. 

[da²]

22 minutes ago, ryan said:

So instead I updated it so that $form->getErrors(null) forces it to re-check all the fields

That looks good if online documentation has a note on it. ?

Thanks for your commitment. ?