search

jonathangeiger / kohana-jelly

See the link below for the most up-to-date code
https://github.com/creatoro/jelly
MIT License
146 stars 34 forks source link

Jelly issue when processing models with non-standard Meta #159

Closed AlexKupreev closed 14 years ago

AlexKupreev commented 14 years ago

Meet this when working on my Jelly-MPTT module. It overrides Jelly_Meta in some manner that can become a problem when there are other classes extending Jelly_Meta. I've resolved this by http://github.com/AlexKupreev/jelly-mptt/commit/41706df5e6f0ad7372aafeec4efb682e0efdd14b, that was encapsulated only in mptt module. Things went right until I invoke Jelly::select(MPTT_Model) without Jelly::factory(MPTT_Model) before. Jelly::select(MPTT_Model) initializes Meta from original Jelly and to override that I should overwrite native Jelly::select() and Jelly::builder() for Jelly_MPTT::select() and Jelly_MPTT::builder().

I think it is redundant because model extends MPTT. The simpler solution is a small modification of original Jelly::builder() by invocation Jelly::factory() in it:http://github.com/AlexKupreev/jelly-mptt/commit/9be3d723fcfffb27cec18a1cfb0b5423d7872e0c.

Is it the right way? If yes, maybe original Jelly repo can apply that patch?

banks commented 14 years ago

I don't understand why you have to subclass Jelly_Meta. Can you not just add the additional fields you need to the field list before it is finalised in your abstract MPTT model?

AlexKupreev commented 14 years ago

You mean putting these fields in Jelly_Model_MPTT::initialize()? hmmm... Just now I can't prove it so I need to think again. Thanks for advice!

banks commented 14 years ago

You mean putting these fields in Jelly_Model_MPTT::initialize()?

Yep. It's specifically designed to allow inherited structures add meta data.

AlexKupreev commented 14 years ago

My argument for putting MPTT fields in Meta was in this case we can 1) set custom names for fields and 2) access these fields names much simpler: Model_MPTT->left_column for example. Otherwise we should invoke Model_MPTT->meta()->fields('left_column')->name. Even if we miss the beauty, is the last approach faster in terms of performance?

banks commented 14 years ago

You should be able to do 1 though a field's natural aliasing - You would perhaps limit your model to always having a file 'left_column' but the table column could be called anything and you could even optionally add aliases (in normal Jelly speak) so that $model->left also accessed it. In this way $model->left_column would return a value rather than the Field object but that is normal Jelly behavior and there is no reason that MPTT fields should be treated differently.

2) access these fields names much simpler

As I've mentioned, non-standard access is a minus rather than a plus as far as I'm concerned. It may be neater but it is more to learn, more to go wrong and ultimately requires unnecessary changes to the core to get it to work properly. I think you would be better of working with Jelly meta as it was designed.

Performance is neither here nor there - I can pretty much gaurantee that avoiding a few function calls one way or another will make absolutely zero difference to your real-world database-driven application. If you end up doing something like batch processing hundreds of thousands of model objects and you profile and find that it is this access which is slowing you down, then perhaps make a bit of effort to avoid the additional calls but I'd be amazed if it is field meta access that caused any real issues!

AlexKupreev commented 14 years ago

Thanks for help, Paul! I've tried to rebuild jelly-mptt in more convenient style.