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 |
#41 | text-format-busted-2348459.fail_.patch | 3.79 KB | larowlan |
#41 | interdiff.txt | 4.3 KB | larowlan |
#40 | text-format-busted-2348459.fail3_.patch | 6.81 KB | larowlan |
#40 | interdiff.txt | 1.14 KB | larowlan |
Comments
Comment #1
KartagisComment #2
JKingsnorth CreditAttribution: JKingsnorth 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
KartagisStep 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
lokapujyaTried those steps. It works fine. Anything different about your setup?
Comment #5
lokapujyaseeing it now that I used formatted text field.
Comment #6
KartagisYou are right, it works on plain text.
Comment #7
pendashteh CreditAttribution: pendashteh commentedComment #8
pendashteh CreditAttribution: pendashteh 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
pendashteh CreditAttribution: pendashteh 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
pendashteh CreditAttribution: pendashteh commentedComment #11
BerdirSo 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
pendashteh CreditAttribution: pendashteh 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
pendashteh CreditAttribution: pendashteh commentedA new issue is created to make Textarea use #pre_render rather than proprocess function:
https://www.drupal.org/node/2350931
Comment #14
graindor CreditAttribution: graindor commentedComment #15
Wim LeersThis 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 LeersBerdir 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
Berdirtext 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
Anonymous (not verified) CreditAttribution: Anonymous 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 CreditAttribution: kattekrab commentedI'll have a go at updating the issue summary.
Comment #20
kattekrab CreditAttribution: kattekrab commentedComment #21
kattekrab CreditAttribution: kattekrab commentedtitle change for clarity
Comment #22
kattekrab CreditAttribution: kattekrab commentedComment #23
pendashteh CreditAttribution: pendashteh 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
pendashteh CreditAttribution: pendashteh 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" 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
pendashteh CreditAttribution: pendashteh 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
lokapujyaAnytime 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
pendashteh CreditAttribution: pendashteh commented@lokapujya Could you explain more about this line: 'Another way, would be to mimic the way filter module removes '#pre_render'.'?
Comment #29
lokapujyaWe 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 CreditAttribution: 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
larowlanSo 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
larowlanSo if the test passes - does that point to something in Field API?
Comment #33
tim.plunkettThis could just be
$form_builder->getForm($this);
Comment #34
pendashteh CreditAttribution: pendashteh 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
catchI think we should handle #2382601: Tests which pass while they should fail! in here.
Bumping this to critical now anyway.
Comment #36
pendashteh CreditAttribution: pendashteh 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
larowlanClosed #2382601: Tests which pass while they should fail! as duplicate
Comment #38
larowlanClosed #2382601: Tests which pass while they should fail! as duplicate
Comment #39
larowlanHere'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
larowlanSo yep, its something in standard - looking at editor module first (already looked at quickedit)
Patch to demo fail
Comment #41
larowlanOk, 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
larowlanUpdated issue summary with tech details
Comment #45
Anonymous (not verified) CreditAttribution: Anonymous commentedlarowlan++
Comment #46
tim.plunkettNice sleuthing!
Comment #47
alexpottCommitted 2b60900 and pushed to 8.0.x. Thanks!
Comment #49
Wim LeersWow! Awesome detective work!
Also: thanks for the follow-up; this is indeed brittleness we should avoid.
Comment #51
gmoore777 CreditAttribution: gmoore777 commentedThis problem still exits with Drupal 8.0.0-beta6.
As the administrator, if I add a new field to my Account settings of type "Text (formatted)",
and then try to edit my account settings, and click "Save", I will get:
"field_name field is required. "
I will add one interesting note, if I try to edit my account settings, again,
and change the edit box of this new field from "Basic HTML" to "Restricted HTML",
I am able to Save out of that form, and View the text I just added.
The second interesting note, is, if I try to edit my account settings, again,
and change the edit box from "Restricted HTML" back to "Basic HTML", I am
able to Save out of that form, but I am unable to View the new text I just added.
I still see the old text that I had added above when the edit box was set to
"Restricted HTML". In other words, it didn't save the new text, but it at least
allowed me to Save out of that form without the error of "field_name field is required. "
Note to self:
I tested, not extensively, adding new fields of:
Text (plain)
Text (plain, long)
Text (formatted, long)
Text (formatted, long, with summary)
Boolean
Comments
Date
Email
Number (decimal)
and those seemed to be fine.
(FYI: I do have a few 3rd party modules added. This is not a barebones Drupal 8 beta 6 installation.)
Comment #52
gmoore777 CreditAttribution: gmoore777 commentedJust re-reproduced using a barebones Drupal 8 beta 6 installation.
Comment #53
tim.plunkettPlease don't test using betas, use the latest code from git.
Comment #54
xjmIt sounds like this is a slightly different issue in #51 and #52 than in the original report; could you file a new issue and link it here?
Comment #55
alexpottI can not reproduce this. This would occur if I set the field to required - which is exactly what should happen.
Filed #2442255: Changing text formats on a field makes it impossible to edit.
Comment #56
gmoore777 CreditAttribution: gmoore777 commentedI just installed the 8.0.0-beta7+19-dev (2015-Feb-26) version, and I can reproduce the original bug.
Yes, I do make the newly created field in the User account to be "required".
I do add text into my newly created field edit box which is set to "Basic HTML", and when
I click "Save", I get "... field is required".
Nothing is saved. The edit box, has been cleared out.
If I look at the MySQL database in the table of: user__field_doggy, there are zero rows in that table.
(I called my new field "DOGGY")
I only mentioned about changing the "Text format" to "Restricted HTML on making the saving of the
changes successful, only as a means to help the person figure out what is wrong,
and to show that a person may not be able to reproduce the bug, if they every made this "Text format" change
once in the lifetime of that drupal installation.
Cause once you get the form to save, it will save for subsequent times, even when the
"Text format" is set back to "Basic HTML'.
So this bug should be re-opened, but I will let someone else do that, at this time.
Comment #57
paulmckibbenI'm confirming that this issue still exists in Drupal 8.0.0 beta 7. If I add a field of type "Text (formatted)" to a content type, then attempt to create a node of that type, the field data is not saved.
Comment #58
catchPlease retest with the latest dev, it's nearly a month since the last beta release.
Comment #59
paulmckibben@catch, thanks, and sorry about that. I just tested against the latest from git head and the problem does not occur there. However, there is a behavior difference: there is no WYSIWYG editor on formatted text fields. Is that intentional? (FWIW, the behavior is consistent with formatted text fields in D7)