search

atk4 / ui

Robust and easy to use PHP Framework for Web Apps
https://atk4-ui.readthedocs.io
MIT License
440 stars 105 forks source link

Fix typecasting for TreeView #2022

Closed mkrecek234 closed 10 months ago

mkrecek234 commented 1 year ago

Solves issue #2021

mkrecek234 commented 1 year ago

Fix above works for single value treeItem correctly, however for multiple-value item the typecastLoadField does not support json yet. @mvorisek Can you check please? The sample with multiple-value is not working anymore: https://github.com/atk4/ui/blob/c6b9ed7364e835e4b0d835af7f763f24ae6cf63c/demos/form-control/tree-item-selector.php#L44

mkrecek234 commented 1 year ago

This cannot be right. You cannot mix load/save typecasting direction.

And as posted originally, I would like to have this tested by Behat. Not only to prevent it into the future, but as not tested, it was not fixed yet nor with #1991.

Has to be typecastLoadField and not save which was wrong in the original code. However, the jsonarray data type is not supported, may be you can have a look at it. For integer it is properly working with the initial fix

mvorisek commented 1 year ago

"load" direction is almost surely wrong, as "load" direction implies a value to be loaded from an input, which is probably not the case here

mkrecek234 commented 1 year ago

Valid point - the key solution has to be that scalar values shall not be typecasted (see latest commit), as the Vue component cannot handle ids with thousandSeparator. Still typecasting for non-scalars.

Working and tested now in live setup and demos, sorry behat test has to be provided by another contributor

mvorisek commented 1 year ago

Still the fix is wrong, the ID can be internally stored as any type - see #1991 prooving this case - ID wrapped in an object.

mkrecek234 commented 1 year ago

Would appreciate to see your improved code proposal - the current implementation in develop branch is definitely wrong, and also not working in a defined setup

mvorisek commented 1 year ago

The main question is why is the typecast an issue. Any fix of getValue method will introduce inconsistency with Form\Control\Input::getValue() method. I am currently out of the time to look into this as the proper fix incl. test might take MD easily.

mkrecek234 commented 1 year ago

The main question is why is the typecast an issue. Any fix of getValue method will introduce inconsistency with Form\Control\Input::getValue() method. I am currently out of the time to look into this as the proper fix incl. test might take MD easily.

The Vue TreeItemSelector does not support an Input field value which has thousandSeparator, so ID = 1.750 will not work, even though Atk4 saves and loads it properly. The input field thus has to have a non-typecasted ID inside, otherwise TreeViewItemSelector will not mark the correct item as active.

mkrecek234 commented 1 year ago

The main reason for the above issue is simply that treeItems id values are not typecasted, whereas the currently selected value(s) are typecasted. The Vue component simply then cannot match the correct items. So either we typecast the treeItems node id values or we do not typecast the input current value. In the light of the discussion, as those values are all "hidden" system value, typecasting of integers does not add any value here in my opinion.

mvorisek commented 11 months ago

Let's start with a failing test.