As I was building the payment transaction entity controller, I noticed that my copy of the controller from the Customer module included a cache_clear_all() to empty the "page, block, and commerce_customer_profile_load" caches. This code itself is just a straight copy from node.module's own delete implementation. What I'm wondering is... is this really necessary, and will it hurt performance? That's a pretty destructive cache reset... and my hunch is that it's not necessary for our entities in the same way it is for node. We can probably get away with a more targeted cache clear, right?

In any event, I left it in for now so every controller can be audited and updated in one patch here.

Comments

rszrama’s picture

rszrama commented
Issue tags: +beta blocker

Tagging. This is pretty important to get right, so I'd appreciate some review... my hunch is a more targeted cache clear would better suit us here.

mikejoconnor’s picture

mikejoconnor commented
Issue tags: -beta blocker

I really do not believe this issue is a beta blocker. It may be a short cut, or poor coding, but I believe it can be resolved under release candidate.

rszrama’s picture

rszrama commented

Wait, I thought you were going to add an opinion about the issue itself, not the tag...

damien tournoud’s picture

damien tournoud commented

This looks like a straight bug to me, too.

rszrama’s picture

rszrama commented

As in, it actually does appear to be a bug? So we should try to get rid of cache_clear_all() or is there a more targeted cache clear that we (and core) could use? I'm not worried about the beta blocker status...

damien tournoud’s picture

damien tournoud commented

No, as in, "it's just a bug", it doesn't need to be a beta blocker.

rszrama’s picture

rszrama commented

heh, I understand that. I'm just looking for someone to provide advice on the fix. ; )

rszrama’s picture

rszrama commented
Status: Active » Closed (duplicate)

This is not happening any more thanks to #1181534: Refactor entity handling.

rszrama’s picture

rszrama commented
Title: Every entity controller uses cache_clear_all() on delete » Do we want to clear the cache each time we change entities?
Status: Closed (duplicate) » Active

If so, what entity types?

This is repurposed instead as a follow-up to the above linked issue.

rszrama’s picture

rszrama commented
Category: bug » task

Guess we should actually figure this out.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Issue summary: View changes

@rszrama yes.

Take commerce_line_items for example, you start seeing this littering the code: entity_get_controller('commerce_line_item')->resetCache(array($line_item_wrapper->line_item_id->value()));

Right after:
commerce_line_item_save($line_item_wrapper->value());

But if we are making changes to a line item, it had better well update it's cache.