akeeba / fof

Rapid Application Development framework for Joomla!™ 3 and 4
0 stars 0 forks source link

onAfterGet in DataModel? #533

Closed compojoom closed 9 years ago

compojoom commented 9 years ago

I had to manipulate the items of my Model. I need to look at the relations that each item has and on the base of that add a flag to each item in the DataCollection. I found that the DataModel is triggering an "onAfterGetItemsArray" event and it's passing the items as parameter.

So in my model I created a function "onAfterGetItemsArray" and started manipulating the items.

The funny thing was that my queries went to the roof. I was actually supposed to eagerLoad the relations, but that was not the case. My relations were actually lazyloaded and at the end I had the eagerLoad query:

where id IN (....)

Upon investigating I found out that onAfterGetItemsArray was not the proper event. It's too early. I have to manipulate the dataCollection once it is build and not before that.

Do you think it is a good idea to have onGetEagerLoad event in the get function? Or should I better just override the get function?

nikosdion commented 9 years ago

Stop thinking as if you are writing code for FOF 2. You can override your get method and use a transform against the items:

public function get($overrideLimits = false, $limitstart = 0, $limit = 0) { $collection = parent::get($overrideLimits, $limitstart, $limit) return $collection->transform(function($items){ // Do something with your data }); }

Instead of transform() you may use any other method of FOF30\Utils\Collection or FOF30\Model\DataModel\Collection. It is FAR more powerful than what you could do in FOF 2.

On Fri, 24 Jul 2015 at 18:15 Daniel Dimitrov notifications@github.com wrote:

I had to manipulate the items of my Model. I need to look at the relations that each item has and on the base of that add a flag to each item in the DataCollection. I found that the DataModel is triggering an "onAfterGetItemsArray" event and it's passing the items as parameter.

So in my model I created a function "onAfterGetItemsArray" and started manipulating the items.

The funny thing was that my queries went to the roof. I was actually supposed to eagerLoad the relations, but that was not the case. My relations were actually lazyloaded and at the end I had the eagerLoad query:

where id IN (....)

Upon investigating I found out that onAfterGetItemsArray was not the proper event. It's too early. I have to manipulate the dataCollection once it is build and not before that.

Do you think it is a good idea to have onGetEagerLoad event in the get function? Or should I better just override the get function?

— Reply to this email directly or view it on GitHub https://github.com/akeeba/fof/issues/533.

compojoom commented 9 years ago

Yep, this is what I ended up doing yesterday. I just overwrote the get function, but I remember that I read somewhere on the wiki, that unlike joomla's core you are not supposed to override the methods, but to use the triggered events (can't find the passage anymore though).

nikosdion commented 9 years ago

Ideally you would have a new method doing the post processing after get or defer that to where you are using the model. Post processing after get is code stink. You ARE doing something wrong, I can guarantee that. Most likely you need to define get/setAttribute methods (see Akeeba Subscriptions) instead.

So. If you are doing bad architecture you have to jump through hoops and get a clear feeling that what you are doing is bad and doesn't look like something you should do. Now you know why this looks wrong to you: it is wrong :)

Στις Σάβ, 25 Ιουλ 2015 - 11:04 ο χρήστης Daniel Dimitrov < notifications@github.com> έγραψε:

Yep, this is what I ended up doing yesterday. I just overwrote the get function, but I remember that I read somewhere on the wiki, that unlike joomla's core you are not supposed to override the methods, but to use the triggered events (can't find the passage anymore though).

— Reply to this email directly or view it on GitHub https://github.com/akeeba/fof/issues/533#issuecomment-124819012.

compojoom commented 9 years ago

Hm fieldAttribute. I didn't think about it, in matter of fact I didn't know it actually existed :D

However I'm failing to get it to work with it. As far as I understand I have to use

$this->addKnownField() -> to add my field to the Model.

then I have a getFieldAttribute function, which is being called whenever someone tries to access the field. So far so good! However in my case - I need to set the value of the field depending on the relations.

I'm trying to determine if an apartment is free to book for period X. That's why I need to get the active bookings, then go over them and see if we have a booking for period X. If we don't I mark the apartment as free.

Now when I followed the code. I see that get() is calling getItemsArray(), getItems array is calling the bind() function and the bind function is actually calling databaseDataToRecordData() and databaseDataToRecordData() has the following lines:

$method = $this->container->inflector->camelize('get_' . $name . '_attribute');

            if (method_exists($this, $method))
            {
                $this->recordData[$name] = $this->{$method}($value);
            }

It's here where my getFieldAttribute function is called. However at this stage the model hasn't build the relations yet, because get hasn't yet called the eagerLoad method. That's why I end up with an empty Collation in my getFieldAttribute.

So at this stage I'm still thinking that the only way is by overriding the get().

I previously had the calculation if the apartment is free in the OnBeforeBrowse in the view, but it turns out that I generally need to know if the apartments are free or not. So that is why I'm trying to do all this on the Model...

You still think that I'm doing something wrong?

nikosdion commented 9 years ago

So far so good! However in my case - I need to set the value of the field depending on the relations.

Don't. Use the relations instead. If you are replicating information from the relations you are doing something wrong. If you need to combine information from multiple relation fields into a single value you can simply create a method in your Model.

I'm trying to determine if an apartment is free to book for period X. That's why I need to get the active bookings, then go over them and see if we have a booking for period X. If we don't I mark the apartment as free.

So you need a method, not a field. It's not a static value, there's a process involved.

Finally, please ask these questions in the mailing list. The GitHub issues are here for code issues in FOF, not questions about how to use FOF.

compojoom commented 9 years ago

sorry. forgot that there is a list as well.

nikosdion commented 9 years ago

If you have static fields depending only on what is stored on your table then you can use setAttribute and getAttribute.

If you have static fields depending only on what is stored in a related record then you can use setAttribute and getAttribute on the related model.

In any other case what you are doing is synthesizing information based out of your static data, i.e. you have dynamic data. These are best served by methods. Even if you do override get() (NOT advisable) you would have to also override reset() and recordDataToDatabaseData() as well to prevent other issues. Generally, it's unadvisable to store dynamically generated information as fields.

Do note that nothing stops you from overriding __get() to make the methods providing dynamic data to magically work like (read only) properties.

compojoom commented 9 years ago

Yes, you are absolutely right.

So. If you are doing bad architecture you have to jump through hoops and get a clear feeling that what you are doing is bad and doesn't look like something you should do. Now you know why this looks wrong to you: it is wrong :)

What I was doing was absolute shit and utterly moronic. I realized it yesterday when I did go through all the trouble overriding FOF methods, where as you said all I need is a method that does the calculations.

So now I ended up with 30 lines of code that do the calculations and I could go away with all the overrides of FOF methods that I did.

Funny how when you start with the wrong set of ideas you do so much stupid things to repair something that you didn't have to do in the first place.

Thanks for the help again!

nikosdion commented 9 years ago

This is how you learn. Do you think I got FOF right from the first try? Nah. This is why we are already in version 3 :-P