atk4 / filestore

Implements integration between ATK UI and FlySystem
https://agiletoolkit.org/
MIT License
9 stars 8 forks source link

Demo throwing error with latest Atk4/Data #44

Closed mkrecek234 closed 9 months ago

mkrecek234 commented 9 months ago

Steps to reproduce:

Critical Error Atk4\Data\Exception: Reference type mismatch
Exception Parameters
--
ourField | Atk4\Data\Field (file)
ourFieldType | 'integer'
theirField | Atk4\Data\Field (token)
theirFieldType | 'string'

in line https://github.com/atk4/filestore/blob/78009f73d333fe5422fa5a2e23120658c120c759/src/Field/FileField.php#L83

mkrecek234 commented 9 months ago

Can be fixed by explicitly setting the field type to string as tokens are always strings in filestore: https://github.com/atk4/filestore/blob/78009f73d333fe5422fa5a2e23120658c120c759/src/Field/FileField.php#L45

        $this->reference = HasOneSql::assertInstanceOf($this->getOwner()->hasOne($this->shortName, [
            'type' => 'string',
            'model' => $this->fileModel,
            'theirField' => 'token',
        ]));

Or if we want to be more generic with token types, we set:

'type' => $this->fileModel->getField('token')->type

Happy to submit a PR if welcomed.

mkrecek234 commented 9 months ago

@mvorisek Strictly this change should not be needed, so I wonder if the underlying atk4/data change is correct: every hasOne relation by definition has the same type as its source, so it should not be needed to declare it (as it was before). Any idea?

mvorisek commented 9 months ago

The check in atk4/data is great as it prevents you joining different types. Like datetime and int. Also see https://github.com/atk4/data/pull/1149, even string and int mismatch cause non-equaliality in some DBs even when the string type has numeric value.

If you have any ideas, propose a PR :) This seems easy to fix.

mkrecek234 commented 9 months ago

The check is great - I was just surprised that the hasOne has now an integer type wheres shouldn't it automatically get the type of the source field without explicitly mentioning it? Solution you see above - will commit as PR if you are OK

mvorisek commented 9 months ago

fixed by #45