Fix strict types in App::getTag()#1541
Conversation
|
we want to AVOID explicit casts as much as possible - you should cast to string on data side - so no from my side post usecase/steps to reproduce |
|
closing as no response |
|
can you please reopen this one? I am quite busy and will provide a test case. Just closing an issue after two days isn't very polite. |
|
reopened, but keep in mind that I will be very against solving this like you propose ;-) |
|
Thanks for reopening. Well, there has to be some typecast to string somewhere it seems. Its logical that table totals are int/float, and somewhere along the stack trace they need to by cast to string. I didn't find any demo with numeric table totals, otherwise I'd guess they would fail. |
|
revert your fix and update |
|
Have the same issue with a simple model with field of 'type'=>'money'. The following table throws the same error as above: $regiontable->addColumn('continent'); $regiontable->addTotals(['continent' => 'Totals:', 'turnover_LTM' => ['sum']]);` |
|
@mkrecek234 please submit a PR with modified demo with this and only this isses, I will look/fix |
|
@PhilippGrashoff - You should probably type cast the value inside The bug can be reproduce inside /demos/collection/table.php when changing the 'salary' field from money to integer type. |
Typecast to string in getTotalsCellHtml()
|
Well, the question is if we use this fix or start refactoring Persistence/UI now. WDYT? We definitely should fix this error on Table totals soon. |
c53fef8 to
060aee3
Compare
6d4914c to
92f89dd
Compare
|
@PhilippGrashoff does this solve your issue? |
3f57802 to
508debb
Compare
|
Hi, will test, but looks like the proper place to fix this. |
This reverts commit 53187fb.
| public function setSource(array $data, $fields = null) | ||
| { | ||
| // ID with zero value is not supported, fix the data | ||
| if (isset($data[0])) { |
There was a problem hiding this comment.
@georgehristov because of atk4/data#806 when validation is triggered.
I am not sure with this. We can switch to mandatory or allow zero IDs for non SQL persistences only.
Do you know, if zero as ID is actually a problem with some DB engine?
There was a problem hiding this comment.
I was able to reproduce http://sqlfiddle.com/#!9/d28469/1 - at least with MySQL zero is understood as empty ID and AI is applied.
There was a problem hiding this comment.
Sqlite hovewer supports zero - https://dbfiddle.uk/?rdbms=sqlite_3.27&fiddle=f2d14bf69f3a8d2ae85b3ff07703c9fa
There was a problem hiding this comment.
with last fix I think we can keep "required" (and thus reject zero IDs)
563cc28 to
faf1f6a
Compare
faf1f6a to
1724f83
Compare
App::encodeHTML() expects a string as parameter. In App::getTag() its called, and I was able to produce a type error by adding totals to a table.
Here is the fixed version with typecast, note the marked zeros, they are what causes the issue with non-fixed version. The last row is added by Table::addTotals(), which calls App::getTag();
Without the fix, this error is produced: