Problem/Motivation

Currently, the digest entity has instance variables for holding the block and send at as objects. This was done to prevent excess calls for loading, but it may become a problem in which using the generic entity functions (set as opposed to setDisplayBlock) will cause the getters to load an out of date object.

This was introduced in 3154614 (see that for more reasoning). Now it's been realised that this has only fixed a problem halfway which only makes this unpredictable.

Steps to reproduce

I've attached a patch that illustrates the problem by providing a failing test.

This also illustrated an issue with the current tests. The thrown exceptions stopped test execution. That was overlooked before. So the tests method for detecting exceptions has changed.

Proposed resolution

Two methods to go about this.

Do not store a cached instance and just load the object on each call. This only requires the developer to be smart about storing the return to a variable.

Modify or override the set functionality to provide a smarter caching / invalidating method. This seems like it may just incur way to much complexity and excess checks.

Unless there is some bright idea, first method.

CommentFileSizeAuthor
#4 3165282-4.patch8.76 KBDerekCresswell
#3 3165282-3.patch6.76 KBDerekCresswell
#2 3165282-2.patch5.09 KBDerekCresswell
failing-tests.patch3.22 KBDerekCresswell
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

DerekCresswell created an issue. See original summary.

DerekCresswell’s picture

DerekCresswell CreditAttribution: DerekCresswell as a volunteer and at Acro Commerce commented
Status: Active » Needs review
FileSize
3165282-2.patch5.09 KB

Here is it without an caching of dependencies.

DerekCresswell’s picture

DerekCresswell CreditAttribution: DerekCresswell as a volunteer and at Acro Commerce commented
FileSize
3165282-3.patch6.76 KB

Fixed some left over issues and changed a few things.

I want the get functions to not throw errors. The display will return the broken block if something is wrong. The cron expression will simply return NULL.

For the cron expression, I've had to catch all throwables since I need to cover the type error (for passing NULL) and the invalid argument if the value was set incorrectly.

DerekCresswell’s picture

DerekCresswell CreditAttribution: DerekCresswell as a volunteer and at Acro Commerce commented
FileSize
3165282-4.patch8.76 KB

Update, still missed a few things.

Removed save override.
Added checks for NULL send at.

jseniuk’s picture

jseniuk CreditAttribution: jseniuk as a volunteer commented

Looks good, I think returning NULL is perfectly fine. Not caching was probably the best way to go about it, and I guess the only thing I would say is to make sure that the fact that it loads the object each time is documented somewhere, of course.

  • DerekCresswell committed fd6f8be on 1.0.x 
    Issue #3165282 by DerekCresswell: Prevent Digest instance variables from...
DerekCresswell’s picture

DerekCresswell CreditAttribution: DerekCresswell as a volunteer and at Acro Commerce commented
Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.