-
Notifications
You must be signed in to change notification settings - Fork 13
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
Protected init #265
Protected init #265
Conversation
yes, and also in ui and data... I prepared fixes |
d6997ac
to
f87c3a4
Compare
f87c3a4
to
83b8873
Compare
sometimes (at least in ui) we need before/after init - now this can be done easily by overriding @georgehristov @DarkSide666 wdyt? |
/** | ||
* Do not call directly. | ||
*/ | ||
public function invokeInit(): void |
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 is the benefit of protecting init and then exposing it with another method?
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.
there are a lot of situations when you need before/after init method - I was thinking about hooks, but this is fine as you can override invokeInit() - this is needed when init() itself is extended, which is very ofter in ui
and by making init()
protected we can ensure it is (without hacking) not called directly
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.
Yes, I agree with that. Setting View::$model
for instance needs to be done afterInit and not during init.
To follow common naming paterns (as in atk4/data) maybe we can use init
, doInit
(instead of invokeInit
, init
)?
So you keep init public but we move the actual routine under protected doInit
. It will be no BC break then I believe
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.
To follow common naming paterns (as in atk4/data) maybe we can use
init
,doInit
(instead ofinvokeInit
,init
)?
doInit
is horrible
there are two uses and they do not invoke!, they process the operation (delete) after beforeDel and before afterDel:
So you keep init public but we move the actual routine under
protected doInit
. It will be no BC break then I believe
Impossible, the init method must be protected and not called directly :)
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 was talking about the atk4/data#690 where the actual operations are performed in doXx
methods.
Impossible, the init method must be protected and not called directly :)
But we call directly the invokeInit? The matter is only of naming, mostly for consistency.
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.
doXxx there is fine in Sql sense (ie. do/send query)
here we keep init as standard method to implement
But we call directly the invokeInit?
I understand your point - there are some uses, for easy migration, I propose to keep "some" method and keep it public, in optimal world, this method should never be called by used.
public function invokeInit(): void | ||
{ | ||
// make sure init() method is never declared as public | ||
$reflMethod = new \ReflectionMethod($this, 'init'); |
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.
FYI: checked for every instance/call, but it is fast: +0.5% of total testing time for atk4/ui where used heavily
In my opinion this is huge breaking change and almost without any advantage. |
The only thing that needs to be adressed is the For instane in UI View class we call |
@georgehristov let's go on Discord and discuss the final version, I need this to be merged asap |
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.
OK I agree :)
Documented also to be not called directly, but quite large breaking change
How to migrate your code?
replace
public function init()
withprotected function init()
and
->init()
with->invokeInit()