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

HasOneSql is not merging imported field seed properly #1046

Closed
mkrecek234 opened this issue Aug 4, 2022 · 10 comments · Fixed by #1047
Closed

HasOneSql is not merging imported field seed properly #1046

mkrecek234 opened this issue Aug 4, 2022 · 10 comments · Fixed by #1047
Labels

Comments

@mkrecek234
Copy link
Contributor

Steps to reproduce:

  1. You have a model and addExpression which by nature is non-editable.
  2. You for example relate to that model with a hasOne relation, and import that expression as a field

Error TypeError: Atk4\Data\Field::isEditable(): Return value must be of type bool, array returned
in Model:730

The issue is in line 734, as isEditable is not existing as a property for SqlExpressionField.

@mvorisek mvorisek removed the MAJOR label Aug 4, 2022
@mvorisek mvorisek removed their assignment Aug 4, 2022
@mvorisek
Copy link
Member

mvorisek commented Aug 4, 2022

@mkrecek234 please do not add MAJOR to (every) bug, MAJOR is something conceptually wrong or with major impact on most users.

Also, please copy/screenshot full backtrace in case you want to refer to it. Refering to a line number without knowing the file is almost useless. The same for the exception message, as the reason might depend on a context which is shown in exceptions params (added by Exception::addMoreInfo()).

@mkrecek234
Copy link
Contributor Author

image

@mvorisek
Copy link
Member

mvorisek commented Aug 4, 2022

The issue is in line 734, as isEditable is not existing as a property for SqlExpressionField.

There is no isEditable property in atk4/data. There is only Field::isEditable() method, which by inheritance available in SqlExpressionField. Can you please post a minimalistic code to reproduce or is it possible to reproduce it on atk4/ui demos?

@mkrecek234
Copy link
Contributor Author

mkrecek234 commented Aug 4, 2022

Correct - it relates to the missing isEditable() property of SqlExpressionField. In Model::getFields() the following code is not compatible with SqlExpressionField:

                    ($f === 'system' && $field->system)
                    || ($f === 'not system' && !$field->system)
                    || ($f === 'editable' && $field->isEditable())
                    || ($f === 'visible' && $field->isVisible())
                ) {

As I cannot create minimalistic code on demo today, here already the info to reproduce:

  1. Add an expression to any model.
    $model->addExpression('expressionfield', ['expr' => "1"]);

In a child model create a hasOne and import that expression:
$childmodel->hasOne('model_id', ['model' => [Model::class]])->addFields(['expressionfield']);

Then simple check it in a crud:
$crud->setModel($childmodel);

@mkrecek234
Copy link
Contributor Author

I checked further, the issue is that $this->ui['editable']in Field:457 returns an array which includes true and false, rather than just a boolean value.

@mvorisek
Copy link
Member

mvorisek commented Aug 4, 2022

Thank you for the code. I am looking into it.

it relates to the missing isEditable() property of SqlExpressionField

really a property? I seached ->(is)?editable(?!\(), no such property is accessed in the whole project

I belive the issue is caused by ui['editable'] beiing an array -

return $this->ui['editable'] ?? !$this->readOnly && !$this->neverPersist && !$this->system;

@mkrecek234
Copy link
Contributor Author

image

@mvorisek
Copy link
Member

mvorisek commented Aug 4, 2022

yes, doesn't it come from your code?

@mkrecek234
Copy link
Contributor Author

mkrecek234 commented Aug 4, 2022

No, I haven't touched anything there, just standard model definitions

@mvorisek
Copy link
Member

mvorisek commented Aug 4, 2022

issue is here:

$fieldExpression = $this->_addField($fieldName, false, $theirFieldName, array_merge_recursive([

array_merge_recursive is not merging the seed properly - https://3v4l.org/4Tgo8

@mvorisek mvorisek changed the title Bug in Model->getFields in relation to expressions HasOneSql is not merging imported field seed properly Aug 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants