-
Notifications
You must be signed in to change notification settings - Fork 14
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
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. |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about renaming this create
for semantic purposes?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about using only merge
?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_fromSeedPrecheck
is called BEFORE object is constructed. Be very carefull around this and test for this behaviour will be highly welcomed.
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.
PR for test: #311
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\Factory
like: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.