Allow to add() only an object #565
Conversation
mvorisek
force-pushed
the
require_obj_in_add
branch
2 times, most recently
from
2c5d874
to
b8a7a4a
Compare
Codecov Report
@@ Coverage Diff @@
## develop #565 +/- ##
=============================================
- Coverage 86.31% 86.21% -0.11%
+ Complexity 1236 1232 -4
=============================================
Files 28 28
Lines 2733 2727 -6
=============================================
- Hits 2359 2351 -8
- Misses 374 376 +2
Continue to review full report at Codecov.
|
mvorisek
force-pushed
the
require_obj_in_add
branch
5 times, most recently
from
6fd8216
to
606b30a
Compare
mvorisek
force-pushed
the
require_obj_in_add
branch
from
606b30a
to
669dbcc
Compare
There was a problem hiding this comment.
Historically we used add('className') A LOT so this will be a big backward compatibility break :(
I would like to get @romaninsh input here.
| */ | ||
| public function saveAs($class, $options = []) | ||
| public function saveAs(string $class, array $options = []): self |
| */ | ||
| public function newInstance($class = null, $options = []) | ||
| public function newInstance(string $class = null, array $options = []) |
There was a problem hiding this comment.
this will break compatibility. not sure if that's ok.
There was a problem hiding this comment.
That is correct, but to put this project in good shape we need it - as far as I remember these functions were used in a very few places.
| @@ -65,7 +65,7 @@ public function testSaveAs() | |||
|
|
|||
| $m = new Male($p); | |||
| $m->load(1); | |||
| $m->saveAs(new Female()); | |||
| $m->saveAs(Female::class); | |||
There was a problem hiding this comment.
You're right that it's more logical to use class name here not object itself, but this change will break a lot of old code everywhere because historically we used add('className') A LOT :(
There was a problem hiding this comment.
As I said - it looks better, but will break existing projects (again) :(
Can you try to track down @romaninsh and ask what he thinks about this?
There was a problem hiding this comment.
@DarkSide666 I reacted to this as you are writing:
because historically we used add('className')
and this method accepts now exactly this "string className" :)
There was a problem hiding this comment.
Then I guess I added comment in wrong place. It should be for all this PR.
- add() will no longer support add(string $className)
- saveAs() will no longer support saveAs(object $model)
- newInstance() will no longer support newInstance(object $model)
I'm not saying that these changes are bad, but all and every of these are BC breaks...
There was a problem hiding this comment.
Yes and it should be documented like so. This prevents mistakes, allow strong typing and also help the developer to understant what is really does.
Imagine newInstance like new, you never want to allow new (new class())(), it is simply wrong.
mvorisek
force-pushed
the
require_obj_in_add
branch
from
669dbcc
to
48bf0db
Compare
|
This would be one hell of BC break. $foo->add(Some::class); is also extensively used. We could depreciate this and target 4.0 :D |
|
I may be, but as you have already written in Discord, there is a time to move forward and enforce right semantics. Currently the user base is not that great and I strongly advice to prevent bad usage as soon as possible. I propose to create central page with changes, so these changes can be fixed according to that. That is an industry common practice. @romaninsh what will be different in Atk 4.0? My answed: more users, more unwanted usage. Right? |
There was a problem hiding this comment.
Overall this PR is huge BC break, but on the other hand @mvorisek you're right about
what will be different in Atk 4.0? My answed: more users, more unwanted usage. Right?
| @@ -65,7 +65,7 @@ public function testSaveAs() | |||
|
|
|||
| $m = new Male($p); | |||
| $m->load(1); | |||
| $m->saveAs(new Female()); | |||
| $m->saveAs(Female::class); | |||
There was a problem hiding this comment.
Then I guess I added comment in wrong place. It should be for all this PR.
- add() will no longer support add(string $className)
- saveAs() will no longer support saveAs(object $model)
- newInstance() will no longer support newInstance(object $model)
I'm not saying that these changes are bad, but all and every of these are BC breaks...
Plus fix #566