jeremyharris / cakephp-lazyload

A lazy loader for CakePHP entities.
MIT License
61 stars 10 forks source link

Trying to lazy load on new entities #16

Open Zuluru opened 6 years ago

Zuluru commented 6 years ago

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.

jeremyharris commented 6 years ago

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 :)

Zuluru commented 6 years ago

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.

jeremyharris commented 6 years ago

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;
Zuluru commented 6 years ago

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. :-)

jeremyharris commented 6 years ago

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.

Kjubyte commented 6 years ago

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?

lorenzo commented 6 years ago

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

Zuluru commented 6 years ago

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

Kjubyte commented 6 years ago

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?

jeremyharris commented 6 years ago

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.

lorenzo commented 6 years ago

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...

lordphnx commented 3 years ago

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?

lorenzo commented 3 years ago

@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 commented 3 years ago

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.

jeremyharris commented 3 years ago

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.

lordphnx commented 3 years ago

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