cakephp-lazyload icon indicating copy to clipboard operation
cakephp-lazyload copied to clipboard

Trying to lazy load on new entities

Open Zuluru opened this issue 8 years ago • 16 comments

Is there a good reason why LazyLoadEntityTrait::_lazyLoad doesn't short-circuit and return null immediately if $this->isNew()? My use case where this causes problems is perhaps a bit obscure, but I don't see an obvious downside to including this test.

Zuluru avatar Oct 18 '17 22:10 Zuluru

New entities can still have associated records, no? It's possible someone will want to use lazy loading to bring the associated record data after creating a new entity.

I'd like to hear your use case. If you have time, creating a PR would be awesome :)

jeremyharris avatar Oct 19 '17 18:10 jeremyharris

They can, but at least in Cake 3.3.3 (which I am stuck with for the moment), loadInto won't load those associated records anyway, so lazy loading is of no help there. I wish it was, as I do have other situations exactly like this which it would solve. :-) I'll see if I can simplify my use case down to something I can express clearly; it's possible that in doing so I'll find a way to resolve it on my side.

Zuluru avatar Oct 19 '17 19:10 Zuluru

Oh interesting, I didn't know loadInto wouldn't load if the entity is new. Makes sense for some association types but not all.

Depending on your use case, you could mark it as not new before lazy loading (assuming it has been persisted).

$entity->isNew(false);
$entity->association;

jeremyharris avatar Oct 19 '17 20:10 jeremyharris

Sorry, I meant "not persisted", not "new". Apologies for not being clear. I'm not sure why Cake implements loadInto the way they do (seems easy enough to make it work even for entities which have not yet been persisted, but have had the foreign key set), but that's an entirely different conversation, for a different repository. :-)

Zuluru avatar Oct 19 '17 20:10 Zuluru

I'm closing this as it seems to be an application specific need. If others want to be able to load associations into entities that have yet to be persisted, we can re-open for discussion.

jeremyharris avatar Feb 27 '18 16:02 jeremyharris

I need exactly this behaviour. I have a form. If there are some errors on submit, I need to load an association of a not persisted entity. Cakes loadInto doesn't work in this case.

Currently my workaround is to load the assocications in the controller and assign them to the entity:

// deviceDefinition hasMany fields belongsTo field_definition
// deviceDefinition and fields are not persisted yet, field_definition is persisted
foreach ($deviceDefinition->to_fields as $field) {
    $field_definition = $this->getTableLocator()->get('ToFieldDefinitions')->get($field->field_definition_id);
    $field->to_field_definition = $field_definition;
}

Any suggestion for a clean way to implement this feature in cakephp-lazyload?

Kjubyte avatar Jun 08 '18 08:06 Kjubyte

@WilliTheSmith that does not look like a terrible workaround, does it?

lorenzo avatar Jun 08 '18 08:06 lorenzo

It's just very easy to forget to do that, and sort of runs contrary to the whole "lazy" aspect of it. :-)

Zuluru avatar Jun 08 '18 13:06 Zuluru

Yeah, this is a task that fits perfectly into the lazyload idea. So I don't have to do this multiple times in different controllers. It makes the controller actions bigger wich isn't absolutly neccesary there. Lazyload would make exactly the same but I can be lazy. :-) On top this is some kind of code duplication wich I think we could avoid.

@jeremyharris could you reopen this issue for further discussion?

Kjubyte avatar Jun 08 '18 13:06 Kjubyte

Ok, so as I understand it, you wish to lazily load associations for entities that have not yet been persisted but have a foreign key on it (so belongsTo only).

If anything, I feel the original patchEntity is what "feels" inconsistent, as patching a belongsToMany or hasMany would load the actual entities onto the parent, but belongsTo doesn't, unless it's patched like this afaik (I could be wrong here):

$data = [
    'name',
    'belongsToAssoc' => [
        'id' => 1
    ]
]

In that case, it kind of makes sense to be able to lazy load it. I'm curious to know @lorenzo's thoughts on this.

jeremyharris avatar Jun 08 '18 14:06 jeremyharris

I think it is just a challenging problem, as for many types of association it is. It only necessary to have the foreign key, but also other data, or even join other tables...

lorenzo avatar Jun 08 '18 16:06 lorenzo

I was wondering whether there has been any progress on this issue, or whether people have come up with new/cleaner workarounds. I've been bumping into this every since I started using the lazyloader (thanks for the amazing plugin BTW!).

My main usecase is when sending emails in Model.afterSaveCommit (e.g. $mailer->setTo($issue->assignee->email))

I've been using a workaround similar to @WilliTheSmith, but it feels like there should be some setting to enable this. Do I understand correctly that it's a "deficiency" in Cake itself that's preventing this?

lordphnx avatar Jan 09 '21 15:01 lordphnx

@lordphnx The cake ORM started with the idea in mind that lazy loading is more often than not a source of different issues and inefficiencies. That's why the query builder offers so much flexibility for eagerly loading associations. It's guiding you to think of loading associations all at once instead of one at a time.

This plugin bridges the gap for those who don't care too much about efficiency, which is a valid use case when prototyping or for applications with little data. I would not say that there is any specific deficiency preventing thins, but perhaps a lack of interest of covering all cases lazy-loading may have, as the long term solution is to do eager loading.

lorenzo avatar Jan 09 '21 19:01 lorenzo

My main usecase is when sending emails in Model.afterSaveCommit (e.g. $mailer->setTo($issue->assignee->email))

Have you tried to debug why your use case is not working? in afterCommit the entity is no longer new, so there should be no problems loading associations at that point.

lorenzo avatar Jan 09 '21 19:01 lorenzo

I'm not really sure what to do with this issue. My personal suggestion would be to patch the full associated entity, though I know that requires a bit of work on your ends. I might experiment with loading onto non-persisted entities, but I feel like that's a rabbit hole I don't want to go down. I could be wrong, but it doesn't seem like this is enough of a pain point to warrant opening the plugin up to possible edge cases with more complex associations.

jeremyharris avatar Jan 11 '21 18:01 jeremyharris

Thanks for the blazing fast reply ❤️ and your insights! This gives me some new insights/confirmations

lordphnx avatar Jan 15 '21 16:01 lordphnx