Add central/singleton Factory and deprecate FactoryTrait trait#221
Add central/singleton Factory and deprecate FactoryTrait trait#221
Conversation
972d269 to
90216fc
Compare
2d9503e to
6491150
Compare
a2ec463 to
72c69cc
Compare
e2a9803 to
20cf93f
Compare
5bec9f4 to
9a4817b
Compare
d23ce41 to
5da2218
Compare
42e0087 to
3fa4b31
Compare
3fa4b31 to
18cd07c
Compare
fc78823 to
8bd3aef
Compare
ffaf5c9 to
96cff5f
Compare
049e3e8 to
6b0478b
Compare
|
This one I definitely would like @romaninsh to review too. |
romaninsh
left a comment
There was a problem hiding this comment.
Agree that this is not a BC, however updating other core classes to use new Factory trait would cause BC..
It's better if we plan it and suggest users how to update their code.
src/Core.php
Outdated
| /** | ||
| * See \atk4\core\Factory::factory(). | ||
| */ | ||
| public static function factory($seed, $defaults = [], /* remove in 2021-jun, catch passed but no longer supported prefix */...$extraArgs): object |
There was a problem hiding this comment.
How about renaming this create for semantic purposes?
There was a problem hiding this comment.
I am not against, but I would like to see analysis from top 10 frameworks to decise if it is worth or not.
There was a problem hiding this comment.
You should always use ClWithDi::fromSeed, thus renaming this method does not help with daily usage...
src/Core.php
Outdated
| /** | ||
| * See \atk4\core\Factory::mergeSeeds(). | ||
| */ | ||
| public static function mergeSeeds(...$seeds) |
There was a problem hiding this comment.
How about using only merge?
There was a problem hiding this comment.
Current name is much more self explaning, I prefer to keep it.
| */ | ||
| public static function fromSeed($seed = [], $defaults = [])// :static supported by PHP8+ | ||
| { | ||
| if (func_num_args() > 2) { // prevent bad usage |
There was a problem hiding this comment.
You can reduce code here by simpy calling
$object = static::fromSeedUnsafe($seed, $defaults);
static::assertInstanceOf($object);
return $object;
There was a problem hiding this comment.
assertInstanceOf is computed even before the object is even created, which implies stronger security! (as seed can be an user input)
There was a problem hiding this comment.
What I see the assertInstanceOf is called before the $object is returned only?
The rest of the code is the same.
There was a problem hiding this comment.
_fromSeedPrecheck is called BEFORE object is constructed. Be very carefull around this and test for this behaviour will be highly welcomed.
no BC break
merge after: atk4/ui#1251 (one line fix needed for ui/Template because of php7.2 limitation of traits)
By using single point factory we can offer extreme magic - user can inject own Factory and he can return any implementation he wants for any object (based on stacktrace even different) thru the whole project.
For these reasons you should never use direct instantiation:
in Atk code but always create new instance thru
\atk4\core\Factorylike:FactoryTrait was quite a bad design pattern - there is currently no need to remove it, as it can be deprecated only for now without any negative side effect.