Part of meta issue #1802750: [Meta] Convert configurable data to ConfigEntity system

Problem/Motivation

Text formats should be configuration so we can import them using the new configuration system.

Proposed resolution

Convert Text Formats into Configurables.

Remaining tasks

  • Create a FilterFormat class. [done]
  • Update all the functions that works with StdClass to work with instances of FilterFormat. [done]
  • Update the creation of default Filter formats in filter.install and Standard profile. [done]
  • Provide an upgrade path. [done]
  • Convert variable filter_fallback_format to the new Configuration system. [done in other issue]
  • Write upgrade tests.

Followups

User interface changes

None.

API changes

filter_format_save() is replaced by Drupal\filter\Plugin\Core\Entity\FilterFormat::save();
Filter functions that before expect an object now expects a FilterFormat instance.

{filter} and {filter_format} tables are removed, the settings of a filter are now part of FilterFormat::$filter.

Files: 
CommentFileSizeAuthor
#178 drupal8.filter-format.178.patch683 bytessun
PASSED: [[SimpleTest]]: [MySQL] 50,808 pass(es). View
#171 filter.config.171.patch81.61 KBsun
PASSED: [[SimpleTest]]: [MySQL] 50,624 pass(es). View
#171 interdiff.txt962 bytessun
#169 format-1779026-169.patch81.55 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 50,820 pass(es), 1 fail(s), and 0 exception(s). View
#169 interdiff.txt1.66 KBtim.plunkett
#167 format-1779026-167.patch81.2 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/editor/lib/Drupal/editor/Tests/EditorLoadingTest.php. View
#167 interdiff.txt3.34 KBtim.plunkett
#165 filter.config.165.patch77.85 KBsun
FAILED: [[SimpleTest]]: [MySQL] 50,797 pass(es), 4 fail(s), and 0 exception(s). View
#165 interdiff.txt1.29 KBsun
#163 filter.config.163.patch76.56 KBsun
FAILED: [[SimpleTest]]: [MySQL] 50,717 pass(es), 0 fail(s), and 1 exception(s). View
#163 interdiff.txt6.49 KBsun
#161 filter.config.161.patch71.1 KBsun
FAILED: [[SimpleTest]]: [MySQL] 50,636 pass(es), 25 fail(s), and 3 exception(s). View
#161 interdiff.txt583 bytessun
#159 filter.config.159.patch70.9 KBsun
FAILED: [[SimpleTest]]: [MySQL] 49,454 pass(es), 63 fail(s), and 17 exception(s). View
#159 interdiff.txt6.2 KBsun
#152 filter.config.152.patch71.69 KBsun
FAILED: [[SimpleTest]]: [MySQL] 49,435 pass(es), 2 fail(s), and 0 exception(s). View
#152 interdiff.txt4.42 KBsun
#148 filter.config.148.patch69.31 KBsun
FAILED: [[SimpleTest]]: [MySQL] 50,721 pass(es), 2 fail(s), and 0 exception(s). View
#148 interdiff.txt4.72 KBsun
#143 filter.config.143.patch66.02 KBsun
FAILED: [[SimpleTest]]: [MySQL] 50,547 pass(es), 5 fail(s), and 4 exception(s). View
#143 interdiff.txt25.41 KBsun
#140 format-1779026-140.patch65.25 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 50,555 pass(es). View
#139 format-1779026-139.patch65.74 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 50,090 pass(es). View
#139 interdiff.txt476 bytestim.plunkett
#137 format-1779026-137.patch65.73 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 49,312 pass(es), 22 fail(s), and 21 exception(s). View
#137 interdiff.txt393 bytestim.plunkett
#130 format-1779026-130.patch65.66 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 50,185 pass(es). View
#130 interdiff.txt1.2 KBtim.plunkett
#128 format-1779026-128.patch65.08 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 50,053 pass(es), 2 fail(s), and 0 exception(s). View
#128 interdiff.txt1.2 KBtim.plunkett
#115 interdiff.txt1.67 KBtim.plunkett
#115 formats-1779026-115.patch64.89 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 49,140 pass(es), 4 fail(s), and 0 exception(s). View
#113 formats-1779026-113.patch65.83 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 49,328 pass(es), 1 fail(s), and 2 exception(s). View
#113 interdiff.txt15.38 KBtim.plunkett
#110 formats-1779026-110.patch67.3 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 49,229 pass(es). View
#105 formats-1779026-104-a.patch66.64 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 49,172 pass(es), 3 fail(s), and 0 exception(s). View
#105 interdiff-a.txt1.98 KBtim.plunkett
#105 formats-1779026-104-b.patch66.09 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 49,212 pass(es). View
#105 interdiff-b.txt797 bytestim.plunkett
#103 format-1779026-103.patch66.26 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 49,237 pass(es). View
#103 interdiff.txt7.55 KBtim.plunkett
#100 formats-1779026-100.patch65.35 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 49,141 pass(es). View
#100 interdiff.txt2.82 KBtim.plunkett
#98 1779026-text-format-configurables-98.patch63.42 KBheyrocker
PASSED: [[SimpleTest]]: [MySQL] 49,327 pass(es). View
#97 formats-1779026-97.patch63.42 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 49,230 pass(es). View
#97 interdiff.txt2.77 KBtim.plunkett
#94 interdiff.txt8.61 KBtim.plunkett
#94 formats-1779026-94.patch61.91 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 49,229 pass(es), 3 fail(s), and 2 exception(s). View
#92 filter-1779026-92.patch63.56 KBdagmar
FAILED: [[SimpleTest]]: [MySQL] 49,105 pass(es), 19 fail(s), and 2 exception(s). View
#90 filter-1779026-90.patch63.85 KBdagmar
FAILED: [[SimpleTest]]: [MySQL] 48,800 pass(es), 36 fail(s), and 51 exception(s). View
#90 interdiff.txt7.63 KBdagmar
#87 filter-1779026-87.patch59.65 KBdagmar
FAILED: [[SimpleTest]]: [MySQL] 48,752 pass(es), 5 fail(s), and 0 exception(s). View
#87 interdiff.txt1.58 KBdagmar
#85 filter-1779026-85.patch58.04 KBdagmar
FAILED: [[SimpleTest]]: [MySQL] 48,604 pass(es), 7 fail(s), and 2 exception(s). View
#84 filter-1779026-85.patch58.74 KBdagmar
PASSED: [[SimpleTest]]: [MySQL] 48,126 pass(es). View
#84 interdiff-76-85.txt15.77 KBdagmar
#82 filter-1779026-82.patch57.41 KBdagmar
FAILED: [[SimpleTest]]: [MySQL] 48,097 pass(es), 2 fail(s), and 0 exception(s). View
#82 interdiff-76-82.txt13.82 KBdagmar
#78 filter-1779026-78.patch52.46 KBdagmar
FAILED: [[SimpleTest]]: [MySQL] 48,093 pass(es), 3 fail(s), and 0 exception(s). View
#78 interdiff.txt8.02 KBdagmar
#76 filter-1779026-76.patch50.34 KBdagmar
FAILED: [[SimpleTest]]: [MySQL] 47,964 pass(es), 0 fail(s), and 1 exception(s). View
#67 filter-1779026-66.patch50.51 KBdagmar
PASSED: [[SimpleTest]]: [MySQL] 47,754 pass(es). View
#67 interdiff.txt1.79 KBdagmar
#64 filter-1779026-64.patch50.51 KBdagmar
FAILED: [[SimpleTest]]: [MySQL] 47,565 pass(es), 9 fail(s), and 0 exception(s). View
#64 interdiff.txt1.15 KBdagmar
#62 filter-1779026-62.patch50.55 KBdagmar
FAILED: [[SimpleTest]]: [MySQL] 47,316 pass(es), 124 fail(s), and 84 exception(s). View
#62 interdiff.txt837 bytesdagmar
#60 filter-1779026-60.patch50.54 KBdagmar
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View
#60 interdiff.txt442 bytesdagmar
#58 filter-1779026-58.patch42.01 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 46,274 pass(es). View
#58 interdiff.txt2.19 KBtim.plunkett
#54 1779026.text_formats_configurables.54.patch41.94 KBdagmar
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View
#54 interdiff.txt858 bytesdagmar
#52 1779026.text_formats_configurables.52.patch41.94 KBdagmar
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/profiles/standard/standard.install. View
#52 interdiff.txt2.72 KBdagmar
#50 1779026.text_formats_configurables.50.patch40.04 KBdagmar
FAILED: [[SimpleTest]]: [MySQL] 46,072 pass(es), 3 fail(s), and 73 exception(s). View
#50 interdiff.txt678 bytesdagmar
#47 1779026.text_formats_configurables.47.patch40.01 KBdagmar
FAILED: [[SimpleTest]]: [MySQL] 45,512 pass(es), 163 fail(s), and 98 exception(s). View
#47 interdiff.txt6.4 KBdagmar
#44 1779026.text_formats_configurables.44.patch36.94 KBdagmar
PASSED: [[SimpleTest]]: [MySQL] 42,705 pass(es). View
#44 interdiff.txt2.16 KBdagmar
#42 1779026.text_formats_configurables.42.patch34.78 KBdagmar
FAILED: [[SimpleTest]]: [MySQL] 42,665 pass(es), 2 fail(s), and 0 exception(s). View
#38 1779026.text_formats_configurables.38.patch34.57 KBdagmar
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1779026.text_formats_configurables.38.patch. Unable to apply patch. See the log in the details link for more information. View
#38 interdiff.txt1.61 KBdagmar
#36 1779026.text_formats_configurables.36.patch34.3 KBdagmar
FAILED: [[SimpleTest]]: [MySQL] 41,987 pass(es), 1 fail(s), and 0 exception(s). View
#34 1779026.text_formats_configurables.34.patch51.66 KBdagmar
PASSED: [[SimpleTest]]: [MySQL] 41,967 pass(es). View
#34 interdiff.txt2.91 KBdagmar
#30 1779026.text_formats_configurables.30.patch50.39 KBdagmar
FAILED: [[SimpleTest]]: [MySQL] 42,004 pass(es), 12 fail(s), and 19 exception(s). View
#30 interdiff.txt1.73 KBdagmar
#28 1779026.text_formats_configurables.28.patch49.68 KBdagmar
FAILED: [[SimpleTest]]: [MySQL] 41,968 pass(es), 19 fail(s), and 20 exception(s). View
#28 interdiff.txt4.43 KBdagmar
#23 1779026.text_formats_configurables.23.patch48.88 KBdagmar
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1779026.text_formats_configurables.23.patch. Unable to apply patch. See the log in the details link for more information. View
#23 interdiff.txt1.58 KBdagmar
#21 1779026.text_formats_configurables.21.patch47.95 KBdagmar
FAILED: [[SimpleTest]]: [MySQL] 41,322 pass(es), 21 fail(s), and 28 exception(s). View
#21 interdiff.txt1.25 KBdagmar
#19 1779026.text_formats_configurables.19.patch48.01 KBdagmar
FAILED: [[SimpleTest]]: [MySQL] 41,137 pass(es), 44 fail(s), and 27 exception(s). View
#16 1779026.text_formats_configurables.16.patch41.94 KBdagmar
FAILED: [[SimpleTest]]: [MySQL] 39,577 pass(es), 84 fail(s), and 88 exception(s). View
#14 1779026.text_formats_configurables.14.patch38.58 KBdagmar
FAILED: [[SimpleTest]]: [MySQL] 39,580 pass(es), 51 fail(s), and 65 exception(s). View
#12 1779026.text_formats_configurables.12.patch37.3 KBdagmar
FAILED: [[SimpleTest]]: [MySQL] 38,222 pass(es), 472 fail(s), and 815 exception(s). View
#8 1779026.text_formats_configurables.8.patch39.72 KBdagmar
FAILED: [[SimpleTest]]: [MySQL] 38,121 pass(es), 558 fail(s), and 854 exception(s). View
#6 1779026.text_formats_configurables.4.patch27.53 KBdagmar
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: tests were executed, but no results were found. View
#1 1779026.text_formats_configurables.1.patch11.81 KBdagmar
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1779026.text_formats_configurables.1.patch. Unable to apply patch. See the log in the details link for more information. View

Comments

dagmar’s picture

Assigned: Unassigned » dagmar
Status: Active » Needs review
FileSize
11.81 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1779026.text_formats_configurables.1.patch. Unable to apply patch. See the log in the details link for more information. View

I'm working on this. I'm posting a work in progress patch just to get some early feedback to see if I'm implemeting this in the right way.

sun’s picture

Issue tags: +Configurables

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.

dagmar’s picture

Thanks!. I'm working on this patch only on Friday, so the updates will usually on Friday.

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.

Ok, so for Filtered HTML we should have something like this?

filter.filtered_html.yml

format: filtered_html
name: Filtered HTML
cache: 1
status: 1
weight: 0
filters:
  filter_url:
    module: filter
    weight: 0
    settings:
      url_length: 72
  filter_html:
    module: filter
    weight: 0
    settings: 
      allowed_html: <a> <em> <strong> <cite> <blockquote> <code> <ul> <ol> <li> 
      html_help: 1
      html_nofollow: 0
  filter_autop:
    module: filter
    weight: 0
    settings: []

If this is correct, then I should only implement one Class FilterFormat that extends ConfigEntity.

This change presents several API changes. Because at this moment Formats can be loaded without need to load the filters and its configurations.

Is this approach correct? I mean, load all the info of all the filters only to display the available formats in a select box below the body?

sun’s picture

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. :)

dagmar’s picture

FileSize
27.53 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: tests were executed, but no results were found. View

Ok this is what I have at this moment. Still needs a lot of work. I will try to do something else before next Friday :)

dagmar’s picture

FileSize
39.72 KB
FAILED: [[SimpleTest]]: [MySQL] 38,121 pass(es), 558 fail(s), and 854 exception(s). View

The main functionality is working. Let see how many tests fail.

tim.plunkett’s picture

+++ 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()?

dagmar’s picture

FileSize
37.3 KB
FAILED: [[SimpleTest]]: [MySQL] 38,222 pass(es), 472 fail(s), and 815 exception(s). View

Test are failing basically because Form Controller add a 'Save' button instead of 'Save configuration'.

Added changes suggested by @tim.plunkett. Thanks!

dagmar’s picture

FileSize
38.58 KB
FAILED: [[SimpleTest]]: [MySQL] 39,580 pass(es), 51 fail(s), and 65 exception(s). View

Fixed two more tests that work directly with the table {filter}.

Reverted change in 'filter_fallback_format' format.

dagmar’s picture

Issue summary: View changes

Updated issue summary using the Issue Summary Template

dagmar’s picture

FileSize
41.94 KB
FAILED: [[SimpleTest]]: [MySQL] 39,577 pass(es), 84 fail(s), and 88 exception(s). View

Converted filer variables into the new configuration system.

Test will fail due other test are not using the filter API and make they queries directly from {filter} table.

This is my last patch this day. I would like to get some feedback in the following days, I will be working on this issue next Friday.

yched’s picture

Status: Needs review » Needs work

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.

dagmar’s picture

Status: Needs work » Needs review
FileSize
48.01 KB
FAILED: [[SimpleTest]]: [MySQL] 41,137 pass(es), 44 fail(s), and 27 exception(s). View

Moved Conversion of variables into a new issue #1799440: Convert Filter variables to Configuration System.

New patch to fix more tests.

dagmar’s picture

FileSize
1.25 KB
47.95 KB
FAILED: [[SimpleTest]]: [MySQL] 41,322 pass(es), 21 fail(s), and 28 exception(s). View

More fixes.

dagmar’s picture

FileSize
1.58 KB
48.88 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1779026.text_formats_configurables.23.patch. Unable to apply patch. See the log in the details link for more information. View
dagmar’s picture

tim.plunkett’s picture

Status: Needs review » Needs work
Issue tags: +Configuration system, +Configurables
+++ 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.

dagmar’s picture

Status: Needs work » Needs review
FileSize
4.43 KB
49.68 KB
FAILED: [[SimpleTest]]: [MySQL] 41,968 pass(es), 19 fail(s), and 20 exception(s). View
dagmar’s picture

FileSize
1.73 KB
50.39 KB
FAILED: [[SimpleTest]]: [MySQL] 42,004 pass(es), 12 fail(s), and 19 exception(s). View
tim.plunkett’s picture

Status: Needs review » Needs work
xjm’s picture

xjm’s picture

Issue summary: View changes

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

dagmar’s picture

Status: Needs work » Needs review
Issue tags: +Needs tests
FileSize
2.91 KB
51.66 KB
PASSED: [[SimpleTest]]: [MySQL] 41,967 pass(es). View

I think this should fix the remaining tests.

I have included the upgrade path, but the tests for this upgrade path are missing. I'll try to write this tests in the following days.

dagmar’s picture

From #1588422-142: Convert contact categories to configuration system:

One more thing:

For this patch and Contact module it might be acceptable, but for all other ConfigEntity/Configurable conversion issues, please do NOT perform the EntityListController and EntityFormController migrations in the same patch.

The conversion to configuration is complex enough. As in this patch, that conversion usually requires test changes already. The additional entity system changes are moving huge parts of the code into different locations, making the entire thing impossible to review. Due to the test changes required for the config conversion, that means we cannot trust the test results anymore.

So please do not merge the additional entity system changes into a config system conversion patch. The entity system changes can happily be follow-up issues to the config conversion. Mixing them is scope creep and detrimental for making progress.

Thanks!

I'll try to revert the EntityFormController part during this week, some feedback would be appreciated until that.

dagmar’s picture

FileSize
34.3 KB
FAILED: [[SimpleTest]]: [MySQL] 41,987 pass(es), 1 fail(s), and 0 exception(s). View

Removed the EntityFormController, let see if test still pass.

dagmar’s picture

FileSize
1.61 KB
34.57 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1779026.text_formats_configurables.38.patch. Unable to apply patch. See the log in the details link for more information. View
mohit_aghera’s picture

dagmar’s picture

dagmar’s picture

Issue tags: +Needs tests, +Configuration system, +Configurables
FileSize
34.78 KB
FAILED: [[SimpleTest]]: [MySQL] 42,665 pass(es), 2 fail(s), and 0 exception(s). View

Re-rolled

dagmar’s picture

dagmar’s picture

I would like to have some reviews to continue with this issue. Thanks.

yched’s picture

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 ?

dagmar’s picture

FileSize
6.4 KB
40.01 KB
FAILED: [[SimpleTest]]: [MySQL] 45,512 pass(es), 163 fail(s), and 98 exception(s). View

Thanks @yched!

looks like $result is not used at all ?

Remoed.

It's actually called within filter_list_format().

Fixed

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.

Shouldn't those be default .yml files ?

Done.

Thanks @DamZ, @Berdir and @timpunket also for the help in the irc. Now text formats are yml files that are provided by the standard profile and the filter module.

yched’s picture

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() ?

dagmar’s picture

FileSize
678 bytes
40.04 KB
FAILED: [[SimpleTest]]: [MySQL] 46,072 pass(es), 3 fail(s), and 73 exception(s). View

I'm almost sure the fails from #48 are related to a caching issue.

dagmar’s picture

FileSize
2.72 KB
41.94 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/profiles/standard/standard.install. View
dagmar’s picture

FileSize
858 bytes
41.94 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View
heyrocker’s picture

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.

xjm’s picture

tim.plunkett’s picture

Issue tags: +Needs tests, +Configuration system, +Configurables
FileSize
2.19 KB
42.01 KB
PASSED: [[SimpleTest]]: [MySQL] 46,274 pass(es). View

Rerolled!

quicksketch’s picture

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).

dagmar’s picture

Priority: Normal » Major
FileSize
442 bytes
50.54 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

@quicksketch I just readed your issue, lets try to speed up this issue.

This patch removes filter_format_save() and move the logic into Drupal\filter\Plugin\Core\Entity\FilterFormat::save().

Let see if tests still pass.

dagmar’s picture

FileSize
837 bytes
50.55 KB
FAILED: [[SimpleTest]]: [MySQL] 47,316 pass(es), 124 fail(s), and 84 exception(s). View
dagmar’s picture

FileSize
1.15 KB
50.51 KB
FAILED: [[SimpleTest]]: [MySQL] 47,565 pass(es), 9 fail(s), and 0 exception(s). View
yched’s picture

Status: Needs review » Needs work

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.

dagmar’s picture

Status: Needs work » Needs review
FileSize
1.79 KB
50.51 KB
PASSED: [[SimpleTest]]: [MySQL] 47,754 pass(es). View
alexpott’s picture

Status: Needs review » Needs work

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.

yched’s picture

- #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.

alexpott’s picture

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)

yched’s picture

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.

sun’s picture

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!

sun’s picture

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.

andypost’s picture

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

yched’s picture

@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.

dagmar’s picture

Status: Needs work » Needs review
FileSize
50.34 KB
FAILED: [[SimpleTest]]: [MySQL] 47,964 pass(es), 0 fail(s), and 1 exception(s). View

Thanks for the feedback, I'm working to address the suggestions by @yched and @alexpott.

I'm providing a re-roll due #1799440: Convert Filter variables to Configuration System. So I can provide an interdiff later.

dagmar’s picture

FileSize
8.02 KB
52.46 KB
FAILED: [[SimpleTest]]: [MySQL] 48,093 pass(es), 3 fail(s), and 0 exception(s). View

In this patch:

  • Removed hook invokes (Requested in #66). But I didn't updated filter.api.php since hooks names are the same.
  • Config files contains all the data, using valid YAML format. (Requested in #68)
  • Filters in config files are sorted first by enabled and disabled, then by weight (Requested in #69)
  • Fixed REST test (#76)

In my next patch I'll try to write the upgrade path tests.

Also, would be nice if somebody could explain me why filter_format_load is not working in the FilterAdminTest. Seems to be a caching issue, the only way it worked was replacing by entity_load.

dagmar’s picture

Issue summary: View changes

Updated issue summary.

dagmar’s picture

#78: filter-1779026-78.patch queued for re-testing.

dagmar’s picture

Issue tags: +Needs tests, +Configuration system, +Configurables
FileSize
13.82 KB
57.41 KB
FAILED: [[SimpleTest]]: [MySQL] 48,097 pass(es), 2 fail(s), and 0 exception(s). View

Filter remaining test. Started upgrade test (work in progress).

dagmar’s picture

FileSize
15.77 KB
58.74 KB
PASSED: [[SimpleTest]]: [MySQL] 48,126 pass(es). View

(Updated from #78) In this patch:

  • Removed hook invokes (Requested in #66). But I didn't updated filter.api.php since hooks names are the same.
  • Config files contains all the data, using valid YAML format. (Requested in #68)
  • Filters in config files are sorted first by enabled and disabled, then by weight (Requested in #69)
  • Fixed REST test (#76)
  • Started upgrade path. The roles upgrade is missing

Also, would be nice if somebody could explain me why filter_format_load is not working in the FilterAdminTest. Seems to be a caching issue, the only way it worked was replacing by entity_load.

dagmar’s picture

FileSize
58.04 KB
FAILED: [[SimpleTest]]: [MySQL] 48,604 pass(es), 7 fail(s), and 2 exception(s). View

Rerolled

dagmar’s picture

FileSize
1.58 KB
59.65 KB
FAILED: [[SimpleTest]]: [MySQL] 48,752 pass(es), 5 fail(s), and 0 exception(s). View

Fixed some tests Sadly I can't find the cause of the entity_test fails.

This

$this->assertTrue($controller instanceof \Drupal\Core\Entity\EntityAccessController, 'The default entity controller is used for the entity_test entity type.');

Is not related to my patch, but fails. If somebody could point me in the right direction, I can fix this and continue working on the upgrade path.

Related to the removed asserst in the interdiff, the filter table is no available anymore, should I change this table by another one? What module/table should I use to replace those tests?

tim.plunkett’s picture

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.

dagmar’s picture

FileSize
7.63 KB
63.85 KB
FAILED: [[SimpleTest]]: [MySQL] 48,800 pass(es), 36 fail(s), and 51 exception(s). View

Rerolled after #1774332: Better handling of invalid/expired cache entries

This patch attemps to finish the upgrade path, I moved the logic to update permissions from the ui into TextFormats::save().

Also I included the upgrade path for roles.

Sadly I cannot run test locally anymore, seems related to #347988: Move $user->data into own table so I cannot ensure this will pass.

Entity access fails are related to #1848964: EntityManager should process its definitions before altering them

dagmar’s picture

FileSize
63.56 KB
FAILED: [[SimpleTest]]: [MySQL] 49,105 pass(es), 19 fail(s), and 2 exception(s). View

Fixed the problems with the previous patch.

Ok, now the upgrade path tests are working, but the upgrade path itself is not assigning the right permissions. At least test detect this.

I'll try to fix the remaining issues during this week.

tim.plunkett’s picture

Issue tags: -Needs tests
FileSize
61.91 KB
FAILED: [[SimpleTest]]: [MySQL] 49,229 pass(es), 3 fail(s), and 2 exception(s). View
8.61 KB

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.

tstoeckler’s picture

+++ 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

tim.plunkett’s picture

FileSize
2.77 KB
63.42 KB
PASSED: [[SimpleTest]]: [MySQL] 49,230 pass(es). View

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

heyrocker’s picture

FileSize
63.42 KB
PASSED: [[SimpleTest]]: [MySQL] 49,327 pass(es). View

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

heyrocker’s picture

Status: Needs review » Needs work

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!

tim.plunkett’s picture

Assigned: dagmar » Unassigned
Status: Needs work » Needs review
FileSize
2.82 KB
65.35 KB
PASSED: [[SimpleTest]]: [MySQL] 49,141 pass(es). View

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!

Berdir’s picture

Issue summary: View changes

Updated issue summary.

Berdir’s picture

Status: Needs review » Needs work

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?

tim.plunkett’s picture

Assigned: Unassigned » tim.plunkett

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.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
7.55 KB
66.26 KB
PASSED: [[SimpleTest]]: [MySQL] 49,237 pass(es). View

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

tim.plunkett’s picture

Issue summary: View changes

Fix typo.

tim.plunkett’s picture

FileSize
797 bytes
66.09 KB
PASSED: [[SimpleTest]]: [MySQL] 49,212 pass(es). View
1.98 KB
66.64 KB
FAILED: [[SimpleTest]]: [MySQL] 49,172 pass(es), 3 fail(s), and 0 exception(s). View

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.

tim.plunkett’s picture

FileSize
67.3 KB
PASSED: [[SimpleTest]]: [MySQL] 49,229 pass(es). View
Berdir’s picture

Status: Needs review » Reviewed & tested by the community

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).

sun’s picture

Status: Reviewed & tested by the community » Needs work

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...?

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
15.38 KB
65.83 KB
FAILED: [[SimpleTest]]: [MySQL] 49,328 pass(es), 1 fail(s), and 2 exception(s). View

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.

tim.plunkett’s picture

FileSize
64.89 KB
FAILED: [[SimpleTest]]: [MySQL] 49,140 pass(es), 4 fail(s), and 0 exception(s). View
1.67 KB

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

tim.plunkett’s picture

Assigned: tim.plunkett » sun

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.

aspilicious’s picture

+++ 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.

tim.plunkett’s picture

+++ 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.

tim.plunkett’s picture

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

Status: Needs review » Needs work
Issue tags: +Configuration system, +Configurables

The last submitted patch, formats-1779026-115.patch, failed testing.

alexpott’s picture

+++ 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.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
1.2 KB
65.08 KB
FAILED: [[SimpleTest]]: [MySQL] 50,053 pass(es), 2 fail(s), and 0 exception(s). View

Thanks @alexpott!

Status: Needs review » Needs work

The last submitted patch, format-1779026-128.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
1.2 KB
65.66 KB
PASSED: [[SimpleTest]]: [MySQL] 50,185 pass(es). View

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

Status: Needs review » Needs work
Issue tags: -Configuration system, -Configurables

The last submitted patch, format-1779026-130.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
Issue tags: +Configuration system, +Configurables

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

alexpott’s picture

Status: Needs review » Needs work

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...

yched’s picture

Status: Needs work » Needs review

[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...

tim.plunkett’s picture

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

yched’s picture

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...

tim.plunkett’s picture

Assigned: sun » tim.plunkett
FileSize
393 bytes
65.73 KB
FAILED: [[SimpleTest]]: [MySQL] 49,312 pass(es), 22 fail(s), and 21 exception(s). View

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

Status: Needs review » Needs work

The last submitted patch, format-1779026-137.patch, failed testing.

tim.plunkett’s picture

Assigned: tim.plunkett » Unassigned
Status: Needs work » Needs review
FileSize
476 bytes
65.74 KB
PASSED: [[SimpleTest]]: [MySQL] 50,090 pass(es). View

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

This should be good to go!

tim.plunkett’s picture

FileSize
65.25 KB
PASSED: [[SimpleTest]]: [MySQL] 50,555 pass(es). View

Attempted reroll after the blocks patch.

aspilicious’s picture

+++ 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

sun’s picture

Assigned: Unassigned » sun

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.

sun’s picture

Assigned: sun » Unassigned
FileSize
25.41 KB
66.02 KB
FAILED: [[SimpleTest]]: [MySQL] 50,547 pass(es), 5 fail(s), and 4 exception(s). View

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.

Status: Needs review » Needs work

The last submitted patch, filter.config.143.patch, failed testing.

sun’s picture

Assigned: Unassigned » sun

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.)

heyrocker’s picture

Status: Needs work » Needs review
Issue tags: -Configuration system, -Configurables

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

Status: Needs review » Needs work
Issue tags: +Configuration system, +Configurables

The last submitted patch, filter.config.143.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
4.72 KB
69.31 KB
FAILED: [[SimpleTest]]: [MySQL] 50,721 pass(es), 2 fail(s), and 0 exception(s). View

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.

Status: Needs review » Needs work

The last submitted patch, filter.config.148.patch, failed testing.

tim.plunkett’s picture

+++ 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?

sun’s picture

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.

sun’s picture

Status: Needs work » Needs review
FileSize
4.42 KB
71.69 KB
FAILED: [[SimpleTest]]: [MySQL] 49,435 pass(es), 2 fail(s), and 0 exception(s). View

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

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

Status: Needs review » Needs work

The last submitted patch, filter.config.152.patch, failed testing.

sun’s picture

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.

sun’s picture

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. :-/

sun’s picture

sun’s picture

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.

tim.plunkett’s picture

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

sun’s picture

Status: Needs work » Needs review
FileSize
6.2 KB
70.9 KB
FAILED: [[SimpleTest]]: [MySQL] 49,454 pass(es), 63 fail(s), and 17 exception(s). View

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.

Status: Needs review » Needs work

The last submitted patch, filter.config.159.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
583 bytes
71.1 KB
FAILED: [[SimpleTest]]: [MySQL] 50,636 pass(es), 25 fail(s), and 3 exception(s). View

Fixed bogus variable name.

Status: Needs review » Needs work

The last submitted patch, filter.config.161.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
6.49 KB
76.56 KB
FAILED: [[SimpleTest]]: [MySQL] 50,717 pass(es), 0 fail(s), and 1 exception(s). View

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. :)

Status: Needs review » Needs work

The last submitted patch, filter.config.163.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
1.29 KB
77.85 KB
FAILED: [[SimpleTest]]: [MySQL] 50,797 pass(es), 4 fail(s), and 0 exception(s). View

Fixed newborn MetadataGeneratorTest of Edit module.

This one should finally come back green. :)

Status: Needs review » Needs work

The last submitted patch, filter.config.165.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
3.34 KB
81.2 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/editor/lib/Drupal/editor/Tests/EditorLoadingTest.php. View

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

Status: Needs review » Needs work

The last submitted patch, format-1779026-167.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
1.66 KB
81.55 KB
FAILED: [[SimpleTest]]: [MySQL] 50,820 pass(es), 1 fail(s), and 0 exception(s). View

Whoops.

Status: Needs review » Needs work

The last submitted patch, format-1779026-169.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
962 bytes
81.61 KB
PASSED: [[SimpleTest]]: [MySQL] 50,624 pass(es). View

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.

Wim Leers’s picture

#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!

heyrocker’s picture

Status: Needs review » Reviewed & tested by the community

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

tim.plunkett’s picture

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

webchick’s picture

Title: Convert Text Formats to Configuration System » Change notice: Convert Text Formats to Configuration System
Priority: Major » Critical
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

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.

Eric_A’s picture

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.

tim.plunkett’s picture

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

sun’s picture

Status: Active » Needs review
FileSize
683 bytes
PASSED: [[SimpleTest]]: [MySQL] 50,808 pass(es). View

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

Eric_A’s picture

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.

heyrocker’s picture

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.

tim.plunkett’s picture

Title: Change notice: Convert Text Formats to Configuration System » Convert Text Formats to Configuration System
Priority: Critical » Major
Status: Needs review » Fixed
Issue tags: -Needs change record

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.

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

Anonymous’s picture

Issue summary: View changes

Added followups