-
Notifications
You must be signed in to change notification settings - Fork 106
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Convert hardcoded strings in demos to hintable fields #1559
Conversation
c7e040e
to
88474aa
Compare
7ff7e3e
to
6eb73ae
Compare
@DarkSide666 @georgehristov @ibelar please review, very powerfull lib I released for atk4. For now, I am adding it as a dev dependency ie. for demos only I will then adjust everything in demos to make it analyseable/typehintable/refactorable. |
@mvorisek - I think this is fine but would be nice to have a bit more info in the readme file there just to understand what is that library doing and why is it needed: https://github.com/mvorisek/atk4-hintable-mirror Edit: found notes here, reading: https://github.com/mvorisek/atk4-hintable-mirror/blob/develop/src/Data/HintableModel.php#L14 |
@mvorisek - why not bring magic properties natively into ATK Model ? |
@mvorisek I would also suggest adding native trait |
updated PR title
I need more control over this than waiting days until someone merge something. This is why I also removed the PRs for it a few months ago.
There are two issues with trait:
So I can change the functionality to trait and offer it also under If welcomed, I can add it also as atk4/data dep (so it does not have to be required in any other project) and possibly annotate |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
22c6a49
to
144fd01
Compare
144fd01
to
cf23bc0
Compare
cd8ff34
to
f2cce7c
Compare
Is there a specific reason to prefix table field name using 'xxx' or 'yyy'? Why not using the table name instead like 'country_name', 'stats_name' etd? |
The reason I added is solely to identify direct usage of original/actual DB field names.
Beatifulness addressed in atk4/data#831 |
@@ -234,7 +234,7 @@ protected function getPathWithAppVars(string $path): string | |||
|
|||
public function demoFilesProvider(): array | |||
{ | |||
$excludeDirs = ['_demo-data', '_includes', '_unit-test', 'special']; | |||
$excludeDirs = ['_demo-data', '_includes']; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -84,7 +84,7 @@ public function getQuery() | |||
public function setModelCondition(Model $model): Model | |||
{ | |||
if ($q = $this->getQuery()) { | |||
$model->addCondition('name', 'like', '%' . $q . '%'); | |||
$model->addCondition($model->title_field, 'like', '%' . $q . '%'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
was a bug
Preface
The hintable support is another major game changer in sense of cleaness and maintainability. For now, adding it to demos only and it is up too every user to use it or not.
Description
Annotate model fields and used them magically as a real properties.
Usage
standard ("value") property
$model->is_folder
===$model->get('is_folder')
$model->is_folder = false
===$model->set('is_folder', false)
"name" property
$model->fieldName()->is_folder
==='is_folder'