Skip to content
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

[3.1.x]Model binding for micro, current state of it #12324

Closed
Jurigag opened this issue Oct 15, 2016 · 4 comments
Closed

[3.1.x]Model binding for micro, current state of it #12324

Jurigag opened this issue Oct 15, 2016 · 4 comments
Milestone

Comments

@Jurigag
Copy link
Contributor

Jurigag commented Oct 15, 2016

Well i wanted to add model binding for micro. But after some talking with @sergeyklay and looking into code of it right now it's adding too much overhead. Also adding it for micro means add it actually to two places - micro class https://github.com/phalcon/cphalcon/blob/master/phalcon/mvc/micro.zep and lazy loader https://github.com/phalcon/cphalcon/blob/master/phalcon/mvc/micro/lazyloader.zep so in order to not have copy-pasted and very similar code in three classes i want to move this code in this if statement - https://github.com/phalcon/cphalcon/blob/master/phalcon/dispatcher.zep#L554 to most likely new class Phalcon\Mvc\Model\Binding. When setting we want to use model binding it will be just created(or maybe it should be static method ?). When zephir will introduce traits it will be changed to trait.

The problem with current model binding implementation is that in each request it's accessing reflection api which is heavy, In some big application when someone want to use model binding it can add overhead. We can't remove reflection to keep this nice feature in my opinion, but what we can do is just to introduce cache mechanism to just cache parameter keys and classes for each action so we will hit reflection api only one time per action if cache is used. Setting it would be just in second parameter of setModelBinding, like name of cache service from di which will be used for caching. Or Phalcon\Mvc\Model\Binding will stay as an class which will have key of service cache in it, tbh i like this idea more so Binding can be just used as seperated component anywhere pretty much.

So in total:

  • retractor current model binding code, remove not needed call_user_func, call_user_func_array
  • move it to separated class, should method be static or not ?
  • introduce cache for remove reflection api overhead

Would appreciate any opinion about this.

@sergeyklay sergeyklay added this to the 3.1.x milestone Oct 15, 2016
@sergeyklay
Copy link
Contributor

👎 for static

@Jurigag
Copy link
Contributor Author

Jurigag commented Oct 15, 2016

Yea i think i will add it just as seperated class with normal method and construct which will accept cache object/service name so it will stay as class forever anyway.

@Jurigag
Copy link
Contributor Author

Jurigag commented Oct 16, 2016

Also i think i will add option to bind multiple params.

@Jurigag
Copy link
Contributor Author

Jurigag commented Jan 12, 2017

Well since it's already implemented in #12341 i close this issue

@Jurigag Jurigag closed this as completed Jan 12, 2017
@sergeyklay sergeyklay modified the milestones: 3.2.x, 3.2.0 Apr 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants