Skip to content

Impl. typecasting for non-display data#1991

Merged
mvorisek merged 13 commits intodevelopfrom
fix_id_typecasting
Feb 18, 2024
Merged

Impl. typecasting for non-display data#1991
mvorisek merged 13 commits intodevelopfrom
fix_id_typecasting

Conversation

@mvorisek
Copy link
Copy Markdown
Member

@mvorisek mvorisek commented Feb 6, 2023

continue #2168

HTML attributes like data should be formatted "more using machine readable format".

fix #2015

@mvorisek mvorisek force-pushed the fix_id_typecasting branch 3 times, most recently from 117c8ba to fc9e662 Compare February 7, 2023 10:27
@mvorisek mvorisek force-pushed the fix_id_typecasting branch 5 times, most recently from af83863 to c0ea0ca Compare February 7, 2023 22:43
@mvorisek mvorisek force-pushed the fix_id_typecasting branch 5 times, most recently from b0a8761 to 2fe5ad6 Compare February 10, 2023 12:41
@mvorisek mvorisek force-pushed the fix_id_typecasting branch 12 times, most recently from 307378e to cbdbb6c Compare February 7, 2024 12:39
@mvorisek mvorisek force-pushed the fix_id_typecasting branch 7 times, most recently from 3f03674 to c8db488 Compare February 13, 2024 13:10
@mvorisek mvorisek force-pushed the fix_id_typecasting branch 2 times, most recently from 1a35de8 to 631a307 Compare February 14, 2024 16:39
@mkrecek234
Copy link
Copy Markdown
Contributor

mkrecek234 commented Feb 19, 2024

Hi @mvorisek Sorry for not being able to review within the last 2 days - I see an issue already with this, as multiple Dropdowns are rendering values wrongly ignoring fully and custom renderRowFunction. E.g. You have a dropdown with this renderRow function:

        $this->renderRowFunction = function(\Atk4\Filestore\Model\File $row) {

            return [
                'value' => $row->get('token'),
                'title' => $row->get('meta_filename')
            ];
        };

Then, this line will return the idFieldeven though you want value to be rendered individually as being token:

$this->_tItem->set('value', $this->getApp()->uiPersistence->typecastAttributeSaveField($this->model->getField($this->model->idField), $row->getId()));

It has to be $res['value'] and not $row->getId() - otherwise there is no point in returning values in a renderRowFunction anymore. I would prefer to remove this BC break as it is not really needed and limits the freedom of the developer to customize.

@mvorisek
Copy link
Copy Markdown
Member Author

mvorisek commented Feb 19, 2024

@mkrecek234 that was introduced in 4b334a3 (#2168) and related #2165

please submit a separate issue for it with a) link to this post, b) minimal self containing repro, c) expected behaviour/fix, and if you can, PR is welcomed.

@mkrecek234
Copy link
Copy Markdown
Contributor

mkrecek234 commented Feb 19, 2024

@mvorisek You certainly had reasons why you wanted to drop the possibility to adjust value which I would object, as there are use cases where it is helpful - so it mainly is to revert your changes with regard to not allowing renderRowFunction for models to set value back to where it was before.

Can we keep it simple and adjust your unreviewed code like this:

        if ($this->model !== null) {
            $res = ($this->renderRowFunction)($row);
            $this->_tItem->set('value', array_key_exists('value', $res) ? $res['value'] : $this->getApp()->uiPersistence->typecastAttributeSaveField($this->model->getField($this->model->idField), $row->getId()));
        } else {
            $res = ($this->renderRowFunction)($row, $key); // @phpstan-ignore-line https://github.com/phpstan/phpstan/issues/10283#issuecomment-1850438891
            $this->_tItem->set('value', (string) $res['value']); // @phpstan-ignore-line https://github.com/phpstan/phpstan/issues/10283
        }

@mvorisek
Copy link
Copy Markdown
Member Author

Thank you for pointing the issue out. The reason is anything rendered in a custom renderer is wrong as it needs to be processed/parsed when selected - this time, using non-custom parser. So there is more going on and it is your job to do this.

@mkrecek234
Copy link
Copy Markdown
Contributor

I adjusted my own code writing a custom parser already so if there is no interest by you or others to keep the old, easy functionality and you are not agreeing on the simplified adjustment, I'm fine keeping it as is.

@mvorisek
Copy link
Copy Markdown
Member Author

Michael, developing a framework is about pursuing robust, logical but easy to use design. If you think there is something to improve, please open a separate issue.

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

Labels

Development

Successfully merging this pull request may close these issues.

Typecasting not advisable for Links

2 participants