Fix strict types in App::getTag()

[PhilippGrashoff]

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:

[mvorisek]

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

[mvorisek]

closing as no response

[PhilippGrashoff]

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.

[mvorisek]

reopened, but keep in mind that I will be very against solving this like you propose ;-)

[PhilippGrashoff]

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.

[mvorisek]

revert your fix and update /demos/collection/table.php demo to replicate

[mkrecek234]

Have the same issue with a simple model with field of 'type'=>'money'. The following table throws the same error as above:
`$regiontable = \atk4\ui\Table::addTo($app);
$regiontable->setModel($groupmodelsplit, false);

$regiontable->addColumn('continent');
$regiontable->addColumn('turnover_LTM', [\atk4\ui\Table\Column\Money::class]);

$regiontable->addTotals(['continent' => 'Totals:', 'turnover_LTM' => ['sum']]);`

[mvorisek]

@mkrecek234 please submit a PR with modified demo with this and only this isses, I will look/fix

[ibelar]

@PhilippGrashoff - You should probably type cast the value inside Table\Column\getTotalsCellHtml() method:

    public function getTotalsCellHtml(Field $field, $value)
    {
        return $this->getTag('foot', (string) $this->getApp()->ui_persistence->typecastSaveField($field, $value));
    }

The bug can be reproduce inside /demos/collection/table.php when changing the 'salary' field from money to integer type.

[PhilippGrashoff]

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.

[mvorisek]

@PhilippGrashoff does this solve your issue?

[PhilippGrashoff]

Hi, will test, but looks like the proper place to fix this.

[mvorisek]

@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?

[mvorisek]

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.

[mvorisek]

Sqlite hovewer supports zero - https://dbfiddle.uk/?rdbms=sqlite_3.27&fiddle=f2d14bf69f3a8d2ae85b3ff07703c9fa

[mvorisek]

with last fix I think we can keep "required" (and thus reject zero IDs)

/cc @DarkSide666 @georgehristov