Invoke call to entity_modified_last through the hook system

This seems reasonable... lets let this marinate for a bit though to see if anyone has some other thoughts

The patch #4 helps site from breaking if entity_modified module is not installed.
The watchdog() function format does not look good.
Also,
I don't understand the way $key variable is used.
static $key = 1000;
return (string)$key++;

$key++ will always return 1001.

I've made some corrections in patch #4 and uploaded it.

Similar to what entity_modified module has done here, http://cgit.drupalcode.org/entity_modified/tree/entity_modified.module?i...
we could just return "1" if the entity_modified module is not found.

I actually experienced this when importing a database from a production site to a local site. I checked the database and found that the module was enabled, so assumed it to be a caching issue. Clearing caches resolved my issue. Posting this in case someone else experiences the error under similar circumstances.

These patches don't cover the case where the entity_modified module exists in the filesystem but has yet to be installed. The call to module_load_include() doesn't do any checks that the module is enabled and blindly loads that file, which seems pretty dangerous. As a result you will get a PDOException if the table is missing.

Here is a patch that avoids trying to load the module file directly. It's either there and enabled, or it's not. If for some reason this function is being called before the entity_modified module is initialized by core, then that seems like a separate issue.

Hi there,

Here is the author of entity_modified. I also originally worked with Crell in IRC on the caching part of dfp.

What needs to happen here is that you disable the caching if entity_modified_last function does not exist and you probably don't want to spam watchdog with messages either to avoid breaking sites. I suggest to add hook_requirements error.

On the other hand it is a clear dependency, so maybe a fatal (as is now) is also fine.

But do not just return 1, because that means your token cache won't be updated in case that you update any referenced node or user.

I think since the module is required as dependency in the info file, it cannot be optional

dependencies[] = entity_modified

In other words, if we want to use #11, we need to remove the dependency from the info file.

alternatively, we can simply enable entity_modified in a hook_update()

I am still not sure I would like to have the second option, because we don't have a need to "cache all things"

Ok, here is my solution:

• Made DFP token cache optional, since not everyone needs it. Added new option in the admin UI
• Only show the cache lifetime input when token cache is ticked
• Added a validation to prevent enabling the cache while Entity modified is not enabled (I am checking if the function exists, rather then module exists, just in case they change the API for any reason)
• Removed dependency on Entity Modified and turned into a soft dependency.
• Added a hook_requirement to show the relevant message.
• And lastly, moved the caching logic into a helper function _dfp_token_replace_cache()

Please review and let me know if I forgot anything

I forgot to enable the module during the tests

   function setUp() {
     // Enable a couple modules.
-    parent::setUp('ctools', 'dfp', 'taxonomy');
+    parent::setUp('ctools', 'dfp', 'taxonomy', 'entity_modified');
     menu_rebuild();

One last thing

 function dfp_uninstall() {
   // Delete variables.
   variable_del('dfp_set_centering');
+  variable_del('dfp_token_cache_enabled');
+  variable_del('dfp_token_cache_lifetime');
 }

1. +++ b/dfp.admin.inc
   @@ -119,11 +119,23 @@ function dfp_admin_settings($form, $form_state) {
   +  $form['global_tag_settings']['dfp_token_cache_enabled'] = array(
   +    '#type' => 'checkbox',
   +    '#title' => t("Enable DFP Token cache"),
   +    '#default_value' => variable_get("dfp_token_cache_enabled", TRUE),
   +    '#description' => t('DFP custom cache for token replacements. Requires Entity Modified module.'),
   +  );
   

   Can we disable this if Entity Modified is not enabled?

2. +++ b/dfp.admin.inc
   @@ -240,6 +251,15 @@ function dfp_admin_form_validate($element, &$form_state)
   +  if (($settings['dfp_token_cache_enabled']['#value']) == TRUE) {
   +    if (!function_exists('entity_modified_last')) {
   

   This can be 1 if statement

Good idea

This looks pretty good to me now .... anyone else want to kick the tires?

Looks good! +1 for RTBC

A new stable release would be nice because the last stable is broken due to this error for me.
Thank you!

Hold on, I think setting the setting to be disabled when entity_modified is causing a problem.
Because the default value of variable_get("dfp_token_cache_enabled", TRUE) is True, we have a situation where the box is ticked but the form validation fails.

I think we could change the default value to FALSE when the module is not enabled, but when enabled, get the value from the variable as usual

'#default_value' => (module_exists('entity_modified')) ? variable_get("dfp_token_cache_enabled", TRUE) : FALSE

Sorry about re-opening this, I just realized the problem with disabling the option after it was already committed

Grrrr ... because the value of a disabled form element is sent anyway. Sigh.

This looks like the correct solution to me but havent had a chance to actually test. Will try later today, but if someone else has a minute that would be great

(nice catch @marcelovani)