Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
The following variables have to be converted to the new configuration system.
- filter_allowed_protocols
- filter_fallback_format
Comment | File | Size | Author |
---|---|---|---|
#63 | filter_variables_cmi-1799440-63.patch | 8.38 KB | Albert Volkman |
#63 | interdiff.txt | 3.92 KB | Albert Volkman |
#61 | filter_variables_cmi-1799440-61.patch | 8.25 KB | dagmar |
#61 | interdiff.txt | 1.76 KB | dagmar |
#59 | filter_variables_cmi-1799440-59.patch | 7 KB | dagmar |
Comments
Comment #1
dagmarFirst patch.
Comment #3
Albert Volkman CreditAttribution: Albert Volkman commentedSetting default values.
Comment #5
Albert Volkman CreditAttribution: Albert Volkman commentedSilly me.
Comment #7
Albert Volkman CreditAttribution: Albert Volkman commentedI believe it was failing because the data format wasn't matching up.
Comment #9
Albert Volkman CreditAttribution: Albert Volkman commentedIgnore #7. It looks as though the config object is never being loaded. I'm not familiar with the yaml discovery mechanism, however.
Comment #10
dagmarThe 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()?
Comment #12
dagmar_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().
Comment #14
dagmarLets try a new approach. Moved filter_allowed_protocols to the system module. We need that config before the filter module is installed.
Comment #16
dagmarDefining the allowed_protocols in the UnitTestBase class allows to reduce a lot of code.
Comment #18
Berdir- 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.
Comment #19
dagmar@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.
Comment #20
sunMany parts of this patch seem to be outdated and still try to access filter.settings or filter.protocols instead of system.protocols.
This typo is most likely the cause for the test failures.
I wasn't really happy with the config object/file name system.protocols.
Suggestion:
Let's rename this to
system.filter
In turn, I think we can shorten this key to just "protocols"
Comment #21
dagmar@sun, thanks! lets try with this patch then.
Comment #22
dagmarSorry, this patch.
Comment #23
sunOh.
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.:
This applies to drupal_strip_dangerous_protocols() only.
Comment #24
BerdirThis still contains the allowed protocolls definition.
And this file also still seems to be around.
Comment #25
dagmarI'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
Comment #26
gddPlease 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.
Comment #28
ruth_delattre CreditAttribution: ruth_delattre commentedComment #29
ruth_delattre CreditAttribution: ruth_delattre commentedComment #30
znerol CreditAttribution: znerol commentedAttempting to resolve test failures and add upgrade path test.
Comment #31
znerol CreditAttribution: znerol commentedThis patch includes the following fixes:
array_flip
aroundconfig('system.filter')->get('protocols')
in_filter_url
modules/filter/lib/Drupal/filter/config
to core/modules/filter/configset
but actuallysave()
config inUnitTestBase::setUp()
system_update_
number in order resolve a naming conflict with an update hook which got commited meanwhileComment #32
znerol CreditAttribution: znerol commentedAdd upgrade test. Also had to fix
UnitTestBase::setUp()
such that default protocols are set there too.Comment #34
znerol CreditAttribution: znerol commentedThis hopefully fixes the tests.
Comment #35
znerol CreditAttribution: znerol commentedComment #37
znerol CreditAttribution: znerol commentedAnother try.
Comment #38
znerol CreditAttribution: znerol commentedComment #40
znerol CreditAttribution: znerol commentedBecause
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 withinsetUp
.Comment #41
znerol CreditAttribution: znerol commentedComment #43
znerol CreditAttribution: znerol commented#40: filter_variables_cmi-1799440-39.patch queued for re-testing.
Comment #45
znerol CreditAttribution: znerol commented#40: filter_variables_cmi-1799440-39.patch queued for re-testing.
Comment #46
dagmarHi @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.
Comment #47
znerol CreditAttribution: znerol commented@dagmar, actually tests pass with #40 on my local machine, so we should be fine.
Comment #49
dagmar#46: filter_variables_cmi-1799440-46.patch queued for re-testing.
Comment #51
dagmarComment #52
znerol CreditAttribution: znerol commentedUnassigning myself and hope for reviews and a decision whether we go with #40 or #46.
Comment #53
dagmarThis is the interdiff between #40 and #51
Comment #54
BerdirI think there are two completely different problems here that need to be solved differently.
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.
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.
Comment #55
znerol CreditAttribution: znerol commented@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.
Comment #56
dagmarThanks @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().
Comment #57
BerdirThat 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.
Comment #58
znerol CreditAttribution: znerol commented@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.
Comment #59
dagmar@Berdir awesome I didn't know about the DrupalUnitTest, tests are much more faster now.
Regarding
I think we are ok. If config are provided by the system module, they are loaded before call drupal_strip_dangerous_protocols().
Comment #61
dagmarWell... 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.
Comment #62
sunAlmost there!
Final remarks:
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.
"
1) In all of these tests, please remove the enableModules(), and instead add this to setUp():
That imports the default config of System module only.
2) In $modules, instead of 'system', you need to specify 'filter'.
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.
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.
Comment #63
Albert Volkman CreditAttribution: Albert Volkman commentedDone.
Comment #64
sunThank you! :)
This looks great and is RTBC, if the bot comes back green.
Comment #65
catchCommitted/pushed to 8.x, thanks!
Comment #66.0
(not verified) CreditAttribution: commentedRemoved variable filter_default_format