Convert Text Formats to Configuration System

Awesome! :)

Thanks for asking for early feedback - I totally have some essential :)

+++ b/core/modules/filter/filter.module
@@ -409,6 +397,110 @@ function filter_modules_disabled($modules) {
+function filter_entity_info() {
+  $types['text_format'] = array(
+    'label' => 'Text Format',
+    'entity class' => 'Drupal\filter\TextFormat',
...
+    'config prefix' => 'text_format',
...
+  $types['text_format_filter'] = array(
+    'label' => 'Text Format Filter',
+    'entity class' => 'Drupal\filter\Filter',
...
+    'config prefix' => 'text_format_filter',

1) The internal entity type ID/name should be 'filter_format' instead of 'text_format'.

2) I don't know have a strong opinion on TextFormat vs. FilterFormat, but most likely FilterFormat makes more sense.

3) The config prefix should be 'filter.format'.

4) The filters in a text format are like effects in image styles. They are not separate config entities, but instead they define the text format's actual behavior and configuration. Therefore, they should be defined as sub-keys of a 'filters' key in the text format config object.

Great! :)

1) The config object name should be "filter.format.[id]", i.e.:

- filter.filtered_html.yml
+ filter.format.filtered_html.yml

2) Yes, FilterFormat extends ConfigEntity! :)

3) Loading formats without their filters... hrm. ;) Good + interesting point there. However, I can't see a way around that right now that wouldn't completely diverge from the direction of how we've converted image styles and effects. So for now I'd say, let's do it in the identical way and keep the methodology consistent. If we need or want to change it later, then we can still do so. :)

+++ b/core/modules/filter/filter.installundefined
@@ -119,31 +24,30 @@ function filter_install() {
-  $plain_text_format = array(
-    'format' => 'plain_text',
-    'name' => 'Plain text',
-    'weight' => 10,
-    'filters' => array(
-      // Escape all HTML.
-      'filter_html_escape' => array(
-        'weight' => 0,
-        'status' => 1,
-      ),
-      // URL filter.
-      'filter_url' => array(
-        'weight' => 1,
-        'status' => 1,
-      ),
-      // Line break filter.
-      'filter_autop' => array(
-        'weight' => 2,
-        'status' => 1,
-      ),
+  $plain_text_format = entity_create('filter_format', array());
+  $plain_text_format->format = 'plain_text';
+  $plain_text_format->name = 'Plain text';
+  $plain_text_format->weight = 10;
+  $plain_text_format->filters = array(
+    // Escape all HTML.
+    'filter_html_escape' => array(
+      'weight' => 0,
+      'status' => 1,
+    ),
+    // URL filter.
+    'filter_url' => array(
+      'weight' => 1,
+      'status' => 1,
+    ),
+    // Line break filter.
+    'filter_autop' => array(
+      'weight' => 2,
+      'status' => 1,
     ),
   );
-  $plain_text_format = (object) $plain_text_format;

Why not keep this as an array and just pass it to entity_create()?

Ideally, wed shouldn't have a 'module' entry for each filter, it's not config per se but derived data.
I guess a "turn filters into Plugins" followup issue would make it easier to get rid of those, though, so probably not a blocker for that patch.

+++ b/core/modules/filter/filter.moduleundefined
@@ -149,6 +150,77 @@ function filter_menu() {
 /**
+ * Implements MODULE_config_import_create().
+ */
+function filter_config_import_create($name, $new_config, $old_config) {
+  if (strpos($name, 'filter.format.') !== 0) {
+    return FALSE;
+  }
+
+  $filter_format = entity_create('filter_format', $new_config->get());
+  $filter_format->save();
+  return TRUE;
+}
+
+/**
+ * Implements MODULE_config_import_change().
+ */
+function filter_config_import_change($name, $new_config, $old_config) {
+  if (strpos($name, 'filter.format.') !== 0) {
+    return FALSE;
+  }
+
+  list(, , $id) = explode('.', $name);
+  $filter_format = entity_load('filter_format', $id);
+
+  $filter_format->original = clone $filter_format;
+  foreach ($old_config->get() as $property => $value) {
+    $filter_format->original->$property = $value;
+  }
+
+  foreach ($new_config->get() as $property => $value) {
+    $filter_format->$property = $value;
+  }
+
+  $filter_format->save();
+  return TRUE;
+}
+
+/**
+ * Implements MODULE_config_import_delete().
+ */
+function filter_config_import_delete($name, $new_config, $old_config) {
+  if (strpos($name, 'filter.format.') !== 0) {
+    return FALSE;
+  }
+
+  list(, , $id) = explode('.', $name);
+  entity_delete_multiple('filter_format', array($id));
+  return TRUE;

It doesn't appear that any of this is necessary. I've seen this in several conversions, but we didn't need it at all for Views, and I didn't need for porting view modes. If you just implemented it because it looked important, maybe we need better docs?

From config_import_invoke_owner() (which invokes those):
Allow modules to take over configuration change operations for higher-level configuration data.

Ignore #27, that will be fixed in #1806178: Remove code duplication for hook_config_import_*() implementations of config entities

Meta issue for these: #1802750: [Meta] Convert configurable data to ConfigEntity system

Fixed {filter} table name. Added a new remaining task

#1: 1779026.text_formats_configurables.1.patch queued for re-testing.

I'm not fully familiar with the "convert to ConfigEntity" tasks yet, so this cannot be considered a sufficient review.

+++ b/core/modules/filter/filter.admin.inc
@@ -344,9 +341,13 @@ function filter_admin_format_form_validate($form, &$form_state) {
+  $result = filter_format_load($format_name);
+  $filter_formats = entity_load_multiple('filter_format');
+  foreach ($filter_formats as $format) {
+    if ($format->name == $format_name && $format->format != $format_format) {
+      form_set_error('name', t('Text format names must be unique. A format named %name already exists.', array('%name' => $format_name)));
+      break;
+    }

looks like $result is not used at all ?

+++ b/core/modules/filter/filter.module
@@ -282,22 +340,10 @@ function filter_format_save($format) {
+  $return = $format->save();
+

Do we really want to keep a wrapper filter_format_save() on top of $format->save() ? What logic do we have in there that wouldn't rather belong in the FilterFormat::save() method ?

+++ b/core/modules/filter/filter.module
@@ -691,6 +737,21 @@ function _filter_list_cmp($a, $b) {
 /**
+ * Sorts an array of filters by filter weight, module, name.
+ *
+ * Callback for uasort() within filter_formats().
+ */
+function _filter_format_filter_cmp($a, $b) {

It's actually called within filter_list_format().

+++ b/core/modules/filter/filter.module
@@ -761,9 +822,13 @@ function filter_list_format($format_id) {
-      $result = db_query('SELECT * FROM {filter} ORDER BY weight, module, name');
-      foreach ($result as $record) {
-        $filters['all'][$record->format][$record->name] = $record;
+      $filter_formats = filter_formats();
+      foreach ($filter_formats as $filter_format) {
+        foreach ($filter_format->filters as $filter_name => $filter) {
+          $filter['name'] = $filter_name;
+          $filters['all'][$filter_format->format][$filter_name] = (object)$filter;
+        }
+        @uasort($filters['all'][$filter_format->format], '_filter_format_filter_cmp');
       }

Wouldn't filter_list_format($format_id) be now just: load $format entity, return $format->filters ?

+++ b/core/profiles/standard/standard.install
@@ -13,7 +13,7 @@
 function standard_install() {
   // Add text formats.
-  $filtered_html_format = array(
+  $filtered_html_format_config = array(
     'format' => 'filtered_html',
     'name' => 'Filtered HTML',
     'weight' => 0,

@@ -40,10 +40,10 @@ function standard_install() {
       ),
     ),
   );
-  $filtered_html_format = (object) $filtered_html_format;
+  $filtered_html_format = entity_create('filter_format', $filtered_html_format_config);
   filter_format_save($filtered_html_format);
 
-  $full_html_format = array(
+  $full_html_format_config = array(
     'format' => 'full_html',
     'name' => 'Full HTML',
     'weight' => 1,

@@ -65,7 +65,7 @@ function standard_install() {
       ),
     ),
   );
-  $full_html_format = (object) $full_html_format;
+  $full_html_format = entity_create('filter_format', $full_html_format_config);
   filter_format_save($full_html_format);

Shouldn't those be default .yml files ?
Maybe install profiles don't get their default .yml files copied over at install time like modules do ? Is there an issue for this ?

The yml files includes no filter settings ? I would expect at least filter_html to have the list of filtered tags ?

-Wouldn't filter_list_format($format_id) be now just: load $format entity, return $format->filters ?
- Hm, filtes are objects, but the configuration save they as arrays. The conversion is required. I tried to avoid as much as possible api changes.

I guess that would be something for a "filters as plugins" patch to address. Can we then at least leave a @todo in filter_list_format() ?

I didn't have time to do an extensive review, however at a glance I agree with yched's comment in #46 that we should look into refactoring or even removing filter_format_save(). It looks like much of this can be removed or moved.

This will also now need a reroll because of #1763974: Convert entity type info into plugins (see conversion instructions at http://drupal.org/node/1827470.

#54: 1779026.text_formats_configurables.54.patch queued for re-testing.

Rerolled!

Thanks for this great work so far guys. I'd love to see this in before I continue pushing further with WYSIWYG configuration (which will be bound to the format configuration) in #1833716: WYSIWYG: Introduce "Text editors" as part of filter format configuration. Right now that patch just extends the existing Filter module tables, but obviously this patch removes those tables. If we get this patch and the other patch completed, then not only will format configuration be exportable through CMI, but WYSIWYG configuration also (since they become the same thing).

Yay for the end of filter_format_save() !

Cannot do a full-fledged review, but :

+++ b/core/modules/filter/lib/Drupal/filter/Plugin/Core/Entity/FilterFormat.phpundefined
@@ -0,0 +1,155 @@
+    if ($return == SAVED_NEW) {
+      module_invoke_all('filter_format_insert', $this);
+    }
+    else {
+      module_invoke_all('filter_format_update', $this);

I think the Entity system takes care of invoking the proper hook_entity_[entity_type]_[op]() ops during save(), so invoking dedicated hooks shouldn't be needed - which de-facto means the existing hooks are renamed, thus filter.api.php needs to be updated accordingly.

21 days to next Drupal core point release.

Awesome work!

+++ b/core/modules/filter/config/filter.format.plain_text.ymlundefined
@@ -0,0 +1,13 @@
+format: plain_text
+name: Plain text
+weight: 10
+filters:
+  filter_html_escape:
+    weight: 0
+    status: 1
+  filter_url:
+    weight: 1
+    status: 1
+  filter_autop:
+    weight: 2
+    status: 1

+++ b/core/profiles/standard/config/filter.format.filtered_html.ymlundefined
@@ -0,0 +1,16 @@
+format: filtered_html
+name: Filtered HTML
+weight: 0
+filters:
+  filter_url:
+    weight: 0
+    status: 1
+  filter_html:
+    weight: 1
+    status: 1
+  filter_autop:
+    weight: 2
+    status: 1
+  filter_htmlcorrector:
+    weight: 10
+    status: 1
diff --git a/core/profiles/standard/config/filter.format.full_html.yml b/core/profiles/standard/config/filter.format.full_html.yml

diff --git a/core/profiles/standard/config/filter.format.full_html.yml b/core/profiles/standard/config/filter.format.full_html.yml
new file mode 100644
index 0000000..bc01d83

index 0000000..bc01d83
--- /dev/null

--- /dev/null
+++ b/core/profiles/standard/config/filter.format.full_html.ymlundefined

+++ b/core/profiles/standard/config/filter.format.full_html.ymlundefined
+++ b/core/profiles/standard/config/filter.format.full_html.ymlundefined
@@ -0,0 +1,13 @@

@@ -0,0 +1,13 @@
+format: full_html
+name: Full HTML
+weight: 1
+filters:
+  filter_url:
+    weight: 0
+    status: 1
+  filter_autop:
+    weight: 1
+    status: 1
+  filter_htmlcorrector:
+    weight: 10

All Yaml values should all be strings so for example... this is what filter.format.plain_text.yml looks like after I press save in the admin interface after a minimal install.

format: plain_text
name: 'Plain text'
cache: '1'
status: '1'
weight: '10'
roles:
  anonymous: anonymous
  authenticated: authenticated
filters:
  filter_url:
    status: '1'
    weight: '1'
    settings:
      filter_url_length: '72'
    module: filter
  filter_autop:
    status: '1'
    weight: '2'
    module: filter
    settings: {  }
  filter_htmlcorrector:
    status: '0'
    weight: '10'
    module: filter
    settings: {  }
  filter_html_escape:
    status: '1'
    weight: '0'
    module: filter
    settings: {  }
  filter_html:
    status: '0'
    weight: '-10'
    settings:
      allowed_html: '<a> <em> <strong> <cite> <blockquote> <code> <ul> <ol> <li> <dl> <dt> <dd>'
      filter_html_help: '1'
      filter_html_nofollow: '0'
    module: filter
  filter_html_image_secure:
    status: '0'
    weight: '9'
    module: filter
    settings: {  }
langcode: und

If they're not then it's possible that the config system will detect a change when there is none. OTOH: there is a patch to enable variable type persistence in YAML files.

- #68 shows that filters not enabled in the format (status:0) are saved back in the yml file.
The form submit should filter items with status = 0 and not save those.
Meaning, the status flag doesn't need to be part of the yml file, since it will always be 1.
[edit: Ignore this. As per discussion in #70/#75 below, we want to keep config for disabled filters in a format]

- Also, the filters should probably be ordered by weight before writing the file (this should happen within FilterFormat::save(), before calling parent::save())

- Last remark, the input formats shipped in default config should include full settings for the filter_url and filter_html filters, and 'empty array' settings entries for the others, not rely on default values that will get saved back. (i.e : default config should not be written by hand, but be taken from real-life config files saved by the system)

Those three things would make the yml files more diff-friendly. CMI files should be idempotent, i.e. save the config in the UI without changing anything, the file should see no changes.

Not sure about not saving the one's with status 0 - if we don't save them when the user re-enables any custom settings they have made will be lost. Is that what we want and this is not what happens in Drupal 7 (as far as I can discern after a very quick skim)

Right, I guess that's debatable. The current input formats UI behaves the way you describe :
- Configure a filter in an input format, save
- Remove the filter, save
- enable it again, the previous settings for the filter are back in place as default values.

I'd tend to consider that an input format remembering the settings a disabled filter had last time it was enabled, so that they are back again in case it gets re-enabled some day, is not a feature we should really expect from the config system. IMO config files should contain the strict definition that corresponds to the desired run-time behavior, and are not meant to keep track of history - CSVs are here for that.
That's not a behavior we'd expect from an image style, for example - and input formats and image styles are conceptually not really different : an ordered list of configured plugins with a machine name...

Also, this behavior means that the config file for a given input format contains entries for all filters available in the system at the moment it was saved. Enable a module that defines a new filter, then save(load(filter_foo)) (UI or programmatic) adds a new entry in the config file, idempotency is lost. I'm not sure idempotency is a critical behavior of config files, but it would sure help managing deployments and making sense of diffs.

So I guess in the end it should be filter.module's maintainers to make that call.

The last two points in #69 still stand, though.

Filter module maintainer here. :) I didn't look through the patch yet (I know, I know, ... I have to), but I spent some time thinking through the last comments on storing disabled filter settings:

1. Contrary to effects in image styles, available and configurable filters are a fixed list and can be enabled and disabled, but they cannot be added and removed. Idempotency is always good to think about though.
2. The current code retains settings of disabled filters, because users sometimes only want to temporarily test whether disabling a filter has a possible impact on what they want to do.
3. There are advanced filters that have a lot of options and whose settings can be complex to set up — dismissing them on save could be understood as data loss.
4. Worse, temporarily disabling a filter and re-enabling it (without re-configuring it from scratch) would make your site insecure.

So, I personally didn't like to see the disabled filters + settings in the config file either, so I can perfectly get behind the concerns here. However, given Filter module's intentions and also the security aspects involved, the above aspects lead me to the conclusion that we should retain the current behavior. We can investigate whether and how we could optimize that in a separate follow-up issue.

I will try hard to have a look at the latest patch ASAP. Thanks for all the hard work so far!

Dang, forgot to mention: One possible way to address this would be to compare the filter settings against its default settings defined in code upon save, and if the filter is disabled AND the settings are identical to the defaults, only store the filter's position/order/weight within the text format but no settings.

However, that would require new tests to verify that this works as expected, so I still think we should do that in a follow-up issue.

The default configuration in /core/modules/filter/config/*.yml can happily diverge from a full definition and does not have to specify all filters though, since the format will be automatically enhanced when it is saved through the API upon import.

There's a similar discussion about disabled state #1826602: Allow all configuration entities to be enabled/disabled

@sun : Yeah, I really don't like keeping disabled filters in here, but agreed that the current UI behavior makes sense given the security aspects of the input_format UI.
I edited my #69 to remove that request.

The assertion mentioned in #87 is bogus, it tests a behavior that shouldn't exist. It's cleaned up in #1848964: EntityManager should process its definitions before altering them.

The tests and the upgrade path had different names for the formats.
Also, a disabled format will not show up on the permissions page.

I'd like to think this is RTBC, but it should have one more review.

+++ b/core/modules/filter/filter.install
@@ -162,6 +44,47 @@ function filter_update_8000() {
+/**
+ * Drop the {filter} and {filter_format} tables.
+ */
+function filter_update_8002() {
+  db_drop_table('filter');
+  db_drop_table('filter_format');
+}

I think the new standard is to not actually drop these tables in D8 in case any contribs reference it in any way. We should just leave it as is for now and add it to #1860986: Drop left-over tables from 8.x

Ah! Two more test fixes.
Also I removed the db_drop_table(), I'll add it to that issue now.

Patch didn't apply anymore, here's a reroll to HEAD.

Mostly nitpicks

#84.1 - Per #1782244-25: Convert image styles $style array into ImageStyle (extends ConfigEntity), all hook_ENTITY_NAME_CRUD() hooks should not be documented anywhere outside of entity.api.php. So they should be removed from filter.api.php.

+++ b/core/modules/filter/filter.admin.incundefined
@@ -61,13 +61,12 @@ function filter_admin_overview($form) {
+  $filter_formats = filter_formats();

@@ -345,9 +342,12 @@ function filter_admin_format_form_validate($form, &$form_state) {
+  $filter_formats = entity_load_multiple('filter_format');

In situations where we are retrieving all formats, we should probably standardize on using entity_load_multiple(), unless there's a specific reason filter_formats() is used in the first example, in which case we should probably document it.

+++ b/core/modules/filter/filter.installundefined
@@ -119,30 +24,7 @@ function filter_install() {
+  // See core/modules/filter/config/filter.format.plain_text.yml.

I can understand wanting to save the central comment in filter_install(), but having a function exist with nothing but comments in it seems hacky and weird. Maybe we can move it into the @file block?

This is realllly close. Thanks dagmar for your dedication to pushing this through!

Opened #1870642: Consider removing filter_formats() for entity_load_multiple('filter_format') as a follow-up, and this should address the other two. Thanks @heyrocker!

Unassigning so that it doesn't scare away would-be reviewers.

Comment #100!

Updated issue summary.

This looks quite nice already, bunch of smaller things that we can now do in a better way I think.

+++ b/core/modules/filter/filter.admin.incundefined
@@ -345,9 +342,12 @@ function filter_admin_format_form_validate($form, &$form_state) {
-  $result = db_query("SELECT format FROM {filter_format} WHERE name = :name AND format <> :format", array(':name' => $format_name, ':format' => $format_format))->fetchField();
-  if ($result) {
-    form_set_error('name', t('Text format names must be unique. A format named %name already exists.', array('%name' => $format_name)));
+  $filter_formats = entity_load_multiple('filter_format');
+  foreach ($filter_formats as $format) {
+    if ($format->name == $format_name && $format->format != $format_format) {
+      form_set_error('name', t('Text format names must be unique. A format named %name already exists.', array('%name' => $format_name)));
+      break;
+    }

Doesn't need to be resolved here, also didn't check if there was any discussion about this yet, but does this still make sense?

format is what must be unique, what do we care if name (the label) is or is not? You can create as many content types, fields, views, .. with the same label as you want, why not filters?

+++ b/core/modules/filter/filter.installundefined
@@ -162,6 +39,39 @@ function filter_update_8000() {
+ * Migrate filter formats into configuration.

MigrateS?

+++ b/core/modules/filter/filter.moduleundefined
@@ -357,7 +296,8 @@ function filter_format_disable($format) {
 function filter_format_exists($format_id) {
-  return (bool) db_query_range('SELECT 1 FROM {filter_format} WHERE format = :format', 0, 1, array(':format' => $format_id))->fetchField();
+  $formats = entity_load_multiple('filter_format');
+  return !empty($formats[$format_id]);

Similar one, but this certainly can just be a entity_load('filter_format', $format_id) now?

+++ b/core/modules/filter/filter.moduleundefined
@@ -761,9 +721,19 @@ function filter_list_format($format_id) {
+          // Before Conversion to CMI, filter were objects, now they are arrays.
+          // Convert filters back to objects to reduce the impact of changes.
+          // @todo Follow-up: filters should be arrays instead of objects.

Is there an issue for this already? I'd also suggest to make all of this a @todo comment, because the complete comment is related to the @todo.

+++ b/core/modules/filter/filter.moduleundefined
@@ -775,8 +745,9 @@ function filter_list_format($format_id) {
         $filter->title = $filter_info[$name]['title'];
-        // Unpack stored filter settings.
-        $filter->settings = (isset($filter->settings) ? unserialize($filter->settings) : array());
+
+        $filter->settings = isset($filter->settings) ? $filter->settings : array();

Missing the context here, shouldn't this be enforced globally, not just here, when the entity is loaded?

+++ b/core/modules/filter/lib/Drupal/filter/Plugin/Core/Entity/FilterFormat.phpundefined
@@ -0,0 +1,160 @@
+   * Implements Drupal\Core\Entity\EntityInterface::save().
+   */
+  public function save() {
+    $this->name = trim($this->name);
+    $this->cache = _filter_format_is_cacheable($this);
+    if (!isset($this->status)) {
+      $this->status = 1;
+    }
+    if (!isset($this->weight)) {
+      $this->weight = 0;

Uh, doesn't this belong in a storageController::pre/postSave() implementation? Why is it in the class?

+++ b/core/modules/filter/lib/Drupal/filter/Tests/FilterDefaultFormatTest.phpundefined
@@ -42,7 +42,17 @@ function testDefaultTextFormats() {
+    $minimum_weight = NULL;
+    foreach (entity_load_multiple('filter_format') as $format) {
+      if (is_null($minimum_weight)) {
+        $minimum_weight = $format->weight;
+      }
+      else {
+        if ($minimum_weight > $format->weight) {
+          $minimum_weight = $format->weight;
+        }
+      }

This looks quite complicated? Can't we start with $minimum_weight = 0 or something to simplify the loop?

+++ b/core/modules/php/php.installundefined
@@ -9,13 +9,20 @@
-  $format_exists = (bool) db_query_range('SELECT 1 FROM {filter_format} WHERE name = :name', 0, 1, array(':name' => 'PHP code'))->fetchField();
   // Add a PHP code text format, if it does not exist. Do this only for the
   // first install (or if the format has been manually deleted) as there is no
   // reliable method to identify the format in an uninstall hook or in
   // subsequent clean installs.
+  $format_exists = FALSE;
+  $filter_formats = entity_load_multiple('filter_format');
+  foreach ($filter_formats as $format) {
+    if ($format->name == 'PHP code') {
+      $format_exists = TRUE;
+      break;
+    }
+  }
   if (!$format_exists) {
-    $php_format = array(
+    $php_format_config = array(
       'format' => 'php_code',
       'name' => 'PHP code',
       // 'Plain text' format is installed with a weight of 10 by default. Use a
@@ -30,8 +37,8 @@ function php_enable() {

@@ -30,8 +37,8 @@ function php_enable() {
         ),
       ),
     );
-    $php_format = (object) $php_format;
-    filter_format_save($php_format);
+    $php_format = entity_create('filter_format', $php_format_config);

This one should also be like 10x easier if the code relies on the format instead of the name :)

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Tests/TaxonomyTestBase.phpundefined
@@ -51,11 +51,13 @@ function createVocabulary() {
   function createTerm($vocabulary) {
+    $filter_formats = filter_formats();
+    $format = array_pop($filter_formats);
     $term = entity_create('taxonomy_term', array(
       'name' => $this->randomName(),
       'description' => $this->randomName(),
       // Use the first available text format.
-      'format' => db_query_range('SELECT format FROM {filter_format}', 0, 1)->fetchField(),
+      'format' => $format->format,
       'vid' => $vocabulary->id(),

Isn't there a filter_default() function or something like that that could help here? Or just rely on plain_text, which we know exists?

There are more of these below.

+++ b/core/modules/taxonomy/taxonomy.installundefined
@@ -71,7 +71,7 @@ function taxonomy_schema() {
-        'description' => 'The {filter_format}.format of the description.',
+        'description' => 'The Filter Format id of the description.',

Not sure if the "id" is necessary at all or should be "identifier"? Also, filter format should probably be lower case?

Thanks for the thorough review!

I think a third of that feedback is fixable now, which I'll do now.
A good part of the filter-specific, not format-specific stuff, should just be left for #1868772: Convert filters to plugins

The last segment is "now that we have this new system, maybe we can refactor!"
I'd like to hold off until after this AND the filter get in. I can open a follow-up.

This is supposed to just be a conversion to ConfigEntity, then filters can become plugins, THEN we can revisit and refactor the system as a whole.

I'm going to pretend each dreditor hunk in #101 is numbered.

1) Fix in followup

2) From http://drupal.org/node/1354: "Note that the first line should start with an imperative verb, so it will make sense to people running update.php."

3) Fixed in newest patch

4, 5) Fixed in #1868772: Convert filters to plugins

6) Fixed in newest patch

7, 8) Fix in followup

9) Fixed in newest patch

Fix typo.

Okay, @Berdir and I talked more, and I agree now that it's worth cleaning up the php_enable() code

Here are two approaches. If both pass, I think A is cleaner than B.

This is #104-a plus the fix from #1871774: ModuleTestBase::assertModuleConfig() assumes all of module's default config is owned by that module

Ok, I think I'm fine with this, the php.install change looks great :)

RTBC, but it will probably need a re-roll once #1871774 is commited (which is already RTBC).

Thanks for pushing this forward! The patch already looks really good so far.

I did not apply it yet and also did not perform an in-depth comparison of old vs. new code. So what follows is a pure code/patch review. I'll try to perform a deeper review ASAP - Filter module involves various security aspects, so we want to make sure that we do not introduce any kind of problems or regressions here.

+++ b/core/modules/filter/config/filter.format.plain_text.yml
@@ -0,0 +1,45 @@
+format: plain_text
+name: 'Plain text'

Hm. This default format lacks a uuid.

OTOH, default config is installed through the config import process, which in turn passes the raw config through a true entity save operation.

Thus, it's probably fine to omit the uuid. But at the same time, we can also remove all declarations for filters that are not enabled from the default config file.

+++ b/core/modules/filter/filter.install
@@ -162,6 +39,39 @@ function filter_update_8000() {
+function filter_update_8001() {
...
+      'roles' => array_values(user_roles(FALSE, 'use text format ' . $filter_format->format)),

1) Why is this using the values instead of the keys returned by user_roles()? AFAICS, the array keys are the role IDs, the array values are the human-readable role labels. We want to store the role IDs.

2) The default config object has each role ID both as key and value. The update should migrate the formats to use the same values as a manually created format.

Btw, it looks like this module update could be simplified by not fetching the rows into objects, but fetching them into arrays via PDO::FETCHASSOC instead. That, combined with querying only needed columns instead of *, could likely trim down this update to a few lines only.

+++ b/core/modules/filter/filter.module
@@ -456,13 +395,14 @@ function filter_formats($account = NULL) {
+      @uasort($formats['all'], 'Drupal\Core\Config\Entity\ConfigEntityBase::sort');

@@ -761,9 +720,19 @@ function filter_list_format($format_id) {
+        @uasort($filters['all'][$filter_format->format], '_filter_format_filter_cmp');

+++ b/core/modules/filter/lib/Drupal/filter/FilterFormatStorageController.php
@@ -0,0 +1,86 @@
+    @uasort($entity->filters, '_filter_format_filter_cmp');

Can we remove the error suppression here?

+++ b/core/modules/filter/lib/Drupal/filter/FilterFormatStorageController.php
@@ -0,0 +1,86 @@
+  protected function preSave(EntityInterface $entity) {
...
+    if (!isset($entity->status)) {
+      $entity->status = 1;
+    }
+    if (!isset($entity->weight)) {
+      $entity->weight = 0;
+    }
...
+    if (!isset($entity->filters)) {
+      $entity->filters = array();
+    }

The current situation with default values for entities is not really clear to me. Is there any particular reason for why we're not setting these default values as the default values of the entity properties? I.e., in the same way the Entity::$langcode property has a default value of 'und'?

+++ b/core/modules/filter/lib/Drupal/filter/FilterFormatStorageController.php
@@ -0,0 +1,86 @@
+      // Sort filters properties by key, to minimize diff issues.
+      ksort($entity->filters[$name]);

Hm. Our past attempts for (prematurely) optimizing for this went all wrong. I wonder whether we should rather remove this ksort() for now.

+++ b/core/modules/filter/lib/Drupal/filter/FilterFormatStorageController.php
@@ -0,0 +1,86 @@
+    // Sort filters by enabled/disabled first then by weight
+    @uasort($entity->filters, '_filter_format_filter_cmp');

It looks like we're doing this sorting both on save and load now. The sorting in filter_list_format() is critical, so I think we can/should drop the additional sorting on save, and perhaps replace it with ksort() instead, so filter settings can be found more easily in a config file.

+++ b/core/modules/filter/lib/Drupal/filter/FilterFormatStorageController.php
@@ -0,0 +1,86 @@
+    if (!empty($entity->status)) {
+      // Save user permissions.

Why are permissions only updated on save when the format is enabled?

+++ b/core/modules/filter/lib/Drupal/filter/Plugin/Core/Entity/FilterFormat.php
@@ -0,0 +1,101 @@
+  public $format;
...
+  public $name;

We should add a @todo for these to rename them to $id and $label, and create a follow-up issue for doing so.

+++ b/core/modules/filter/lib/Drupal/filter/Plugin/Core/Entity/FilterFormat.php
@@ -0,0 +1,101 @@
+  public $uuid = NULL;

Let's remove the default value here.

+++ b/core/modules/filter/lib/Drupal/filter/Plugin/Core/Entity/FilterFormat.php
@@ -0,0 +1,101 @@
+   * An array of name value pairs of the enabled filters for this text format.
+   *
+   * Each element of this array must contain at least the following values:
+   *   - weight: Weight of filter within format.
+   *   - settings: An array of name value pairs that store the filter settings
+   *     for the specific format.
...
+  public $filters = array();

Hm. The documentation here could use some polishing. Let's clarify that the array keys are IDs of filters as registered in hook_filter_info(), and that there is an additional 'module' key denoting the module name that provides a filter for each filter in an existing format, but the module key can be omitted when creating a new format.

+++ b/core/modules/filter/lib/Drupal/filter/Plugin/Core/Entity/FilterFormat.php
@@ -0,0 +1,101 @@
+   * Implements \Drupal\Core\Entity\EntityInterface::id().
+   */
+  public function id() {
+    return $this->format;

It looks like we're missing a corresponding label() method override.

+++ b/core/modules/filter/lib/Drupal/filter/Tests/FilterAPITest.php
@@ -26,7 +26,7 @@ function setUp() {
-    $filtered_html_format = array(
+    $filtered_html_format_config = array(

@@ -41,11 +41,11 @@ function setUp() {
-    $filtered_html_format = (object) $filtered_html_format;
-    filter_format_save($filtered_html_format);
+    $filtered_html_format = entity_create('filter_format', $filtered_html_format_config);
+    $filtered_html_format->save();

Most of these test changes look unnecessarily complex to me... Why didn't we simply replace them as follows?

-$foo_format = array(
-  ...
-);
-$foo_format = (object) $foo_format;
-filter_format_save($foo_format);

+$foo_format = entity_create('filter_format', array(
  ...
+));
+$foo_format->save();

In other words: Let's remove the intermediate $foo_format_config variable and directly invoke entity_create() with the existing array instead, followed by a simple ->save().

+++ b/core/modules/filter/lib/Drupal/filter/Tests/FilterCrudTest.php
@@ -86,16 +84,6 @@ function testTextFormatCrud() {
-    // Verify text format database record.
-    $db_format = db_select('filter_format', 'ff')

Hm. The proper replacement for these test assertions would probably have been to load the raw config() object; i.e., replacing the storage assertions with new storage assertions. However, it is probably OK to drop them entirely. I can't remember why I added those originally.

+++ b/core/modules/simpletest/lib/Drupal/simpletest/Tests/DrupalUnitTestBaseTest.php
@@ -74,8 +74,7 @@ function testEnableModulesLoad() {
   function testEnableModulesInstall() {
-    $module = 'filter';
-    $table = 'filter';
+    $module = 'field';

@@ -88,10 +87,6 @@ function testEnableModulesInstall() {
-    $this->assertFalse(db_table_exists($table), "'$table' database table not found.");
-    $schema = drupal_get_schema($table);
-    $this->assertFalse($schema, "'$table' table schema not found.");

@@ -101,10 +96,6 @@ function testEnableModulesInstall() {
-    $this->assertTrue(db_table_exists($table), "'$table' database table found.");
-    $schema = drupal_get_schema($table);
-    $this->assertTrue($schema, "'$table' table schema found.");

Sorry, these changes are wrong, since they remove essential test coverage. We need to replace 'filter' with some other simple module that has permissions and a database schema.

+++ b/core/modules/system/lib/Drupal/system/Tests/Module/ModuleTestBase.php
@@ -99,7 +99,7 @@ function assertModuleConfig($module) {
     $module_file_storage = new FileStorage($module_config_dir);
-    $names = $module_file_storage->listAll($module . '.');
+    $names = $module_file_storage->listAll();

We added the $module prefix in this line only recently, so I do not understand why it's removed here.

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Tests/TaxonomyTestBase.php
@@ -51,11 +51,13 @@ function createVocabulary() {
+    $format = array_pop($filter_formats);

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Tests/Views/TaxonomyTestBase.php
@@ -125,11 +125,13 @@ protected function mockStandardInstall() {
+    $format = array_pop($filter_formats);

The array_pop() here will probably return the plain_text format in almost all cases - I'm not sure whether that is good or bad. Let's see how it goes.

+++ b/core/profiles/standard/config/filter.format.full_html.yml
@@ -0,0 +1,45 @@
+roles:
+  administrator: administrator
+  anonymous: '0'
+  authenticated: '0'

1) Oh, erm... Did we test whether supplying default config in an installation profile's ./config directory actually works already? I.e., did someone install with Standard profile and see the default formats? This functionality is not covered by tests yet...

2) The 'roles' definition in a format does not look very clean to me. Is there any reason for why we're recording unallowed/disabled roles in the first place? I didn't expect them to be contained.

3) Now that I see diverging 'roles' values, I'm actually fairly sure that the upgrade path is wrong, since it only pollutes values but no keys.

+++ b/core/profiles/standard/standard.install
@@ -382,6 +326,7 @@ function standard_install() {
   // Enable default permissions for system roles.
+  $filtered_html_format = filter_format_load('filtered_html');
   $filtered_html_permission = filter_permission_name($filtered_html_format);
   user_role_grant_permissions(DRUPAL_ANONYMOUS_RID, array('access content', 'access comments', $filtered_html_permission));
   user_role_grant_permissions(DRUPAL_AUTHENTICATED_RID, array('access content', 'access comments', 'post comments', 'skip comment approval', $filtered_html_permission));

Unless I'm missing something, FilterFormat::save() will already grant the necessary user permissions, according to the roles defined in the format — as a result, we shouldn't have to grant those permissions manually anymore...?

I'll respond to things not addressed in the patch.

I didn't touch the error suppresion on the uasorts, because those are specific to filters, not formats, and it's best handled in #1868772: Convert filters to plugins
Same with the ksort()

The user permission saving came is triggered from the format configuration form, which can only be performed on active formats.

I didn't touch the FilterFormat::$filters docs either, that's also handled by the filter conversion.

Entity::id() doesn't check entity keys for performance, but Entity::label() does, so no need for an override.

Whoops, didn't restore testEnableModulesInstall() all the way.

I think this covers enough of your feedback to forge ahead. I'll now go update the filter issue to list the things I've asked to follow-up on there.

+++ b/core/modules/filter/filter.installundefined
@@ -162,6 +39,32 @@ function filter_update_8000() {
+    $filter_format += array(

Why do we de '+=' here? Shouldn't just do "$filter_format = array(..." in each loop?

+++ b/core/modules/filter/filter.moduleundefined
@@ -761,9 +720,19 @@ function filter_list_format($format_id) {
+          // @todo Follow-up: filters should be arrays instead of objects.

the @todo says the opposite of the docs before it.

+++ b/core/modules/filter/filter.installundefined
@@ -162,6 +39,32 @@ function filter_update_8000() {
+  $result = db_query('SELECT * FROM {filter_format}', array(), array('fetch' => PDO::FETCH_ASSOC));
+  foreach ($result as $filter_format) {
...
+    $filter_format += array(

It's adding in extra values into each result from the db_query()

The @todo is implying that we shouldn't have to cast to an object on the following line. Either way, that's all handled in the filters-as-plugins patch.

#115: formats-1779026-115.patch queued for re-testing.

+++ b/core/modules/filter/filter.installundefined
@@ -162,6 +39,32 @@ function filter_update_8000() {
 /**
+ * Migrate filter formats into configuration.
+ *
+ * @ingroup config_upgrade
+ */
+function filter_update_8001() {
+  $result = db_query('SELECT * FROM {filter_format}', array(), array('fetch' => PDO::FETCH_ASSOC));
+  foreach ($result as $filter_format) {
+    // Find the settings for this format.
+    $filters = array();
+    $settings = db_query('SELECT * FROM {filter} WHERE format = :format', array(':format' => $filter_format['format']), array('fetch' => PDO::FETCH_ASSOC));
+    foreach ($settings as $setting) {
+      $setting['settings'] = unserialize($setting['settings']);
+      $filters[$setting['name']] = $setting;
+    }
+
+    // Save the config object.
+    $filter_format += array(
+      'filters' => $filters,
+      'roles' => drupal_map_assoc(array_keys(user_roles(FALSE, 'use text format ' . $filter_format['format']))),
+    );
+    $format = entity_create('filter_format', $filter_format);
+    $format->save();
+  }
+}

None of the other conversions have used entity_create as it depends on entity_get_info which uses module hooks and therefore is not safe to use during an update.

contact_update_8001() is a good template.

Thanks @alexpott!

Whoops, that install change was broken. Yay for working tests! :)

#130: format-1779026-130.patch queued for re-testing.

Now filter_update_8000() is missing creating the manifest files :)

There is a helper function to do this... http://api.drupal.org/api/drupal/core%21includes%21update.inc/function/u...

[edit: crosspost...]

@tim: you need to create manifest entries too, with update_config_manifest_add().

That one's so easily forgotten, I wish we had a more integrated solution here. Something like ConfigEntityUpdateHelper extends Config, where $config->save() takes care of adding the UUID and creating the manifest entry (i.e what $config_entity->save() does, but in an update-kosher way).
Not for that issue, though...

Or some tests to fail when I don't do it...

Yeah, but not sure how that could be done - at the end of update, check that all existing config files for all existing entity types that are also config entities have a manifest entry ? Tricky...

Okay, added. That wasn't too bad :)

Whoops! I confused the entity with the config object. Just using the id directly.

This should be good to go!

Attempted reroll after the blocks patch.

+++ b/core/modules/filter/filter.moduleundefined
@@ -170,6 +171,56 @@ function filter_menu() {
+function filter_config_import_create($name, $new_config, $old_config) {

I know womse stuff is changed regarding these functions. Are they still needed here?

Can we have more reviews? :p

I plan to work on this and tweak it a little in the next few hours this evening. Unassign me if I don't deliver today.

Here we go. Upfront, please note that I'm participating in this issue as Filter module maintainer, not as Configuration system maintainer, which is a slight, but noteworthy difference, and partially, conflict of interest. I mostly ignored the config system perspective and solely focused on Filter module's interests. This means I care less about the config conversion, but much more about the API/UI/sanity/security/documentation/whatnot aspects of Filter module, in order to ensure that existing logic is still contained and the module continues to function as intended.

Created a (rather major) API clean-up follow-up task for this issue:
#1881664: Clean up and complete text format config entity conversion

Reverted the change to ModuleTestBase, which means that this issue is now blocked by:
#1850158: Bugged assumption in ModuleTestBase::assertModuleConfig()

Once that issue is fixed, this patch should pass. Assuming attached patch comes back green, I'd consider this patch to be RTBC from a Filter module/API perspective.

Though let's be clear about the overall, resulting state: As mentioned and linked to above, this change will cause a large amount of follow-up clean-up and polishing work to be done. That's good, appreciated, and also expected. :) However, it should be clear for everyone that this commit will leave the Filter module/API/code in a half-baked state, so the clean-up absolutely has to happen before D8 gets out.

Summary of changes: (apparently, my disciplined commit log is much larger than the interdiff :P)

1. Removed obsolete config import callbacks.
2. Moved explanation for plain_text fallback format into default config file.
3. Changed filter_update_8001() to query and use explicit format and filter data instead of wildcards.
4. Fixed phpDoc in FilterFormat entity class and added docs for uncommon aspects.
5. Ensure consistent definition order for filter properties and simplify format saving.
6. Moved filters uasort() callback into FilterFormat entity class and removed error suppression.
7. Simplified minimum weight determination in FilterDefaultFormatTest.
8. Reverted bogus ModuleTestBase::assertModuleConfig() change. See #1850158: Bugged assumption in ModuleTestBase::assertModuleConfig().
9. Removed default filter weights and default settings from all default format config files.
10. Fixed foreign key schema column descriptions.
11. Changed 'roles' configuration into a simple list of allowed roles.
12. Use Entity methods in new code where easily possible.
13. Fixed missing 'name' in $filters on save, and bogus form values in 'filters'.
14. Updated code for new filter info 'default settings' default value.

The EnableDisableTest failures are expected and will go away with #1850158: Bugged assumption in ModuleTestBase::assertModuleConfig()

I need to study the format permission failures though. The more I think about it, the less I'm sure whether it is legit to store and factually duplicate the allowed user roles within a format's configuration, whereas the actual access is stored + updated elsewhere. (This wasn't the case before - the user roles were dynamically fetched for the format admin form and filter_format_save() performed the remaining wiring between Filter API and User Access API - the actual values, however, weren't stored in the {filter_format} table.)

#143: filter.config.143.patch queued for re-testing.

Found the culprit.

1. Fixed user permissions for disabled formats cannot be updated.
2. Removed error suppression from uasort() in filter_formats().

I consider this patch RTBC, if it comes back green.

+++ b/core/modules/filter/lib/Drupal/filter/Tests/FilterHooksTest.phpundefined
@@ -29,10 +29,6 @@ public static function getInfo() {
-  function setUp() {
-    parent::setUp();
-  }

Aw, I was rather attached to that method! :)

---

I'm not sure what could have broken sorting, was it really the error suppression?

I was just looking into that and tried to debug it in the past minutes...

I don't know why the FilterDefaultFormatTest passed before, but based on the debugging steps so far, we're definitely facing the problem space of #145 — i.e., user role access is stored additionally within formats, next to actual user permissions, which means duplicate data/housekeeping. The reverse housekeeping doesn't happen right now - when granting a user role the permission to use a text format, then the text format is not updated accordingly.

And that's essentially what happens in that test. Now I'm not sure what to do. I'd think the most safe + sensible compromise might be this:

1) Keep the $roles property for formats. [EDIT: Because it really really simplifies default format configurations for installation profiles + tests, and that's almost the most beautiful part of this patch. :)]

2) Populate it on load, based on the actual user role permissions.

3) Update user role permissions according to $roles on save.

4) Potentially even export/save the value into the config object, but essentially, it's just a historical track record and we ignore it.

Alright, attached patch implements #151 and should come back green.

Fixed FilterFormat::$roles are not updated when user role permissions are changed.

The test failures are caused by this form submit handler:

+++ b/core/modules/filter/filter.admin.inc

 function filter_admin_overview_submit($form, &$form_state) {
+  $filter_formats = filter_formats();
...
+      $filter_formats[$id]->set('weight', $data['weight']);
+      $filter_formats[$id]->save();

When replacing filter_formats() with entity_load_multiple('filter_format'), then tests are passing.

For some unknown reason, the formats contained in filter_formats() do not contain the $roles they ought to contain....

...gotcha.

The test performs these actions:

1) Create a format via UI.
2) Grant permissions to test users/roles via User Role API.
3) Change the format weights via UI.

filter_formats_reset() is not invoked between 1) and 3).

The filter admin overview UI loads the formats via filter_formats(), which has its independent cache. That cache is not updated when the user role permissions are changed. Therefore, the previously contained formats are still cached and still not assigned to any roles, since the cache is outdated.

For some reason, entity_load_multiple() doesn't hit that cache problem, but technically it should, too, since off-hand I can't see why the configuration system cache would be invalidated by changing user permissions. (Config entities are not cached, since they'd duplicate the Config cache.)

Hm.

D'oh. It's exactly the fact that entity_load_multiple() is NOT cached, which makes the difference.

Essentially, each config entity is reloaded from (cached) storage whenever it is loaded. The entity itself, however, is not cached, since the entity cache is explicitly disabled in ConfigStorageController. (Duh, that might be another, quite heavy performance optimization...)

And that's why entity_load_multiple() reloads all the formats and re-attaches the actual and currently granted user roles onto $roles.

Whereas filter_formats() is not reloaded and still based on the preexisting cache, which doesn't get flushed, just because user role permissions are changed.

Proper usage of cache tags for filter_formats() + proper invalidation in user_role_*() might be the answer here.

Otherwise, we'd have to drop the $roles property, which I wouldn't like to do. :-/

FYI: #1885830: Enable static caching for config entities

I still need to sleep over it, but I'm relatively sure this boils down to two options only:

1) Proper usage of cache tags — which means to patch the User Role API, as well as the Cache Tags API itself, since the tags are currently bound to a specific cache bin, but the User Role API wouldn't know about Filter module and which cache bins it uses. (Apparently, that cache tags problem came up before, so it would be a very good idea to resolve that in either case.)

2) Remove $roles from filter format config entities. All we *could* still support for convenience would be a super-temporary $roles property that has an impact during the creation of a format only, and which would really only exist for DX/convenience of creating new formats, but would otherwise not be part of text format entities, nor their storage.

#2 sounds like a more reasonable first step, and #1 could be a follow-up.

Limit FilterFormat::$roles to newly inserted entities; restore form submit handling for user roles.

i.e., #157.2.

If this comes back green, we should be ready to go here.

Fixed bogus variable name.

oh, wow, stupid me. Finally figured out what's going on ;)

1. Moved filter_test.module into Filter module.
2. Fixed protected FilterFormat::$roles cannot be accessed without ::get(). Added tests. :)

Fixed newborn MetadataGeneratorTest of Edit module.

This one should finally come back green. :)

Good catch in #163! Editor has some weird stuff in it.

Whoops.

Fixed EditorManagerTest.

Unfortunately, there's no memory backend implementation for path.alias yet, so every DUTB test that installs Filter module needs to install the {url_alias} database schema. We can clean this up later.

#163: Woah, how did those indentation errors still exist? :O I probably introduced those. I really need to work on setting up git commit hooks that do coding standards check some time… :) In any case, my apologies for that weirdness. And YAY, green patch!

I just reviewed this again and it looks really great to me. Given that sun and timplunkett are both good lets get this in!

#171: filter.config.171.patch queued for re-testing.

This is really nice clean-up, thanks! Spent about 20 minutes on this patch but it was very boring because there was nothing to complain about. :D Even came with an upgrade path + tests.

Committed and pushed to 8.x. Thanks!

This will need a change notice.

Did we lose the format property on filter objects?

When the filter object is passed into my filter process callback function the format property is now gone. I'm guessing it is related to this change.

I'll be posting a patch for #1868772: Convert filters to plugins soon, and that will completely revamp filters anyway.

We can quickly fix that for now though, independently of that conversion issue.

I've unpostponed #1881664: Clean up and complete text format config entity conversion.

Lastly, I've also filed #1893730: Provide an in-memory mock implementation of Path\AliasManager for tests and update.php

We can quickly fix that for now though, independently of that conversion issue.

In the other issue it was suggested to discuss a removal in a follow-up to that issue. A big +1 for quickly reverting the removal, even if the removal may make a lot of sense. Why make these conversions harder on contrib?

The D7 practice of selecting all table values and then not unsetting foreign keys may lead to more cases of properties getting lost without discussion when there are no use cases within core.

http://drupal.org/node/1910176 is start of a change notice for review. I largely copied it from the change notice for [#1907430]. There may be more to add but this covers everything in the issue summary at least.

This was (thankfully) a pretty straight conversion, so heyrocker's change notice was spot on. #1868772: Convert filters to plugins is where more of the API functions will be removed/changed.