The following variables have to be converted to the new configuration system.

  • filter_allowed_protocols
  • filter_fallback_format
CommentFileSizeAuthor
#63 filter_variables_cmi-1799440-63.patch8.38 KBAlbert Volkman
#63 interdiff.txt3.92 KBAlbert Volkman
#61 filter_variables_cmi-1799440-61.patch8.25 KBdagmar
#61 interdiff.txt1.76 KBdagmar
#59 filter_variables_cmi-1799440-59.patch7 KBdagmar
#59 interdiff.txt1.16 KBdagmar
#56 filter_variables_cmi-1799440-56.patch6.81 KBdagmar
#56 interdiff.txt2.35 KBdagmar
#53 interdiff.txt3.23 KBdagmar
#51 filter_variables_cmi-1799440-51.patch6.7 KBdagmar
#51 interdiff.txt463 bytesdagmar
#46 filter_variables_cmi-1799440-46.patch6.7 KBdagmar
#46 interdiff.txt2.71 KBdagmar
#40 filter_variables_cmi-1799440-39.patch7.03 KBznerol
#40 interdiff-5.txt1.22 KBznerol
#37 filter_variables_cmi-1799440-36.patch6.57 KBznerol
#37 interdiff-4.txt4.8 KBznerol
#34 filter_variables_cmi-1799440-32.patch7.76 KBznerol
#34 interdiff-3.txt2.11 KBznerol
#32 filter_variables_cmi-1799440-31.patch7.53 KBznerol
#32 interdiff-2.txt2.71 KBznerol
#31 filter_variables_cmi-1799440-30.patch5.36 KBznerol
#31 interdiff.txt3.64 KBznerol
#22 filter_variables_cmi-1799440-22.patch5.42 KBdagmar
#21 filter_variables_cmi-1799440-21.patch5.84 KBdagmar
#16 filter_variables_cmi-1799440-16.patch5.53 KBdagmar
#14 filter_variables_cmi-1799440-14.patch5.38 KBdagmar
#12 filter_variables_cmi-1799440-12.patch4.39 KBdagmar
#12 interdiff.txt1.04 KBdagmar
#10 filter_variables_cmi-1799440-10.patch4.02 KBdagmar
#7 filter_variables_cmi-1799440-7.patch3.63 KBAlbert Volkman
#7 interdiff.txt2.35 KBAlbert Volkman
#5 filter_variables_cmi-1799440-5.patch3.67 KBAlbert Volkman
#3 filter_variables_cmi-1799440-3.patch4 KBAlbert Volkman
#3 interdiff.txt1.52 KBAlbert Volkman
#1 1799440.filter_variables_cmi.1.patch3.64 KBdagmar
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dagmar’s picture

Status: Active » Needs review
FileSize
3.64 KB

First patch.

Status: Needs review » Needs work

The last submitted patch, 1799440.filter_variables_cmi.1.patch, failed testing.

Albert Volkman’s picture

Status: Needs work » Needs review
FileSize
1.52 KB
4 KB

Setting default values.

Status: Needs review » Needs work

The last submitted patch, filter_variables_cmi-1799440-3.patch, failed testing.

Albert Volkman’s picture

Status: Needs work » Needs review
FileSize
3.67 KB

Silly me.

Status: Needs review » Needs work

The last submitted patch, filter_variables_cmi-1799440-5.patch, failed testing.

Albert Volkman’s picture

Status: Needs work » Needs review
FileSize
2.35 KB
3.63 KB

I believe it was failing because the data format wasn't matching up.

Status: Needs review » Needs work

The last submitted patch, filter_variables_cmi-1799440-7.patch, failed testing.

Albert Volkman’s picture

Ignore #7. It looks as though the config object is never being loaded. I'm not familiar with the yaml discovery mechanism, however.

dagmar’s picture

Status: Needs work » Needs review
FileSize
4.02 KB

The problem seems to be related that drupal_strip_dangerous_protocols() is called in the installation proccess and expects values from a non-existent configuration.

With this patch, the installation can be completed. I looked at the database and the values for filter.settings are properly saved in {cache_config}.

So the question here is, can we live with this? or Is there a better way to get the default values in drupal_strip_dangerous_protocols()?

Status: Needs review » Needs work

The last submitted patch, filter_variables_cmi-1799440-10.patch, failed testing.

dagmar’s picture

Status: Needs work » Needs review
FileSize
1.04 KB
4.39 KB

_filter_url is called by Drupal\filter\Tests\FilterUnitTest which doesn't run the upgrade of variables, so we provide the default values here if they are not available from config().

Status: Needs review » Needs work

The last submitted patch, filter_variables_cmi-1799440-12.patch, failed testing.

dagmar’s picture

Status: Needs work » Needs review
FileSize
5.38 KB

Lets try a new approach. Moved filter_allowed_protocols to the system module. We need that config before the filter module is installed.

Status: Needs review » Needs work

The last submitted patch, filter_variables_cmi-1799440-14.patch, failed testing.

dagmar’s picture

Status: Needs work » Needs review
FileSize
5.53 KB

Defining the allowed_protocols in the UnitTestBase class allows to reduce a lot of code.

Status: Needs review » Needs work

The last submitted patch, filter_variables_cmi-1799440-16.patch, failed testing.

Berdir’s picture

- Patch is currently a mix of filter.settings and system.settings.
- If it's just a single test that requires those protocolls then it should be set there, not in the global unit test base class.
- Also, if that test calls functions, then it should probably not be a unit test but extend from the new DrupalUnitTestBase, which allows to enable modules, which in turn should properly load the default settings file, I think.

dagmar’s picture

@berdir, thanks, but the issue is a bit more complicated. The main issue with the DrupalUnitTestBase is that cannot access to the database, and therefore we cannot set the proper default values for the allowed protocols.

I tried in #16 to set the default values calling config('system.protocols')->set('allowed_protocols', ...); but seems it not working.

We need neither, find way to set the default values without connect to the database or, provide the default values I did in #14, but I don't like that approach.

I tried without success to use a MemoryCache to avoid connect to the database, but seems not be working as expected.

Maybe somebody with more experience in the CMI can fix this, I don't have other ideas to fix this issue.

sun’s picture

+++ b/core/modules/filter/filter.install
@@ -143,7 +143,26 @@ function filter_install() {
+function filter_update_8000() {
+  update_variables_to_config('filter.settings', array(
...
+    'filter_allowed_protocols' => 'allowed_protocols',

Many parts of this patch seem to be outdated and still try to access filter.settings or filter.protocols instead of system.protocols.

+++ b/core/modules/filter/filter.module
@@ -1552,7 +1552,7 @@ function _filter_url($text, $filter) {
+  $protocols = array_flip(config('sysytem.protocols')->get('allowed_protocols'));

This typo is most likely the cause for the test failures.

+++ b/core/modules/simpletest/lib/Drupal/simpletest/UnitTestBase.php
--- /dev/null
+++ b/core/modules/system/config/system.protocols.yml

I wasn't really happy with the config object/file name system.protocols.

Suggestion:

Let's rename this to system.filter

+++ b/core/modules/system/config/system.protocols.yml
@@ -0,0 +1,13 @@
+allowed_protocols:

In turn, I think we can shorten this key to just "protocols"

dagmar’s picture

Status: Needs work » Needs review
FileSize
5.84 KB

@sun, thanks! lets try with this patch then.

dagmar’s picture

Sorry, this patch.

sun’s picture

+++ b/core/modules/simpletest/lib/Drupal/simpletest/UnitTestBase.php
@@ -68,6 +68,8 @@ protected function setUp() {
+    config('system.filter')->set('protocols', array('http', 'https', 'ftp', 'news', 'nntp', 'telnet', 'mailto', 'irc', 'ssh', 'sftp', 'webcal', 'rtsp'));

Oh.

I didn't notice this detail before.

In this case, we probably want and need to make an exception to our usual stance for default configuration values. In concrete terms:

drupal_strip_dangerous_protocols() should try to retrieve the config value, but if it comes back empty, use the pre-existing default; e.g.:

config('system.filter')->get('protocols') ?: array();

This applies to drupal_strip_dangerous_protocols() only.

Berdir’s picture

+++ b/core/modules/filter/filter.installundefined
@@ -143,7 +143,26 @@ function filter_install() {
+ * @ingroup config_upgrade
+ */
+function filter_update_8000() {
+  update_variables_to_config('filter.settings', array(
+    'filter_fallback_format' => 'fallback_format',
+    'filter_allowed_protocols' => 'allowed_protocols',
+  ));

This still contains the allowed protocolls definition.

+++ b/core/modules/system/config/system.filter.ymlundefined
--- /dev/null
+++ b/core/modules/system/config/system.protocols.ymlundefined

+++ b/core/modules/system/config/system.protocols.ymlundefined
+++ b/core/modules/system/config/system.protocols.ymlundefined
@@ -0,0 +1,13 @@

@@ -0,0 +1,13 @@
+allowed_protocols:
+  - http
+  - https
+  - ftp
+  - news

And this file also still seems to be around.

dagmar’s picture

Title: Convert Filter variables to Configuration System » [STOP THE TESTS FOR THIS ISSUES] Convert Filter variables to Configuration System
Priority: Normal » Critical

I'm temporary changing this to bring the attention of the right people.

The last two files have been being tested during the last 12 hours! This means two test bots are basically not available for other tests.

After 45 minutes the test finish and automatically starts again. @rfay, @jthorson please try to stop the the
execution of the tests for:
http://qa.drupal.org/pifr/test/374588
http://qa.drupal.org/pifr/test/374598

gdd’s picture

Title: [STOP THE TESTS FOR THIS ISSUES] Convert Filter variables to Configuration System » Convert Filter variables to Configuration System
Priority: Critical » Normal

Please do not misuse the critical status. Rest assured that the people who need to know do in fact know, but people are traveling for badcamp and it may be some time before things get fixed.

Status: Needs review » Needs work

The last submitted patch, filter_variables_cmi-1799440-22.patch, failed testing.

ruth_delattre’s picture

Assigned: Unassigned » ruth_delattre
ruth_delattre’s picture

Assigned: ruth_delattre » Unassigned
znerol’s picture

Assigned: Unassigned » znerol

Attempting to resolve test failures and add upgrade path test.

znerol’s picture

Status: Needs work » Needs review
FileSize
3.64 KB
5.36 KB

This patch includes the following fixes:

  • Remove array_flip around config('system.filter')->get('protocols') in _filter_url
  • Move modules/filter/lib/Drupal/filter/config to core/modules/filter/config
  • Not only set but actually save() config in UnitTestBase::setUp()
  • Increment system_update_ number in order resolve a naming conflict with an update hook which got commited meanwhile
znerol’s picture

Add upgrade test. Also had to fix UnitTestBase::setUp() such that default protocols are set there too.

Status: Needs review » Needs work

The last submitted patch, filter_variables_cmi-1799440-31.patch, failed testing.

znerol’s picture

This hopefully fixes the tests.

znerol’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, filter_variables_cmi-1799440-32.patch, failed testing.

znerol’s picture

Another try.

znerol’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, filter_variables_cmi-1799440-36.patch, failed testing.

znerol’s picture

Because FilterUnitTest is a unit-test, there is no configuration and the list of protocols is empty. Lets work around that and supply the allowed protocol within setUp.

znerol’s picture

Status: Needs work » Needs review

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

The last submitted patch, filter_variables_cmi-1799440-39.patch, failed testing.

znerol’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, filter_variables_cmi-1799440-39.patch, failed testing.

znerol’s picture

Status: Needs work » Needs review
Issue tags: +Configuration system
dagmar’s picture

Hi @znerol, you can't call config()...->save() in the Unit test because it doesn't have connection to the database.

Also, there is no point to pass the default settings in the setup if you are using a default when no value is returned by the config function.

Lets try with this.

znerol’s picture

@dagmar, actually tests pass with #40 on my local machine, so we should be fine.

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

The last submitted patch, filter_variables_cmi-1799440-46.patch, failed testing.

dagmar’s picture

Status: Needs work » Needs review

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

The last submitted patch, filter_variables_cmi-1799440-46.patch, failed testing.

dagmar’s picture

Status: Needs work » Needs review
FileSize
463 bytes
6.7 KB
znerol’s picture

Assigned: znerol » Unassigned

Unassigning myself and hope for reviews and a decision whether we go with #40 or #46.

dagmar’s picture

FileSize
3.23 KB

This is the interdiff between #40 and #51

Berdir’s picture

I think there are two completely different problems here that need to be solved differently.

+++ b/core/includes/common.incundefined
@@ -1299,7 +1299,7 @@ function drupal_strip_dangerous_protocols($uri) {
     // During test-setup system.filter.protocols may be empty. In order to
     // avoid test breakage, a list of protocols necessary to run tests is
     // supplied in that case.
-    $allowed_protocols = array_flip(config('system.filter')->get('protocols') ?: array('http', 'https'));
+    $allowed_protocols = array_flip(config('system.filter')->get('protocols') ?: array('ftp', 'http', 'https', 'irc', 'mailto', 'news', 'nntp', 'rtsp', 'sftp', 'ssh', 'tel', 'telnet', 'webcal'));

This is the first problem that we have to solve. This function is defined in common.inc and called and used before we have a chance to import the default config.

The first patch (red) solves this problem by only defining the minimally required valid protocols under the assumption that under normal operation, the default config does exist and before (especially in the initial phase of the installer, not sure if something else requires this too) we don't need more than http/https. There's no need to display an irc or ssh link at that point, for example. The other simply duplicates the default.

I personally prefer the minimal version unless this is a problem, but I don't really care.

+++ b/core/modules/filter/filter.moduleundefined
@@ -1552,7 +1552,7 @@ function _filter_url($text, $filter) {
   // we cannot cleanly differ between protocols here without hard-coding MAILTO,
   // so '//' is optional for all protocols.
   // @see filter_xss_bad_protocol()
-  $protocols = config('system.filter')->get('protocols');
+  $protocols = config('system.filter')->get('protocols') ?: array('ftp', 'http', 'https', 'irc', 'mailto', 'news', 'nntp', 'rtsp', 'sftp', 'ssh', 'tel', 'telnet', 'webcal');
   $protocols = implode(':(?://)?|', $protocols) . ':(?://)?';
   // Prepare domain name pattern.

@@ -22,13 +22,6 @@ public static function getInfo() {
     );
   }
-  public function setUp() {
-    parent::setUp();
-    // Manually set the allowed protocols. The list should be kept in sync with
-    // core/modules/system/config/system.filter.yml.
-    config('system.filter')->set('protocols', array('http', 'https', 'ftp', 'news', 'nntp', 'telnet', 'mailto', 'irc', 'ssh', 'sftp', 'webcal', 'rtsp'))->save();

This is the second problem. What happens here is that FilterUnitTest fails because it does not import the config (obviously, it's a unit test) but then calls this function. The first patch possibly only works because the caching is extremely failure-tolerant and just eats any thrown exception and the second adds yet another duplication of those default settings.

IMHO, the actual problem is quite simple: FilterUnitTest is *not* a unit test. It only works because it's built on the assumption that filter is a required module and therefore filter.module is loaded under any circumstance.

The proper fix for this problem is to change this class to extend DrupalUnitTestBase (and rename it to something that makes sense), enable the filter.module with enableModules() (which will make sure that the default config is copied and exists) and then it should just work.

znerol’s picture

@Berdir: Do you think it is too risky to go with the minimal variant and convert the test in a follow-up? If possible I'd rather keep those variable->config patches as small as possible. I fear that each of them will have to be rerolled a couple of times.

dagmar’s picture

Thanks @berdir. So, this patch replaces the unit test by a web test. I didn't renamed the test, but included a comment explaining the reason to extend WebTestCase.

Also, because we now have the system module installed, we don't need anymore the default values for the config()->get().

Berdir’s picture

That will now fail again for for the function in common.inc, no? As you reverted that too·.. As I said, two different problems that need a different solution :)

Also, I did mean to make it a DrupalUnitTest, see http://drupal.org/node/1829160, that's faster than a webtest. But if that works, then that's fine too I guess.

znerol’s picture

@Berdir: I think in the long run we should turn the low-level filter functions into one or more components and use filter.module only for the UI part, i.e. for putting together text formats. But that'll definitely be a follow-up.

dagmar’s picture

@Berdir awesome I didn't know about the DrupalUnitTest, tests are much more faster now.

Regarding

That will now fail again for for the function in common.inc, no?

I think we are ok. If config are provided by the system module, they are loaded before call drupal_strip_dangerous_protocols().

Status: Needs review » Needs work

The last submitted patch, filter_variables_cmi-1799440-59.patch, failed testing.

dagmar’s picture

Status: Needs work » Needs review
FileSize
1.76 KB
8.25 KB

Well... Seems upgrade paths really needs some default values after all.

So, in this patch I converted XssUnitTest into a DrupalUnitTestBase

And added the default values for drupal_strip_dangerous_protocols() that znerol introduced in #39.

sun’s picture

Status: Needs review » Needs work

Almost there!

Final remarks:

+++ b/core/includes/common.inc
@@ -1296,7 +1296,10 @@ function drupal_strip_dangerous_protocols($uri) {
+    // During upgrade path tests system.filter.protocols may be empty. In order
+    // to avoid test breakage, a list of protocols necessary to run tests is
+    // supplied in that case.

This should be removed and replaced with:

"
filter_xss_admin() is called by the installer and update.php, in which case the configuration may not exist (yet). Provide a minimal default set of allowed protocols for these cases.
"

+++ b/core/modules/filter/lib/Drupal/filter/Tests/FilterUnitTest.php
@@ -7,13 +7,21 @@
+  public static $modules = array('system');

@@ -22,6 +30,11 @@ public static function getInfo() {
+    $this->enableModules(array('system'));

1) In all of these tests, please remove the enableModules(), and instead add this to setUp():

config_install_default_config('module', 'system');

That imports the default config of System module only.

2) In $modules, instead of 'system', you need to specify 'filter'.

+++ b/core/modules/system/lib/Drupal/system/Tests/Upgrade/SystemUpgradePathTest.php
@@ -79,6 +79,15 @@ public function testVariableUpgrade() {
+    $expected_config['system.filter'] = array(
+      'protocols.0' => 'http',
+      'protocols.1' => 'https',
+    );

+++ b/core/modules/system/tests/upgrade/drupal-7.system.database.php
@@ -94,6 +94,10 @@
+  ))
+->values(array(
+  'name' => 'filter_allowed_protocols',
+  'value' => 'a:2:{i:0;s:4:"http";i:1;s:5:"https";}',
 ))

The injected D7 variable value should be different from the default/fallback value of drupal_strip_dangerous_protocols(); i.e., let's add some more protocols there (e.g., ftp, mailto).

Also, minor: Leading whitespace before the closing parenthesis of the previous values.

+++ b/core/modules/system/lib/Drupal/system/Tests/Upgrade/SystemUpgradePathTest.php
@@ -79,6 +79,15 @@ public function testVariableUpgrade() {
+    $expected_config['filter.settings'] = array(
+      'fallback_format' => 'pagan_poetry'
+    );

+++ b/core/modules/system/tests/upgrade/drupal-7.system.database.php
@@ -105,4 +109,8 @@
+db_update('variable')
+  ->fields(array('value' => 's:12:"pagan_poetry";'))
+  ->condition('name', 'filter_fallback_format')
+  ->execute();

I don't think it is a good idea to change the filter_fallback_format variable value to a bogus value in this upgrade path test. It is bogus, because there is no text format with the name 'pagan_poetry'. Once we will convert text formats, the bogus config value might cause problems.

I think it is OK to just test for the 'plain_text' value.

Albert Volkman’s picture

Status: Needs work » Needs review
FileSize
3.92 KB
8.38 KB

Done.

sun’s picture

Status: Needs review » Reviewed & tested by the community

Thank you! :)

This looks great and is RTBC, if the bot comes back green.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

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

Anonymous’s picture

Issue summary: View changes

Removed variable filter_default_format