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

Typecasting not advisable for Links #2015

Closed mkrecek234 closed 6 months ago

mkrecek234 commented 1 year ago

Steps to reproduce:

  1. You have a table and add a Link decorator that uses the integer id and you have thousandSeparator set to . and an id like 123.456.
  2. Click that link.

The id is typecasted in the link, but it should not, $this is a grid:

        $this->addDecorator('order_doc_no', [\Atk4\Ui\Table\Column\Link::class,
            'url' => 'salesorderdoc.php?odaction=edit&odid={$id}']);

Expected result: Open http://salesorderdoc.php?odaction=edit&odid=123456 Current Result: Open http://salesorderdoc.php?odaction=edit&odid=123.456

Otherwise $app->stickyGet('odid') will return a string which it should.

mkrecek234 commented 1 year ago

@mvorisek Just an idea - wouldn't it be helpful if we could pass a parameter to HtmlTemplate to optionally make it render all variables with non-typecasted field values? For Links this would be highly recommended, as you rarely pass a typecasted field value in a URL parameter.

mvorisek commented 1 year ago

HtmlTemplate does not do any typecasting except data are passed as Model which is probably the case here

also are special characters in URL (values) like & escaped properly?

mkrecek234 commented 1 year ago

@mvorisek Do you have any approach to solve this issue? Still I don't think we should render integers typecasted into URLs...

mvorisek commented 1 year ago

if the above is true, the problem I described is more problematic but not so hard to fix :), and your issue should be (in theory, if it does not target other system) irrelevant, as the input should be typecasted with back correctly

https://github.com/atk4/ui/pull/1991 is draft PR to address bidirectional typecasting

mkrecek234 commented 1 year ago

@mvorisek The more I think about it, the more I am convinced that the developer should still have a way to decide if an integer should be formatted or not. Suggestion: We introduce field type atk4_formatted_integer which will get thousandSeparator, wheres standard integer type not.

It is too strict to format all integers, even if you just want money type to have thousandSeparator.

mvorisek commented 1 year ago

Can you please share examples where unformatted integers should be used? I do not say they do not exist, but I am not fully convinced they are stronger. The machine readable data are not a good usecase, as they represent a different domain and different persistence must be used instead.

URL - I agree machine readable format should be used.

mkrecek234 commented 1 year ago

A product EAN barcode for example, a ticket number, a customer no., a serial number;

mvorisek commented 1 year ago

To be able to search them using a barcode reader (which acts as keyboard)? Otherwise SN should have spaces for better UX.

mkrecek234 commented 1 year ago

@mvorisek Any progress on the problematic handling of integer type being typecasted with thousandSeparator, even though the might be used for internal/system purposes? The current solution really is far from ideal in my opinion.

mvorisek commented 1 year ago

I belive the problem (for links) comes from https://github.com/atk4/ui/blob/e0e9b34ae470e63c94ec9415696044c3707d546e/src/Table/Column/Link.php#L146

HtmlTemplate uses ui persistence only when data are passed in Model - perfect.

So we need some machine persistence stored in App. Also review the Link::getHtmlTags, it should not ignore args even if the URL template is passed, but instead of append them.

Unit test must be added into https://github.com/atk4/ui/blob/develop/tests/TableColumnLinkTest.php.