[fix] Multiline #1631
[fix] Multiline #1631
Conversation
Align with latest data changes
| } | ||
| } | ||
| $id = $row_model->save()->getId(); | ||
| $id = $entity->save()->getId(); |
There was a problem hiding this comment.
save is outside foreach, is that intended?
otherwise feel free to merge it
There was a problem hiding this comment.
Yes - the intention is to collect all record values for one row into the for loop and save after.
There was a problem hiding this comment.
but a few lines above there is a load... related with https://github.com/atk4/ui/pull/1631/files#r628487155
There was a problem hiding this comment.
bad explanation from me.
The data collect from load contains all rows and each row has one set of record.
The second for loop is too loop for each field and check if the current field is editable prior to set is value.
Once all fields are set within the row, then the record is save.
Hope it is clearer now...
Maybe we could use $entity->save($row) directly but I am not sure how Data will react if trying to pass a field => value set to a non editable field... This is were my knowledge of Data fails 😄 Multiline will display the non editable field as readonly input.
There was a problem hiding this comment.
AFAK this relies on :N typecasting from single cell value which... Which at least is not good to be saved n times when looping.
feel free to merge without my review once you are happy with the changes ;-)
src/Form/Control/Multiline.php
Outdated
| if ($fieldName === $row_model->id_field && $value) { | ||
| $row_model = $row_model->load($value); | ||
| if ($fieldName === $model->id_field && $value) { | ||
| $entity = $model->load($value); |
There was a problem hiding this comment.
Theoretically the id_field could be anywhere in the array, couldn't it? So it could be that some values are already set to the entity by the rows below ($entity->set($fieldName, $value), and then $entity->load($value) comes after. That could overwrite values already set by $entity->set($fieldName, $value).
I think load() should be the first thing happening, any set() should happen after.
There was a problem hiding this comment.
you are right - I guess it never breaks before probably because id is generally the first field in a row. I will make changes. Good catch!
| @@ -420,13 +433,15 @@ public function setModel(Model $model, array $fieldNames = []): Model | |||
| * Otherwise, form will try to save 'multiline' field value as an array when form is save. | |||
| * $multiline = $form->addControl('multiline', [Multiline::class], ['never_persist' => true]) | |||
| */ | |||
| public function setReferenceModel(Model $refModel, string $linkByFieldName, array $fieldNames = []): Model | |||
| public function setReferenceModel(string $refModelName, Model $modelEntity = null, array $fieldNames = []): Model | |||
There was a problem hiding this comment.
search setReferenceModel, docs needs to be adjusted.
In general, do we need to pass Model at all?
There was a problem hiding this comment.
Doc updates. In general, the Model entity will be set using the model set in Form. But there might be cases where you need to pass it with the method.
Align with latest data changes.
BC-BREAK
Multiline->setReferenceModel() method parameters has changed.
You now have to passed the reference name and the model entity to where reference apply: