Skip to content

Accept only string|null as template value strictly#1989

Merged
mvorisek merged 5 commits intodevelopfrom
stricter_template
Jan 31, 2023
Merged

Accept only string|null as template value strictly#1989
mvorisek merged 5 commits intodevelopfrom
stricter_template

Conversation

@mvorisek
Copy link
Copy Markdown
Member

@mvorisek mvorisek commented Jan 31, 2023

part of #1987

template cannot judge what persistence/type to use for typecasting

@mvorisek mvorisek marked this pull request as ready for review January 31, 2023 10:48
@mvorisek mvorisek merged commit f288153 into develop Jan 31, 2023
@mvorisek mvorisek deleted the stricter_template branch January 31, 2023 11:03
@mkrecek234
Copy link
Copy Markdown
Contributor

mkrecek234 commented Feb 1, 2023

@mvorisek Why do we need this BC-break? It is very common to run set and just pass any model field to fill in template variables. The BC-break leads to an error because each and every set variable has to be first converted to string.

Use Case:
You have a HTML invoice template and want to set the integer order number.

Before:
$template->set('invoice_no', $model->get('invoice_no'))

Now:
$template->set('invoice_no', (string) $model->get('invoice_no'))

What is the benefit justifying this BC-break?

@mvorisek
Copy link
Copy Markdown
Member Author

mvorisek commented Feb 1, 2023

Please read the PR description and let's continue the discussion in a private chat, as the real impact should be minimal with the best atk4/ui usage.

@mvorisek
Copy link
Copy Markdown
Member Author

mvorisek commented Feb 1, 2023

image

@mkrecek234
Copy link
Copy Markdown
Contributor

Why is PHP-standard typecasting not allowed here where model field is an integer?

\Atk4\Ui\Header::addTo($app, [$model->get('stock')]); 

Simple code, simple PHP typecasting. Do we really want to enforce everyone to code like this?

\Atk4\Ui\Header::addTo($app, [(string) $model->get('stock')]); 

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.

2 participants