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

Field names missing in Model->getField, causing corrupt Exception message #1704

Closed
mkrecek234 opened this issue Nov 24, 2021 · 15 comments · Fixed by #1711
Closed

Field names missing in Model->getField, causing corrupt Exception message #1704

mkrecek234 opened this issue Nov 24, 2021 · 15 comments · Fixed by #1711

Comments

@mkrecek234
Copy link
Contributor

mkrecek234 commented Nov 24, 2021

In recent develop, example demos/form/form2.php don't work:

  • First form can never be saved, as iso cannot be empty, nor cannot it be changed
  • If you deactivate all validation handling in onSubmit to make it work again, still it will throw an error Javascript Error Syntax error, unrecognized expression: #

The second form throws error Atk4\Ui\Exception: Callback requested, but never reached. You may be missing some arguments in request URL. on save.

@mvorisek
Copy link
Member

I have already seen Javascript Error Syntax error, unrecognized expression: # somewhere during other issue debugging, this is definitely a bug that must be fixed.

@mvorisek
Copy link
Member

@ibelar can you please look at the JS issue? It seems something is not escaped properly or deescaped/evaluated twice.

@mvorisek mvorisek added the MAJOR label Nov 24, 2021
@mkrecek234
Copy link
Contributor Author

More background @ibelar : For me I get the Javascript Error Syntax error, unrecognized expression: # always when saving a form which is placed in a Modal Virtual Page. Help appreciated as this is a major new bug

@mvorisek
Copy link
Member

@mkrecek234 can you identify which atk4/ui commit started producing this issue?

@ibelar
Copy link
Contributor

ibelar commented Nov 29, 2021

This is likely du to Data\Field error and badly formatted ValidationException throw when Field::$name is equal to null and has nothing to do with javascript.

see: https://github.com/atk4/data/blob/develop/src/Field.php#L282

Perhaps a regular exception should be thrown when $name property is null.

Otherwise the ValidationException will try to execute this javascript expression :
$("#atk_layout_maestro_form").form("add prompt","","Must not be null")

Which end up in showing the error message above because of the missing field name.

@mkrecek234 : Thanks for pointing out the error form2.php error. I will issue a fix for it. When you see this error, please check for Exception within your model.

@mvorisek
Copy link
Member

iso cannot be empty because of the model definition

the main issue is the JS escaping

@mkrecek234
Copy link
Contributor Author

@ibelar By removing the form->save() in the sample form2.php you circumvent this bug. There is an issue saving the form. We should keep real form-> save samples in our demos to demonstrate (and also have real life tests). Can you please check? In fact, this is a new bug that came with the last develop updates.

@mkrecek234
Copy link
Contributor Author

@mvorisek @ibelar I found the reason for my issue: The Javascript Error Syntax error, unrecognized expression: # actually is caused in my case by a thrown Exception in the protected function _typecastSaveField(Field $field, $value)in Persistence.php at https://github.com/atk4/data/blob/975b27f4de756118860da069ffff103633cc7f3a/src/Persistence.php#L271

The Exception however is not properly shown, but then the above Javascript Error is shown only. Can you check how to make sure that thrown Exception are properly displayed and not with the unrecognized expressions?

@ibelar
Copy link
Contributor

ibelar commented Dec 1, 2021

The error message shown in form2.php happen because there is an exception thrown during the validation process.
this line in init-db: $c = $this->getModel()->tryLoadBy($this->fieldName()->name, $this->name); is throwing an Exception.

This Exception is later caught within Form::onSubmit() method, which generate a Fomantic form('add prompt') message:

    public function onSubmit(\Closure $callback)
    {
        $this->onHook(self::HOOK_SUBMIT, $callback);

        $this->cb->set(function () {
            try {
                $this->loadPost();
                $response = $this->hook(self::HOOK_SUBMIT);

                if (!$response) {
                    if (!$this->model instanceof \Atk4\Ui\Misc\ProxyModel) {
                        $this->model->save();

                        return $this->success('Form data has been saved');
                    }

                    return new JsExpression('console.log([])', ['Form submission is not handled']);
                }

                return $response;
            } catch (\Atk4\Data\ValidationException $val) {
                $response = [];
                foreach ($val->errors as $field => $error) {
                    $response[] = $this->error($field, $error);
                }

                return $response;
            }
        });

        return $this;
    }

What is happening is that $val->errors does not contains any field name, thus generating a bad Fomantic add prompt instruction using an empty field name. The add prompt generated is: $("#atk_layout_maestro_form").form("add prompt", "", "Must not be null")

When running this instruction return by the server, Fomantic throw an exception, because of badly formatted add prompt message, which is catch by apiService and display within a modal.

This error has nothing to do with Javascript, it simply catch a badly formatted add prompt message.

I think to correctly fix this, there should be a distinction between a real validation exception and an regular exception throw within the validate method. And I think this should be resolve on the Atk4\Data side. @mvorisek
what do you think?

@mvorisek
Copy link
Member

mvorisek commented Dec 1, 2021

we have to throw an exception uniformly with other validation exceptions, not sure what you mean exactly, please submit a PR if you think something behave wrongly

@mkrecek234 mkrecek234 changed the title Data-driven forms don't save Field names missing in model->save, causing corrupt Exception message Dec 2, 2021
@mkrecek234
Copy link
Contributor Author

@mvorisek @ibelar I investigated this further. The issue indeed is that the field name is empty for all fields (short_name though is set) coming from this function: https://github.com/atk4/data/blob/975b27f4de756118860da069ffff103633cc7f3a/src/Persistence.php#L159

With the missing name though, any validation failure will also miss the field name, which ultimately creates the JsExpression error. @mvorisek may be you have an idea how to correct the $field = $model->getField($fieldName); to fill also the name in the array?

@mkrecek234 mkrecek234 changed the title Field names missing in model->save, causing corrupt Exception message Field names missing in Model->getField, causing corrupt Exception message Dec 2, 2021
@mvorisek
Copy link
Member

mvorisek commented Dec 2, 2021

@mkrecek234 thanks for envestigation, let me know which array, eg. what do you expect exactly on which line, I think I can help quickly then ;-)

@mkrecek234
Copy link
Contributor Author

mkrecek234 commented Dec 2, 2021

@mvorisek on the line under the link above, $row still has complete field elements with name in it.
The command $field = $model->getField($fieldName) then returns an array which has name empty, but some other like short_name are there. So if you adjust getField to fill the array including the name, it should be alright.

@mvorisek
Copy link
Member

mvorisek commented Dec 2, 2021

I belive only short_name should be used and all other names should ideally throw an exception (we can unset easily, then it will emit an undefined warning due special trait from atk4/core) :)

@mkrecek234
Copy link
Contributor Author

Your take @mvorisek - the missing name leads to

                $response = [];
                foreach ($val->errors as $field => $error) {
...```

be rendered with $field being empty.

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

Successfully merging a pull request may close this issue.

3 participants