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.
When running a migration from Drupal 6 to Drupal 8, the following notice ("status") arises:
Undefined index: allowed_html FilterSettings.php:45
Here is the result of a config-export:
uuid: xxxx
langcode: fr
status: true
dependencies: { }
name: 'Filtered HTML'
format: filtered_html
weight: 0
filters:
filter_html:
id: filter_html
provider: filter
status: true
weight: -10
settings:
allowed_html: ''
filter_html_help: true
filter_html_nofollow: false
Comment | File | Size | Author |
---|---|---|---|
#25 | undefined_index-2826505-25.patch | 4.1 KB | yogeshmpawar |
#25 | interdiff-2826505-22-25.txt | 651 bytes | yogeshmpawar |
#22 | interdiff.txt | 1.12 KB | quietone |
#22 | 2826505-22.patch | 4.08 KB | quietone |
#15 | 2826505-15.patch | 4.08 KB | quietone |
Comments
Comment #2
harsh.behl CreditAttribution: harsh.behl at TO THE NEW commentedMay be this could help.
Comment #3
harsh.behl CreditAttribution: harsh.behl at TO THE NEW commentedComment #4
drzraf CreditAttribution: drzraf commentedFixed.
With this patch applied, the notice is gone.
Comment #5
mikeryanCan we get a test that triggers the notice?
Patch contains tab characters - should be spaces.
Comment #6
drzraf CreditAttribution: drzraf commentedWhat I can say to help is that D6 DB contains various text formats,
* Some where allowed for no role
* Other where with none of the options (like a PHP-filter text format)
* Could be noted that I had some geshi filter stuff in the DB (the module was removed without proper uninstall)
Pretty sure that one of these triggered the notice.
Comment #7
harsh.behl CreditAttribution: harsh.behl at TO THE NEW commentedUpdated patch with no tabs.
Comment #8
quietone CreditAttribution: quietone commentedI am getting this error with a source D6 that is a fresh install of Ubercart 6, with a store and a couple of products. Made an brief attempt at a test but then, I couldn't generate the error again.
Comment #9
quietone CreditAttribution: quietone commentedTracked this to FilterFormat::prepareRow(). When the deita is 0, the $settings array is made from several variables, allowed_html_$format, filter_html_help_$format, and filter_html_nofollow_$format. In the failure case, when $format=1, all those variables do not exist. Then, in the process plugin, trying to write to any index on $settings fails, causing the error. I don't know how those variable got deleted when building an Ubercart test site.
So, making a test would require deleting those variables from the test database before running the test. That seems excessive for a change that we could argue should be made in of itself, that is the process should check that it is safe to write to the array before doing so.
Comment #10
quietone CreditAttribution: quietone commentedMarking NR, to get feedback on the need for a test.
Comment #11
mikeryanComment #12
mglamanJust caught this, too. Running
commerce_migrate
tests caused a failure due to this notice.Comment #13
mikeryanSanity-checking inputs is something that should be done in and of itself, yes, and if we were purely sanity-checking for the sake of it I might agree we don't need tests. But, given that this is something multiple people have seen in real life, I do think we should test for it.
Comment #14
mikeryanComment #15
quietone CreditAttribution: quietone commentedOK, guess I'm just feeling lazy.
Here is a test and the fix.
Comment #17
mikeryanLooks good, thanks!
Comment #18
drzraf CreditAttribution: drzraf commentedMay I suggest to improve error reporting or at least document here what's going on?
The root causes of the DB inconsistency are unknown.
But isn't it for that lack of detail? Does the message "
Undefined index: allowed_html FilterSettings.php:45
" make it clear enough?IE:
Is the named filter causing the issue by itself?
Is it only one of its configuration? (which one then?)
Also, now that the patch somehow ignore the filter (totally?) during the migration, will a warning/message be stored somewhere?
I think that could be of value if some of these questions could be answered in this thread.
thx
Comment #19
mglamanFor what it's worth, I realize that I am the cause of this bug. I tried to trim up the original
commerce_migrate
test fixtures to piggy back off of core's, and quietone had re-rolled them for me. So I did create a one of a kind possible scenario.I'm all for throwing exceptions and failing early. Perhaps we should throw an exception about an unexpected change from what a default Drupal 6 database would contain? Or, work around it as proposed.
Comment #20
quietone CreditAttribution: quietone commentedNot so sure about that. I just did a fresh install of D6. And in the variable table there is no entry for allowed_html. So, the process plugin really should check that the array key exists before trying to write to it.
Comment #21
Gábor HojtsyLooks good other than minor code style things, but especially a comment needs updating:
Hm, not image field tests, right?
Minor: Period at end of line/sentence missing.
Comment #22
quietone CreditAttribution: quietone as a volunteer commentedBoth fixed.
Comment #23
phenaproximaIt's filter_settings, not filter_id. :) Otherwise, this looks fine to me. RTBC once this is fixed.
Comment #24
yogeshmpawarComment #25
yogeshmpawarChanges made as per comment #23 & also added interdiff.
Comment #26
mikeryanYep, think this is good to go.
Comment #29
Gábor HojtsyLooks good now, thanks! Great to see the added test coverage, woot :)