Since the node and node_revision tables are directly manipulated via db_update() calls, this module can run into conflicts with the Entity cache module (which requires that any updates to entities use the Drupal APIs rather than direct queries to the database.

This error manifests itself in the form of the node module thinking the quiz has been edited by another user, throwing this error:

The content on this page has either been modified by another user, or you have already submitted modifications using this form. As a result, your changes cannot be saved.

Looking into the code a bit, this occurs in quiz.module:

/**
 * Updates the max_score property on the specified quizzes
 *
 * @param $quizzes_to_update
 *  Array with the vid's of the quizzes to update
 */
function quiz_update_max_score_properties($quizzes_to_update) {
  if (empty($quizzes_to_update)) {
    return;
  }

  db_update('quiz_node_properties')
    ->expression('max_score', 'max_score_for_random * number_of_random_questions + (
      SELECT COALESCE(SUM(max_score), 0)
      FROM {quiz_node_relationship} qnr
      WHERE qnr.question_status = ' . QUESTION_ALWAYS . '
      AND parent_vid = {quiz_node_properties}.vid)')
    ->condition('vid', $quizzes_to_update, 'IN')
    ->execute();

  db_update('quiz_node_properties')
    ->expression('max_score',
      '(SELECT COALESCE(SUM(qt.max_score * qt.number), 0)
      FROM {quiz_terms} qt
      WHERE qt.nid = {quiz_node_properties}.nid AND qt.vid = {quiz_node_properties}.vid)')
    ->condition('randomization', 3)
    ->condition('vid', $quizzes_to_update, 'IN')
    ->execute();

  db_update('node_revision')
    ->fields(array('timestamp' => REQUEST_TIME))
    ->condition('vid', $quizzes_to_update, 'IN')
    ->execute();

  db_update('node')
    ->fields(array('changed' => REQUEST_TIME))
    ->condition('vid', $quizzes_to_update, 'IN')
    ->execute();

  $results_to_update = db_query('SELECT vid FROM {quiz_node_properties} WHERE vid IN (:vid) AND max_score <> :max_score', array(':vid' => $quizzes_to_update, ':max_score' => 0))->fetchCol();
  if (!empty($results_to_update)) {
    db_update('quiz_node_results')
      ->expression('score',
        'ROUND(
        100 * (
          SELECT COALESCE (SUM(a.points_awarded), 0)
          FROM {quiz_node_results_answers} a
          WHERE a.result_id = {quiz_node_results}.result_id
        ) / (
          SELECT max_score
          FROM {quiz_node_properties} qnp
          WHERE qnp.vid = {quiz_node_results}.vid
        )
      )')
      ->condition('vid', $results_to_update, 'IN')
      ->execute();
  }
}

It isn't immediately obvious to me what this is doing, but I will continue to investigate and hopefully find a way that allows for the use of node_save().

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jhedstrom’s picture

Issue summary: View changes
jhedstrom’s picture

Ah, I see this is called when new questions are added to the quiz.

Would it be an over-simplification to simply not update the changed column? That is what is causing the system to get out of whack with the cached entity.

jhedstrom’s picture

Status: Active » Needs review
FileSize
816 bytes

This patch resolves the immediate conflict with Entity cache, but like I said above, I am not sure if this is an over-simplification, or if this will cause issues if node changed timestamps are not updated as they currently are. The alternative would be to call node_save here.

FreekVR’s picture

Version: 7.x-4.x-dev » 7.x-5.x-dev
FileSize
1.09 KB

Thanks jhedstrom, this saved me a lot of debugging woes ;)!

Ideally quiz would simply call node_save() or entity_save() proper, to prevent this kind of issue. This issue still occurs in the 5.x, branch, but I'd consider the patch more of a workaround than an actual fix (which would be calling entity_save(), or perhaps use a metadata wrapper?)

Here's a reroll of the patch for the 5.x-branch.

djdevin’s picture

I'm not even sure why that code was necessary, since before this function is called, a new revision may have already been created (and timestamped). Maybe this only has an effect when Quiz auto revisioning is turned off.

If someone else can investigate and mark as RTBC I'll commit it since all the tests seems fine.

FreekVR’s picture

Priority: Normal » Critical
Status: Needs review » Needs work

Unfortunately I've found additional issues - when creating a new revision, the ->save() method for the question controller is being called, which executes a drupal_goto(). Effectively none of the code after line 1173 of the node.module (as well as any other hook_node_update/hook_update implementations) is being executed due to the redirect. This includes a resetCache call to the node entity controller - the new node revision is never being cached, so subsequent load-calls are loading the old vid of the question. Other issues could also arise due to other modules (as well as node core) code not being executed.

I'll have to do some additional debugging with the latest dev version, but looking at the code this might be difficult to fix - since $form_state (to which a redirect should be added) isn't passed around.

djdevin’s picture

Is this critical because of entity cache? Or does this affect Quiz core somehow?

Critical

Bugs
Critical bugs include those that:
Render a system unusable and have no workaround.
Cause loss of data.
Expose security vulnerabilities.
Cause tests to fail in HEAD on the automated testing platform, since this blocks all other work.

FreekVR’s picture

There's a risk for loss of data, due to hooks with a weight higher than quiz not being called, and the following code from node.module NEVER being called when creating revisions for questions:

    // Save fields.
    $function = "field_attach_$op";
    $function('node', $node);

    module_invoke_all('node_' . $op, $node); // << This is where the redirect takes place in the latest dev, code below this line isn't called
    module_invoke_all('entity_' . $op, $node, 'node');

    // Update the node access table for this node.
    node_access_acquire_grants($node);

    // Clear internal properties.
    unset($node->is_new);
    unset($node->original);
    // Clear the static loading cache.
    entity_get_controller('node')->resetCache(array($node->nid));

    // Ignore slave server temporarily to give time for the
    // saved node to be propagated to the slave.
    db_ignore_slave();
  }
  catch (Exception $e) {
    $transaction->rollback();
    watchdog_exception('node', $e);
    throw $e;
  }
FreekVR’s picture

Priority: Critical » Normal
Status: Needs work » Needs review

Alright, I've created a seperate issue for this issue, as it affects more than just entity cache. That means this issue can get it's original status back - but it won't ever work properly unless the related issue is also fixed.

djdevin’s picture

@FreakVR is your most recent patch still necessary? I'm not familiar with entity cache and wondering if #2423363: drupal_goto() called in node form submissions resolved the issue?

FreekVR’s picture

It's still needed, as the quiz_update_max_score_properties() is updating data directly in the database, not updating the cache. On the node edit form, the timestamps will differ, causing the error message mentioned.

djdevin’s picture

Status: Needs review » Fixed

Sounds good, and does not seem like it should have been there. Fixed.

  • djdevin committed 01d20ad on 7.x-5.x authored by FreekVR
    Issue #2230221 by FreekVR, jhedstrom: Incompatible with the Entity Cache...

Status: Fixed » Closed (fixed)

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

jarrodirwin’s picture

Version: 7.x-5.x-dev » 7.x-4.x-dev
Status: Closed (fixed) » Reviewed & tested by the community

@djdevin I've just run into this same problem running on 7.x-4.0-rc2 which at this time of writing is the head of the 7.x-4.x branch.

Apply the patch from #3 still applies cleanly and fixes the issue.

I see you committed it to the 5.x branch but not the 4.x. Can we have this committed please?

djdevin’s picture

Status: Reviewed & tested by the community » Fixed

I cherry-picked 01d20ad into 7.x-4.x.

  • djdevin committed 1f3bd6f on 7.x-4.x authored by FreekVR
    Issue #2230221 by FreekVR, jhedstrom: Incompatible with the Entity Cache...

Status: Fixed » Closed (fixed)

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