Broken Upgrade: filter #7001

ralphb: lets get to teh bottom of the cause of the prbolem you are having before we deem this critical and not ralphb's site-specific. Can you be more specific in your list of modules (copy and pate from drush sm would be great)

Looking at the Filter update #7001, shouldn't the insert values be 'module' => 'filter' instead of 'filter' => 'filter' ??

/**
 * Break out "escape HTML filter" option to its own filter.
 */
function filter_update_7001() {
  $result = db_query("SELECT format FROM {filter_format}")->fetchCol();
  $insert = db_insert('filters')->fields(array('format', 'module', 'delta', 'weight'));

  foreach ($result as $format_id) {
    // Deprecated constants FILTER_HTML_STRIP = 1 and FILTER_HTML_ESCAPE = 2.
    if (variable_get('filter_html_' . $format_id, 1) == 2) {
      $insert->values(array(
        'format' => $format_id,
        'filter' => 'filter',  /* SHOULDN'T THIS BE 'module' => 'filter' ? */
        'delta' => 4,
        'weight' => 0,
      ));
    }
    variable_del('filter_html_' . $format_id);
  }

  $insert->execute();
}

Regression came in #570900: Destroy remnants of update_sql(), for the curious.

Here's a simple test both with and without the patch in #5.

Really, I think we have proven the upgrade fix sufficiently.

Not to be a pain, but could we please label that something other than 'filter_update_7001 test' for future generations? What exactly is that data testing?

@webchick: Yes, you are a pain. :)

I think the patch in #5 (i.e., the one without the test) is fine, but if you must I've changed the name to 'Escape HTML Filter' which matches (case-insensitively) the comment in filter_update_7001().

All the patch does is test whether the code which would upgrade an "Escape HTML Filter" actually runs. Nothing more. It certainly doesn't check that the filter is still there after the upgrade procedure. (By "Escape HTML Filter", I mean "HTML Filter" where on the configuration page you choose "Escape HTML").

I understand the need for tests, but it seems silly to me to keep writing these ad-hoc tests for things which were bungled up in DBTNG-type conversions. Oh well.

Sorry, I'm still not understanding how the test you've written fails without the patch and passes without it. I'm glad that it does! But we either need a comment there or a better name or... something... to explain this, because in future refactorings we want to make sure we don't lose this test since it's legit.

@webchick: Did you see #12. Is that name not acceptable?

Literally the upgrade path is using wrong DB calls so that it dies if it ever gets run. It only runs if there is an input filter with the "Filter HTML" module enabled and on the configuration screen you choose "Escape HTML." (This is a pretty rare configuration). So that's what the test did -- it created just such an "Escape HTML Filter".

I did. #12 is what continued to confuse me despite the human-readable name. Maybe I should stop reviewing patches at 12am or.. something.... :P

I think what you're maybe saying is that this:

 ->values(array(
+  'name' => 'filter_html_3',
+  'value' => 'i:2;',
+))

...needs a comment above it like:

// Set a custom input format to use one of the system-provided filters in order
// to test that FLERZYWHATSIT happens.
 ->values(array(
+  'name' => 'filter_html_3',
+  'value' => 'i:2;',
+))

But I can't quite parse out what FLERZYWHATSIT is. Can you help? :)

This has been languishing in the issue queue for far too long. What was a simple typographical fix --- the {filters} table has a column named 'module' not 'filter' --- is getting held up because of a desire to create a properly documented test.

Now the test provided in #12 (and in #9) does detect the presence of this patch, but the scope of the test is rather narrow --- it only ensures that the filter_update_7001() code actually runs. I've uploaded two patches.

The first, 989556-17.patch, includes the test with the following comment in response to Angie in #16:

+// Add the Escape HTML filter to the custom input format 'Escape HTML Filter'
+// to test that the filter may be upgraded to its Drupal 7 equivalent.

The second, 989556-17_0.patch, includes only the typographical fix.

(Sorry for the duplicitous filenames!!)

I'd recommend committing the version with the test if you find the comment tolerable, and the version without the test if you find the comment confusing.

Thanks, that's what I was looking for.

Committed to HEAD.

And I know it's frustrating when patches get held up for comments, but it's also frustrating to have a phantom test failure sometime far into the future and have no hope about figuring out why.

Thanks webchick for your infinite patience.