Default text formats are not saved properly without accompanying values

Setting as needs review for the bot

Here is a version to use in a make file

This needs to be fixed in d8 first

Here's a patch against D8, ready for review. And a new D7 version for after that one gets in :)

hm, lets try those again.

Comment from @hefox:

the patch is changing the logic to say having a format other than blank format means the field is not empty even if there's nada in it, which makes me suspect might cause issues assuming hook_is_empty is used elsewhere.

I added that in the case of content, we don't want the content saved if there's only a text-format (but no content) provided, but in the case of *default values* we do. It seems we should be saving differently for default values than for content.

+ if ((!isset($item['value']) || $item['value']) === '' && (!isset($item['format']) || $item['format'] === '')) {

Should this have been:

+ if ((!isset($item['value']) || $item['value'] === '') && (!isset($item['format']) || $item['format'] === '')) {

Just asking for my own clarity :)

@scottalan yes, good catch with the parenthesis!

Okay, here's a new approach that just avoids the empty check if the field is being used to set a default value (i.e. where $entity is empty)

#13: allow_default_text_format_per_field-1278886-13.patch queued for re-testing.

The patch in 13 worked for me.

I have attached a patch which cleans up some white space and should also include a small fix to the test that was failing which should make it pass. I've never modified a test before so hopefully this modification is correct.

Fixing tags per http://drupal.org/node/1517250

#12 d7 patch works for latest drupal 7.18

Hi there,

I'm not sure if this change is really necessary. Do we really need to save the input format of the default value if the default value is empty?

If the default value is empty, means that the user will have to write the content of the field and then, the input format selected by default should be on "Text Processing".

I'm not sure if I'm not understanding correctly the issue or the issue itself doesn't make any sense.

By the way the solution for tests failing (#17) is not changing the test, but fixing the issue that is causing the test to fail. A test should only be changed if the requirement has changed too.

I think this works as designed and the issue can be closed.

Regards,
David.

Reading the issue summary, I tend to agree with #20.

The only possible way this could result in an actual problem is if you'd have a filter enabled for a particular format, which inserts (arbitrary) content. Only in that case, an empty text value would be a valid and an actual value, since it is dynamically enhanced upon view. Thus, if the format does not get saved, the automatic enhancement does not happen, because the selected format was not stored.

However, I'm not aware of any filter implementation that turns "" (nothing) into something concrete (and it sounds a bit silly).

Therefore, I'd agree with won't fix.

That is, unless there is a more concrete issue summary that clarifies what exactly breaks without a fix for this and why this is needed.

I encountered this issue when I inherited a site with multiple text formats (Filtered HTML, Full HTML, and WYSIWYG HTML) all of which had slightly different validation rules and one of which used CKEditor.

The site was primarily used by non tech savvy users who would constantly complain that they couldn't figure out how to change the font or insert links into articles they were writing. The reason being the "Filtered HTML" format would always be selected when they clicked Add content. I believe this is because it is by default weighted higher on the Content authoring / Text formats page. "Text Processing" only allows the user to choose between "Plain text" and "Filtered text" it does not allow a default "Text format" to be specified.

The only way to have anything other than highest weighted format appear when a user clicks Add content is to enter text into the Default text box. I just entered, "Write your article here" in the Default value and then changed to the WYSIWYG text format.

While this is not a technical problem it is definitely a usability problem. If a user wants to specify a default Text format (other than the highest weighted format) they need to be able to do so. Right now the only apparent option way to do so is by choosing a Text format in the Default Value field. Unfortunately this doesn't work.

There is a consensus that the current functionality is the intended functionality, this it is not a bug.

I don't believe that two or three people is consensus, especially when a big focus of D8 is usability. If I change anything inside of a frame and then press save on the frame, what I changed should be saved.

When I encountered this, I searched the Issues for what I thought was a bug and found there was already a bug report created for it. If anything you could say there is just as much consensus that this is a bug. Although we have confirmed that Drupal is indeed doing what it was designed to do, I believe this is a bug on the grounds of usability. That is why I have tagged this with Needs usability review.

Until someone from the usability team looks into this please leave it in its original category of Bug Report. I do agree with sun that a fix should be postponed while evidence is gathered.

Thanks,

Kevin

@Kevin Morse: Can you give a link for the other bug report on this?

re: #22:

Thanks for clarifying. The point where I lost you is "default text format".

Filter module provides a pretty advanced concept for default text formats already. It automatically figures out which formats are available and intersects that with the formats that the current user is able to access. Out of that limited set of formats, the first one (weighing lowest) is the current user's default format.

That part seems to be missing in your equation, as your use-case seems to be to create a new, empty node, with a pre-configured text format? And you're making the bold assumption that the "other" user who's supposed to edit that node (also) has access to the text format you selected?

If the current user does not have access to the selected text format, then all the user sees is a disabled textarea that informs him/her that its content cannot be edited due to access constraints.

@see #1109010: Did not save custom text format configuration in body fields and #1015510: Specify a default format for text fields (not long text)

I also find it should be considered a serious usability issue - steps for my case :

1. Create several input formats (e.g. 'full_html', and 'simple_html'), both accessible to the user / role who's testing (or admin)
NB: I used 'full_html' as default input format for body fields

2. Create a custom field, long text, with the text format 'simple_html'

3. When you edit the node with this custom field, the default selected input format is 'full_html' (and thus, the appearance of the Wysiwyg which can have a different button setup per input format, is the wrong one)

Current workaround (which I'm gonna test now that I know it's a core issue) : @see http://drupal.org/node/1015510#comment-5099898

function MODULENAME_form_CONTENTTYPE_node_form_alter(&$form, &$form_state) {
  $form['field_example']['und'][0]['#format'] = "filtered_html";

  return $form;
}

The use-case that you're describing rather sounds like #784672: Allow text field to enforce a specific text format

Tagging

If you're using features, there is a slightly better workaround:

1) Enter a default value in the field where you want to set the filter
2) Set the filter type to what you want
3) Save
4) Update your feature with this current state
5) Edit the feature and replace the default value with an empty string

Now your feature will provide the UX that you want.

1. Install the Better Features module.
2. Go here (admin/config/content/formats/settings) and check the "Use field default" checkbox
3. Go back to your field and save the default text format again
4. Profit.

1. should read Install the Better Formats module.

unfortunately it is marked as unstable for D7
falling back to: #27
... 3 years old bug ... it was such an error to deploy a site on drupal :(

Removing the novice tag.

Why: because issue is postponed, contentious and/or lacking consensus.

Novice tag documentation: https://www.drupal.org/core-mentoring/novice-tasks

I'm not sure what more info can be provided here, so setting back to Active.

I consider myself to be a quite advanced Drupal user. Yet I was baffled when I saved field instance settings, setting a "default" text format, then to edit it again and see that this setting was not saved.

In my view it's quite a simple problem:
There is a form, which I edit, so I expect that when I reload the form, everyting I entered is still the same.
But it's not, which confuses the user, hence it's a usability issue.

I think the explantions in #22 and #24 are spot-on.

Might be best to so something like this then. I wanted one format for the initial node and another format for all commenting:

function MODULE_form_alter(&$form, &$form_state, $form_id) {
  switch ($form_id) {
    case 'custom_page_node_form':
      $form['body'][LANGUAGE_NONE][0]['#format'] = "format_one";
      break;

    case 'comment_node_custom_page_form':
      $form['comment_body'][LANGUAGE_NONE][0]['#format'] = "format_two";
      break;

    default:
  }
}

I'm still using this patch on all my sites. In my view this is definitely a UX problem for the site-builder, and therefore a core bug. The solution also offers a UX improvement to the user entering content. win-win!

@Sun's comment in #26 is a valid one. Drupal does have a complicated process for figuring out what the "default" text format should be based on the current user and their level of access.

However, the user + access calculation will result in the same "default" format everywhere on a Drupal site, and in reality, not all filtered text fields are equal. You may have one field where you want users to have a WYSIWYG (for example, body) and others where you specifically do not want users to have a wysiwyg. Drupal's calculations do not take this into effect.

Though it's possible to demand that everyone who encounters that problem should provide either a custom module with a `form_alter` in it, or require that they use a complicated set of contrib modules, the solution in this patch is much simpler.

@Sun, what's the harm in setting a default format? A user who does not have access to the default won't get it, they'll get the behavior we see now. But a user who does have access to that format gets a UX improvement. I'm not sure I understand the hesitation here.

changing status.

Also, I updated the Drupal 7 patch to match fixes in #17. Attaching them both again here.

I applied the patch in #39 to my D7 site. The default is applied when adding a node but not when editing a node. The text format defaults to the first listed.

@akosipax once a node is saved it will have a format associated with the text. Defaults will only show before the node is saved, otherwise it will pull whatever was in the database for each, and default to that.

Right. Just to be clear, let's say I have the following text_formats in the following order:

filtered_html
full_html

1. I configured body's default to be full_html.
2. I create a node. body's default format is full_html as expected.
3. I save the node without touching the body field; That is, body is empty and has the default full_html.
4. I edit the node that I just created; body is empty and has filtered_html as the selected text format.

In the last step, I was expecting the format to be full_html. Can you reproduce this as well? If so, is this intentional behaviour?

Also, I'm having the following behaviour with the patch. I don't think this is intentional since I imagine it is a generally unwanted behaviour. You can try the following steps with a core field type like "Text"

1) Access settings of a field (either a new field or an existing field; /admin/structure/types/manage/[node_type]/fields/[field_name])
2) Save the settings of the field without entering default value for that field.
3) Create a new node [that has that field whose settings we just saved]
4) Save the node without entering anything in that field.

Result: The "empty" data of that field is saved. field_data_[field_name] has a row for that empty / NULL field.
Expected: That field should not be considered empty and should not be saved. field_data_[field_name] should not have any row for that empty / NULL field.

Additional Notes:
The empty data is cleared if you edit the node and save it again -- without entering anything in the field.
I believe this has to do with the changes in the patch:

 function field_default_submit($entity_type, $entity, $field, $instance, $langcode, &$items, $form, &$form_state) {
-  // Filter out empty values.
-  $items = _field_filter_items($field, $items);
+  // Empty values are okay for setting default values so we want to avoid
+  // calling _field_filter_items(). Example: set a default text-format.
+  if (!empty($entity)){
+    // Filter out empty values.
+    $items = _field_filter_items($field, $items);
+  }
   // Reorder items to account for drag-n-drop reordering.
   $items = _field_sort_items($field, $items);

If I undo that change, it behaves normally again.

Any update on this for 8.1.x? I was going to try making a new patch but I can't even find this file or function in 8.1.1

I'm wondering if things could change here after #784672: Allow text field to enforce a specific text format gets in. If so, we should postpone this one until after that one's done.

OK, hoping to breath some new life into this 5+ year old issue...

Attached is a patch that fixes the problem that just changing the default text format for a formatted text field does not get saved. Needs tests which I hope to get to soon.

No interdiff as code seems to have changed substantially since #39.

If we change the functionality of TextItemBase::isEmpty, we'll need a CR. Added an issue summary.

Here's a quick workaround for setting Full HTML as default in Drupal 8:

1. Select "Full HTML" in the text format box.
2. Click the "source" button in the editor.
3. Enter " " (without the quotes) in the text box and DO NOT CLICK THE SOURCE BUTTON AGAIN OR THE EDITOR WILL REMOVE IT.
4. Save your changes to the field.

Note that the editor strips the   tag every time, so you'll have to do this every time you make changes to the field. On the other hand, the   tag won't show up when the user creates new content because the editor trims it automatically when the page is loaded, so that's good at least.

1278886-51-default-text-formats.patch works from my point of view… tested it and saves default formatter as expected :)
thx!

FYI: Patch #51 worked for me, too (although in Drupal 8.2.3). Thanks, @mikeker!

Hi!

I had this problem in one of my recent projects so I want to help to finally fix it.

I fixed all the migrate tests, now that isEmpty takes into account the filter the migrate tests need the "filter" module so it can use the filter plugins when it migrates the data.

There is still 1 test failing I will work on that and in the tests this week)

Thanks for all your work so far.

After to check the last failing test I think we are using the wrong approach.

Having this:

 return ($value === NULL || $value === '') && ($format === NULL || $format === '');

I think this will make to isEmpty()return false the 99% of the times because the fields by default have a format, that is why all the migrate tests now required the filter module because now it had to deal with the filter because isEmpty() is returning false...

I will start fresh and work on a different approach.

IMHO it would be better to break the return line down into a few lines instead of one line with that many comparison operators. Could help debugging later.

Jen Lamptons' D7 patch has been rerolled here:
#2852019-2: D7 Default text formats are not saved properly without accompanying values

Ideally somehow we would need to compare the previous textformat with the new one if the field is empty and the textformat is the same the isEmpty() should return true... but no idea how access the previous value from there. :-/

The workaround on #54 does not work if the field doesn't have the Editor. :-(

Spent some time to understand what was going on when I could not change the formatter, quite irritating issue!

We've been successfully using patch #51 for about a year now. However, we just installed the Paragraphs module and noticed an incompatibility with the patch. If a paragraph includes a formatted text field, the field seems unable to recognize that it's empty while the patch in place. This becomes apparent when the field is configured with an unlimited number of values. Even if the field is left blank when the node is saved, editing it later will reveal two instances of that field. Saving it again will result in three instances, then four, then five, and so on – even though all instances have been left blank. The problem goes away once the patch is removed. I believe Paragraphs always passes field formatting information, causing it to fail the isEmpty() test, even though the field is indeed blank.

Hi Slanger.

What you mention is exactly what I mentioned on #62. It is not exclusive of paragraphs but all the fields with formats.

With the patch any field with format will return false when execute isEmpty even if it actually is empty.

I noticed it while working on #59 and several tests started to fail because isEmpty was now returning the wrong value.

Hi,

You don't need to enter non-whitespace text in the default value field. Just go to admin/config/content/formats and drag the needed text format to the top. Save and go to your textfield and set the correct text format and it will save.

You don't need to enter non-whitespace text in the default value field. Just go to admin/config/content/formats and drag the needed text format to the top. Save and go to your textfield and set the correct text format and it will save.

But this will affect all the fields, no? what about if I just need one specific field to have a different text format by default?

Hi gnuget, you are right. This does not seem to work.

This is annoying! Anyone has a temp workaround?

Retagging

D8 version of this patch probably needs a reroll for 8.7.x
#1278886-59: Default text formats are not saved properly without accompanying values

There already is a D7 patch complete with tests (and it passes automated testing).
#2852019-2: D7 Default text formats are not saved properly without accompanying values

Hi,

rerolled #59 in the following patch

hope it'll be green

Attaching a re-roll of #78 for 8.8.x.

Attaching an updated patch that should fix all but one test fail.

The remaining test failing is Drupal\Tests\field_ui\Functional\ManageFieldsFunctionalTest::testCRUDFields. The failing condition occurs in Drupal\Tests\field_ui\Functional\ManageFieldsFunctionalTest::cardinalitySettings, which sets the body field cardinality to 6, creates two nodes, and then tests changing the cardinality to values lower than each of the nodes.

The expected result that is tested against is one node with a body field with two values and one node with a body field with three values. Because of the changes in this patch, both nodes actually have a body field with six values as the text format values are saved even though the text fields are empty.

It seems the last patches provided are based on #59 sadly the approach used in that patch is wrong.

Please check #62, #69 and #70 to know what unwanted effects that approach will cause.

But thanks for keeping this active!

I have rerolled #82 as it was not able to be applied to 8.8.1

I am trying to update my site from Drupal 7 to Drupal 8, but many features do not work
and the default error text formats are an additional hurdle
Since Drupal 8 this problem exists, can we hope for a solution before the arrival of Drupal 9

Hi, this issue is still active, did somebody found a workaround or do we need to patch the core in order to get it working properly? Thanks for the hints :-) :-) :-)

This patch is still needed if you want to specify a default text format with an empty text field - it won't work otherweise. I'm using it for many years now without any problems. You might also want to check the "allowed_formats" module and bug #784672.

This a generic problem for all fields with sub-values that are not part of the empty definition.
For example: on a link field you can't save a text for the link as a default value leaving the link empty.

I feel we shouldn't change the isEmpty definition for fields but rather save the values skipping the isEmpty check as they are going to be stored in config, not the database so null columns are OK in this context.

Then we probably need an additional logic to load those incomplete fields from config into the default values of the widget from the config.

Hi, thanks for the feedback :-) I will try "Allowed Formats" module. Thanks again :-)

Rerolled for 8.9.x

I solved the problem with the Allowed Formats module by allowing only one text format for a particular field.

UPDATE: The solution below doesn't work. It only updates the field settings, but it doesn't work as actual default value. F

I tried the whitespace workaround, but adding a space as default value cancels the required field (if used). Instead I modified the yml config for the fields till we're able to apply a stable patch. E.g.

in the field config yml change the default value from:

default_value: {  }

into

default_value:
  -
    value: ''
    format: filtered_html

I hope this helps. (Replace filtered_html with whatever the format you want to have as default value).

Not certain why the filter module dependency needed to be added to all the tests, and not just the test for the field, but here is a simple version of the patch that applies to 9.4

    $format = NULL;
    if (isset($this->properties['format'])) {
      $format = $this->get('format')->getValue();
    }
    return ($value === NULL || $value === '') && ($format === NULL || $format === '');

Not sure if this is a good way but this may avoid breaking tests that don't have a filter format and avoid breaking contrib modules.

Updating patch as suggested in comment #101.

Sweet, still needs a simple test to check that the configuration is set in the field settings config form.

setting back to NW for the tests.

Removing old tags.

Started a simple CR that is in draft.

Wrote a test for the default formatter (feedback welcome).

Digging into this more I wonder if there should be a different approach. This current one means that isEmpty() will never return true because it will return the format at the very least.

Let's see what breaks.

So not a fan of any of these solutions really. I think the best approach would be for https://www.drupal.org/project/drupal/issues/784672 to either be updated to allow for saving ordering of formats.

Or a follow up made after it's merged to do that. Should cover this scenario.

Now that https://www.drupal.org/project/drupal/issues/784672 landed is this still a bug?

This could still be valid.

You can now specify what formats are allowed per text field.
But you can't set a default format without a default value.

@smustgrave any change in your thinking since #113? Or are we looking for a new approach here?

Would have to think about it some. There's also been changes to the Field UX where the default field doesn't appear without being checked. And if you don't put a default value in it doesn't save as it being checked.

isEmpty() may still be the approach. Haven't tested since #784672: Allow text field to enforce a specific text format landed.

Rerolling the patch for D10.1 (without tests)