Problem/Motivation
Any field of type 'Text (formatted)' simply does not save the data entered into it when a module is enabled that alters the element info for #type text_format and adds an additional #pre_render callback.
Possible data loss: It could simply cause data loss as the user will simply fail to notice the entered data is not saved.
Impossible to submit data: In case the field is set as 'required', the user will not be able to submit the data at all.
How to reproduce?
- Install Drupal 8 with the standard profile.
- Create a field of type 'Text (formatted)' with default configuration.
- Try to save a value for that field. e.g. when you create a content.
Note: This is about a "text" field (as in Field API) not a "textfield" (as in @FormElement("textfield")).
Proposed resolution
Technical reason:
The filter module copies selected element info properties from the top-level #type text_format element to the lower level #type textfield element, and then augments the lower level #type textfield element with its default values using the + $array operator. The + $array operator will not override existing keys.
When text module is enabled on its own, there is no value for #type textfield in #pre_render and so the lower level #type textfield receives the default $array value from the textfield element info.
When editor module is enabled, it alters the element info for #type text_format, adding a #pre_render entry, this is copied into the lower level #type textfield and prevents the default from being added via the default when + $array is used.
The solution in the patch is to make sure that #pre_render values form #type text_format are not copied into the lower level #type textfield element, ensuring the defaults are set.
Before
After
Remaining tasks
Tests are needed.There is a test but it passes while it should fail. Refer to #2382601: Tests which pass while they should fail!.- Follow up issue #2350931: Textarea form element should use #pre_render rather than preprocess function
- Related issue #2349589: JavaScript error on field type Text (formatted)
User interface changes
None.
API changes
Unknown.
Original report by @Kartagis
I noticed that fields created via Field API or otherwise (only on registration form) don't save information. I first noticed this when I created a field via my module and attached it to user entity_type. I then tested it by manually creating a formatted text field with Field UI, and noticed that behaviour too.
Regards,
| Comment | File | Size | Author |
|---|---|---|---|
| #41 |  text-format-busted-2348459.pass_.patch | 4.38 KB | larowlan |
| PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,769 pass(es). [ View ] | |||
| #41 | text-format-busted-2348459.fail_.patch | 3.79 KB | larowlan |
| FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,724 pass(es), 1 fail(s), and 0 exception(s). [ View ] | |||
| #41 |  interdiff.txt | 4.3 KB | larowlan |
| #40 | text-format-busted-2348459.fail3_.patch | 6.81 KB | larowlan |
| FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,744 pass(es), 1 fail(s), and 0 exception(s). [ View ] | |||
| #40 | interdiff.txt | 1.14 KB | larowlan |
Comments
Comment #1
Kartagis commentedComment #2
.John commentedUnable to recreate this issue in the latest dev when adding a field through the UI. Can you provide steps on how to create the issue from a fresh installation of the latest dev?
Comment #3
Kartagis commentedStep 1: Browsed to /admin/config/people/accounts/fields
Step 2: Created a field.
Step 3: Verified that the table for the field has been created.
Step 4: Filled in some value and save.
Step 5: Checked if there is any data in the table.
There is one additional behaviour that I noticed. If you make the field required and fill in some value, I get
Foo field is required..Regards,
K.
Comment #4
lokapujya commentedTried those steps. It works fine. Anything different about your setup?
Comment #5
lokapujya commentedseeing it now that I used formatted text field.
Comment #6
Kartagis commentedYou are right, it works on plain text.
Comment #7
alexarpen commentedComment #8
alexarpen commentedThe simple text field did not used to have the option to be filtered in Drupal 7 and I couldn't find any evidence it does in Drupal 8 on purpose.
This feature is simply implemented in class TextItemBase and is not overridden in class TextItem.
The question is that 'Should simple text field be able to be a filtered text at all?'
Some reference to check:
https://api.drupal.org/api/drupal/developer!topics!forms_api_reference.h...
Comment #9
alexarpen commentedThe problem is that the input field has no name attribute therefore the data is not sent to the server upon form submission.
Comment #10
alexarpen commentedComment #11
Berdir commentedSo we're starting to figure this out.
The current preprocess logic for textfields sits in \Drupal\Core\Render\Element\Textfield::preRenderTextfield(). But somehow, this is not called when we go through a text_format type that then switches to a textfield. Textarea doesn't have the prerender method, it uses preprocess for this. Why is this different?
Comment #12
alexarpen commentedThe problem was caused by filter module. It was making the text field not be able to run #pre_render functions.
Though there is no problem with textarea. The reason is that textarea is using template_preprocess_* function (the old way) rather than using #pre_render methods.
There is a need for another issue saying that textarea should use #pre_render rather than *_preprocess_* function.
There is also another problem would be noticeable after applying this path:
Text fields with formatted text enabled turn into Textarea in front end. This should be discussed in another issue.
There is also a big question remaining: Is does not seem to be a mistake by filter.module because it is simply mobing #pre_render to the child element. Why it's not / should it be applied for the child element?
Comment #13
alexarpen commentedA new issue is created to make Textarea use #pre_render rather than proprocess function:
https://www.drupal.org/node/2350931
Comment #14
graindor commentedComment #15
Wim Leers commentedThis issue is very confusing. Please update the IS to be incredibly explicit about a "text" field (as in Field API) versus a "textfield" (as in
@FormElement("textfield")).This may have been introduced during the conversion of elements into annotated classes.
Comment #16
Wim Leers commentedBerdir asked:
Yes, CKEditor will happily replace
input[type=text]also AFAIK/IIRC.The real question/problem is: why does Drupal allow associating a text format with a
input[type=text]? (Or, probably, why does Drupal allow using ainput[type=text]Field API Widget with a filtered text field?)Comment #17
Berdir commentedtext fields with a format were always (as in, since 7.x at least) supported.
This is not field API specific. Field API text fields use the standard form API #type => text_format with #base_type => 'textfield', that is broken.
And yes, I guess it is related to the @RenderElement changes.
Comment #18
beejeebus commentedouch, this will lead to data loss.
we definitely need tests so that we don't reintroduce bugs that lose user-submitted data.
Comment #19
kattekrab commentedI'll have a go at updating the issue summary.
Comment #20
kattekrab commentedComment #21
kattekrab commentedtitle change for clarity
Comment #22
kattekrab commentedComment #23
alexarpen commented@beejeebus, the problem is a bit more dramatic than not having a test for that. Actually there is a test for that but for an unknown reason, the test passes while it should fail.
@kattekrab, thanks. It is much more clear now!
I am re-submitting the patch as the dev version has changed a bit since then.
Comment #24
alexarpen commentedText field uses '#pre_render' functions to add attributes. In D7 this was being done by template_preprocess_* functions.
The problem is that filter module removes '#pre_render' from $element['value'] which prevents the text field to do the preprocess tasks.
Please review it and move it to RTBC.
I update the issue summary to explain how to reproduce the problem in simple steps.
Comment #25
lokapujya commented" the test passes while it should fail." - Which test is that? How hard would it be to modify the test so that i fails without the patch?
Comment #26
alexarpen commentedThe problem with the test is actually another issue. I will create another issue for that and will explain things there. This patch has nothing to do with the test. It just fixed the bug.
Comment #27
lokapujya commentedAnytime we fix a bug (even more so, for a data loss), we should also add a test or fix the broken test.
Since this bug is only visible when another module (Filter module) is enabled, one way of testing this would be to add a test that enables all Core modules and then runs a form test. Is there a better way? Another way, would be to mimic the way filter module removes '#pre_render'.
Comment #28
alexarpen commented@lokapujya Could you explain more about this line: 'Another way, would be to mimic the way filter module removes '#pre_render'.'?
Comment #29
lokapujya commentedWe could create a dummy test module that does (whatever it is that filter module does) that is preventing the text field to do preprocess tasks AND then enable it during a test. Maybe it's easier just to enable the filter module?
Comment #30
kattekrab commented@alexar - I think that what @beejeebus and @lokapujya are saying is that YOU need to write and add tests to show your patch fixes the bug. And the tests help prevent regressions in future.
Comment #31
larowlan commentedSo here's a test, but the thing is - it passes - without the patch.
Can someone take a look and tell me what I'm missing.
Comment #32
larowlan commentedSo if the test passes - does that point to something in Field API?
Comment #33
tim.plunkett commented+++ b/core/modules/filter/src/Tests/TextFormatElementFormTest.php@@ -0,0 +1,136 @@
+ $form_state = new FormState();
+ $form_state->setBuildInfo(['callback_object' => $this, 'args' => []]);
...
+ $form = $form_builder->buildForm($this, $form_state);
This could just be
$form_builder->getForm($this);Comment #34
alexarpen commented@larowlan and @tim.plunkett
I created another issue regarding the test and put the current test which has a problem there:
#2382601: Tests which pass while they should fail!
I also updated the issue summary.
Comment #35
catch commentedI think we should handle #2382601: Tests which pass while they should fail! in here.
Bumping this to critical now anyway.
Comment #36
alexarpen commented@catch I think they are different issues and should be discussed differently because even though we fix the test issue this problem still persists and needs the patch get applied.
Comment #37
larowlan commentedClosed #2382601: Tests which pass while they should fail! as duplicate
Comment #38
larowlan commentedClosed #2382601: Tests which pass while they should fail! as duplicate
Comment #39
larowlan commentedHere's another test that follows the exact steps to reproduce - but passes :(
*BUT* the exact steps to reproduce applied manually actually reproduce the bug
*SO* I assume it is something in standard install or another enabled module
digging down that path now
Comment #40
larowlan commentedSo yep, its something in standard - looking at editor module first (already looked at quickedit)
Patch to demo fail
Comment #41
larowlan commentedOk, getting somewhere - the cause is indeed editor module.
Fail/Pass patches.
Also fixes #33
Fix is #24
Blew away the web-test added in #39, the kernel test is obviously quicker.
Comment #42
larowlan commentedUpdated issue summary with tech details
Comment #45
beejeebus commentedlarowlan++
Comment #46
tim.plunkett commentedNice sleuthing!
Comment #47
alexpott commentedCommitted 2b60900 and pushed to 8.0.x. Thanks!
Comment #49
Wim Leers commentedWow! Awesome detective work!
Also: thanks for the follow-up; this is indeed brittleness we should avoid.