Problem/Motivation

Currently text fields can either be restricted to plain text, or the user may select between all accessible text formats independently of the context. This means that a privileged user who needs access to a permissive text formats (for example, to put tables or embedded remote content in basic pages) will get access to that format on every formatted text field (for example on a comment field).

There are three problems with this approach, and most experience Drupal developers have faced at least one of those in the past:

Consistency
At the moment you have to count on competence, good will and diligence of privileged users not to put certain markup in certain places. It would be convenient if a text field could be forced to use a specific text format (other than plain text). For example, you may want to make sure that comments only allow a very limited set of HTML tags ("filtertered HTML" for example) independently of the user's role, even if the same user has access to less restrictive formats in other places.
Usability
The ability to select text formats is a common source of confusion. By limiting the available text formats we also remove confusing user interface elements.
Security
If a privileged user account is taken over (for example, through social engineering), the attack surface is large due to the fact that every single text field can be used for XSS injections. By limiting where a dangerous text format can be used, we restrict the possibilities to inject malicious content.

Proposed resolution

Add an optional setting to the text field types that lets the site-builder determine if the text formats should be restricted. This setting is then used in the default textfield and textarea widgets to remove any non-allowed text formats. If nothing is set, the current behavior is unchanged.

Note that as it uses the underlying '#allowed_formats' form API property, the settings can't be used to give access to text formats that the user wouldn't have access to otherwise.

Remaining tasks

None

User interface changes

Checkboxes on list of available formats on text field configuration. Reduced set of allowed formats on content edit forms, where used. No fields use the new setting by default, so the patch doesn't affect the user interface for those who don't do anything with this functionality.

API changes

None

Data model changes

One setting is added to the field settings. The structure of the field data is unchanged.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task. No new functionality is really added, we are only fixing a confusing behavior.
Issue priority Normal
Prioritized changes The main goal of this feature is to improve usability and increase security by presenting less choices to the user, and preventing the use of text formats which otherwise the user would have access in places where these are by design, not necessary.
CommentFileSizeAuthor
#123 allow_text_field_to-784672-123.patch17.96 KBJo Fitzgerald
#111 interdiff.txt2.17 KBmohit_aghera
#111 allow_text_field_to-784672-111.patch17.9 KBmohit_aghera
#108 allow_text_field_to-784672-108.patch17.86 KBeiriksm
#107 allow_text_field_to-784672-107.patch17.71 KBeiriksm
#105 784672-105.patch17.7 KBSonal.Sangale
#102 interdiff-784672-99-102.txt1.29 KBfloretan
#102 784672-102.patch17.7 KBfloretan
#101 interdiff-784672-99-101.txt386 bytesfloretan
#101 784672-101.patch17.83 KBfloretan
#99 784672-99.patch17.27 KBlokapujya
#97 784672-reroll-97.patch583 byteslokapujya
#95 784672-reroll-95.patch17.33 KBlokapujya
#93 784672-reroll-93.patch17.46 KBlokapujya
#82 allow_text_field_to-784672-82.patch18.72 KBjcnventura
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch allow_text_field_to-784672-82.patch. Unable to apply patch. See the log in the details link for more information. View
#82 interdiff-78-82.txt776 bytesjcnventura
#78 allow_text_field_to-784672-78.patch18.72 KBjcnventura
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,631 pass(es), 2 fail(s), and 0 exception(s). View
#78 interdiff-75-78.txt7.79 KBjcnventura
#75 interdiff-65-75.txt3.92 KBbruvers
#75 allow_text_field_to-784672-75.patch17.34 KBbruvers
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,306 pass(es). View
#71 Screen Shot 2015-08-02 at 12.34.03.jpg358.09 KBLewisNyman
#71 Screen Shot 2015-08-02 at 12.29.05.jpg391.93 KBLewisNyman
#65 interdiff.txt1.94 KBjcnventura
#65 allow_text_field_to-784672-65.patch16.82 KBjcnventura
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,714 pass(es). View
#63 allow_text_field_to-784672-63.patch16.67 KBjcnventura
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,717 pass(es), 0 fail(s), and 1 exception(s). View
#62 784672-62-interdiff.txt3.83 KBlokapujya
#62 784672-62.patch16.67 KBlokapujya
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,204 pass(es). View
#59 allow_text_field_to-784672-58.patch15.56 KBfloretan
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,184 pass(es). View
#56 allow_text_field_to-784672-56.patch14.4 KBfloretan
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 98,150 pass(es), 2 fail(s), and 0 exception(s). View
#54 forced-text-formats.png31.39 KBmgifford
#53 allow_text_field_to-784672-53.patch12.79 KBvpeltot
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,750 pass(es). View
#45 limit_text_formats-784672-45.patch12.79 KBfloretan
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,195 pass(es). View
#43 limit_text_formats-784672-43.patch12.67 KBfloretan
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,737 pass(es), 8 fail(s), and 0 exception(s). View
#40 limit_text_formats-784672-40.patch12.71 KBfloretan
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,612 pass(es), 13 fail(s), and 0 exception(s). View
#38 limit_text_formats-784672-38.patch12.71 KBfloretan
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,617 pass(es), 13 fail(s), and 0 exception(s). View
#36 limit_text_formats-784672-36.patch11.89 KBfloretan
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,616 pass(es), 11 fail(s), and 0 exception(s). View
#34 limit_text_formats-784672-34.patch3.64 KBfloretan
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 50,163 pass(es), 459 fail(s), and 437 exception(s). View
#16 drupal-enforce-specific-text-formats-784672-16.patch2.21 KBwjaspers
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-enforce-specific-text-formats-784672-16.patch. Unable to apply patch. See the log in the details link for more information. View
#2 text-field-format-2.patch23.32 KBc960657
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch text-field-format-2.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots. View
text-field-format-1.patch17.21 KBc960657
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch text-field-format-1.patch. Unable to apply patch. See the log in the details link for more information. View
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

c960657’s picture

Status: Needs review » Needs work
c960657’s picture

Status: Needs work » Needs review
FileSize
23.32 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch text-field-format-2.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots. View

Slightly new approach: '#type' => 'text_format' now has a new property, #formats, if only a subset of the format accessible to the user should be available in this field instance.

What do you think of this?

bjaspan’s picture

I don't see how this can go into D7 at this point.

webchick’s picture

Version: 7.x-dev » 8.x-dev

Agreed. Sorry.

c960657’s picture

Title: Allow text field to enfore a specific text format » Allow text field to enforce a specific text format
David_Rothstein’s picture

Something similar to the filter.module part of this issue was discussed at length in #414424: Introduce Form API #type 'text_format' but ultimately did not make it into the final patch. See for example:
http://drupal.org/node/414424#comment-1617600
http://drupal.org/node/414424#comment-2177612
(and many subsequent comments)

The argument I tried to make there was that since we were already doing a massive API change in that issue (completely reworking how #text_format works), it would make sense to slip in both something like the #formats option described here, as well as a #default_format option, since it would require almost no extra work to do so. I was overruled though...

So personally, if limited to the filter.module at least (the field module changes do seem like they might be too much for now) I'd be very in favor of still trying to do this in D7 :) It's possible to do this kind of thing in a contrib module - and people will undoubtedly try to do it - but it's tricky to do correctly (involves somewhat complex form API stuff that most people don't know about), and if you do it wrong, you are very likely to introduce a security issue. Plus, the filter.module changes here wouldn't break any existing code, just introduce a new optional property to the form element. I think adding this API functionality to the Filter module in a way that we know will be easy for contrib modules to use securely is definitely still worth doing. Anyone else agree? And would doing that help allow a contrib module to make the changes to the field UI/functionality that are contained in the rest of this patch?

ogi’s picture

It's sad that such essential ability for user-friendly forms is not in Drupal 7. Please reconsider :-)

ogi’s picture

Looking at the patch in #2, it doesn't provide what I want - ability to hide format fieldset altogether and wysiwyg+tinymce to do its job.

mightyiam’s picture

+1 for Drupal 7!

Hanno’s picture

Issue tags: +accessibility

subscribe, a default text format for textfields might be also relevant to allow adding html tags for accessiblity.

Everett Zufelt’s picture

Issue tags: -accessibility

This may help accessibility in some situations, but definitely isn't an accessibility issue. Removing tag, thanks for bringing it to my attention.

Hanno’s picture

Issue tags: +atag, +wcag2, +language of parts

OK. Tagging this as WCAG/ATAG and 'language of parts' as this feature request would help create accessible content, especially language tagging (WCAG2.0 3.1.2)

mgifford’s picture

#2: text-field-format-2.patch queued for re-testing.

mgifford’s picture

Issue tags: +accessibility

Tagging for accessibility.

Status: Needs review » Needs work

The last submitted patch, text-field-format-2.patch, failed testing.

wjaspers’s picture

FileSize
2.21 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-enforce-specific-text-formats-784672-16.patch. Unable to apply patch. See the log in the details link for more information. View

Drupal 7.x patch on-the-way.

I didn't change how the filter formats permissions are checked against the field, since this will require additional community feedback.
I.E. Should the user's permission to use the format supercede their ability to use a format specifically defined on a text field?

This probably isn't the most efficient patch in the world, but I have tested it locally against 7.x-dev and it works!

EDIT: Opened #1419016: Allow text fields to enforce a specific format so it could be tested against D7.

wjaspers’s picture

Status: Needs work » Needs review

Oops, forgot to set needs review.

Status: Needs review » Needs work

The last submitted patch, drupal-enforce-specific-text-formats-784672-16.patch, failed testing.

David_Rothstein’s picture

I think the strategy we need to take to implement this is:

1. Create an API in the filter system for any module which provides text format elements to specify which formats they want to allow in that instance.
2. Have the Text module implement that API.

We should really avoid hardcoding any special behavior for text fields inside the filter system itself, because that will limit how this can be used.

See also the related issue at #1423244: #allowed_formats property for #type 'text_format' to filter available formats. It almost would make sense to postpone this issue on that one (because that's basically #1 above), except that there's lots of code here already...

Note that there are also a lot of potential security issues that need to be dealt with in implementing this (see #1295248: Allow per-field-instance configuration of allowed formats for discussion and code), and that's one of the reasons why it would be good to at least have the API for this in core regardless of whether or not the code that implements it lives in core too.

mgifford’s picture

We need clarification on why this is an accessibility issue. Sorry.

Hanno’s picture

Some while ago I tagged this issue as helping accessibility. For accessibility (or markup) in single line textfields we need sometimes additional HTML inline tags (span (for 'lang'), dfn and abbr). At the other hand, we don't want to allow HTML block level tags (h1, h2, h3, p) in single line fields, only in textareas.

So, in a website there are possibly two separate filter formats needed: one for single line textfields and one for textareas (where HTML block level tags are allowed). The end-user shouldn't be allowed to mix these sets, because we will end up with content that doesn't validate (ATAG2.0 issue (http://www.w3.org/TR/ATAG20/#principle_b2))
As abbr and dfn is needed on level AAA, it is mostly needed for language of parts (level AA), when someone wants to add text in another language (for example <span lang="fr">Tour de France</span>).

mgifford’s picture

Wished you'd been able to come over for A11ySprint by-the-way. You'd have had a great time with us I'm sure.

I think we just need a bit more of a use case where it's clear that this additional functionality meets the needs of the 80%.

How would this practically be used? I do think that there are a lot of instances where Languages of Parts really isn't dealt with all that well as you know.

What are the options for implementing this now in D7?

Hanno’s picture

@mgifford yes, it would have been nice to help during the sprint.

This issue is at the moment not relevant for most of the users. It becomes more relevant as soon as the node title becomes a text field (#1188394: Make title behave as a configurable, translatable field (again)). At the moment it is not possible to produce titles with language information nor semantic markup (part of WCAG 1.3.1 Info and Relationships: Information, structure, and relationships conveyed through presentation can be programmatically determined or are available in text. (Level A)).

A technique for this is using semantic markup to mark emphasized or special text. For this certain - inline level - HTML tags are needed. Here are some examples taken from WCAG H49:

An excerpt from the <cite>The Story of my Life</cite> by Helen Keller
What she <em>really</em> meant to say was, "This is not ok,  it is <strong>excellent</strong>"!
Helen Keller said, "<q>Self-pity is our worst enemy and if we yield to it, we can never do anything good in the world.</q>"
Beth received 1<sup>st</sup> place in the 9<sup>th</sup> grade science competition.
The chemical notation for water is H<sub>2</sub>O.

Note: Instead of having two filter formats (one for text fields, one for text areas), an alternative could be to automatically disable the block level html tags in filter formats applied to text fields and only allowing them on text areas.

mgifford’s picture

Sadly this issue has been left behind. @Hanno are you in a place where you can try to update the patch?

sun’s picture

Component: field system » text.module
floretan’s picture

#1423244: #allowed_formats property for #type 'text_format' to filter available formats has a patch ready to be reviewed which adds some missing functionality in the text_format form element provided by the filter module. Once it's ready we can use it to make it possible to set the format for a specific field.

BrianLewisDesign’s picture

Issue summary: View changes

This feature is key. Please don't leave it behind. You should be able to select available formats per field. Some fields require a specific one or things break. (I guess if a user didn't have permissions for the available format it should make the field un-editable for them).

mgifford’s picture

Issue tags: +Needs reroll

Agreed. I'm wondering about tagging it for security. It's come up a few times thus far in this thread.

dsoini’s picture

This is very desirable to me. I have had to go about it with an ugly hack:

1. Created a custom module that searches the edit/add form for any field that has text_formats applied to it (sometimes there are many).
2. Hide the drop-down list but keep the guidelines for the default format displayed.
3. I set a default format by entering default text into the content-type field definition. The default text is

.

.
4. My modules looks for that default value and unsets it. I don't want the field polluted with this ugly data.

I like this functionality because sometimes I want a field that is just a simple 3 line box for entering a short amount of minimally-styled text. Perhaps I only want people to be able to use HTML to link some words in the text, but nothing else. I don't want them to have to switch between all the available content types, I don't want them to use a wysiwyg editor and be able to insert tables and headings and videos. I only want them to be presented with a simple box with guidelines on the limited allowed html they can use that is appropriate for that particular field.

The last submitted patch, text-field-format-1.patch, failed testing.

jenlampton’s picture

Issue tags: +needs backport to D7
mgifford’s picture

Looks like #1423244-68: #allowed_formats property for #type 'text_format' to filter available formats was committed 10 months ago. @floretan suggested in #26 above that it "adds some missing functionality in the text_format form element provided by the filter module. Once it's ready we can use it to make it possible to set the format for a specific field."

Thanks for the link @jenlampton and yes, hopefully it gets backported.

floretan’s picture

Status: Needs work » Needs review
Issue tags: +drupaldevdays
FileSize
3.64 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 50,163 pass(es), 459 fail(s), and 437 exception(s). View

The last patch didn't apply at all anymore, but the changes to the typed data API from the past two years have made it quite easy to rebuild this functionality.

Here's what this patch does:

  1. Make an additional setting available on the field editing form for all formatted text fields, like the body field of nodes for example. The checkboxes let you select what text formats should be available for that field.
  2. Use that setting on the widgets for fields of this type to determine what formats are available using the new #available_formats attribute from #1423244: #allowed_formats property for #type 'text_format' to filter available formats. Note that this only restricts what formats are available, it doesn't make formats available that the user doesn't have access to.

Open questions:

  1. Do we want to make the fallback format an option?
  2. The current patch adds the settings on the field level, based on the fact that it really affects what kind of data gets stored in the field, not only how it's entered. However, the validation is only done at the widget level. We should probably add some validation at the field level as well.

Once this gets in, we can have a follow-up issue where we check all fields in the default installation profile that should have limited text formats. For example, comments should probably be limited to "Restricted Html".

Status: Needs review » Needs work

The last submitted patch, 34: limit_text_formats-784672-34.patch, failed testing.

floretan’s picture

Status: Needs work » Needs review
FileSize
11.89 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,616 pass(es), 11 fail(s), and 0 exception(s). View

Adding default configurations to fix most of the exceptions seen in the test results.

Status: Needs review » Needs work

The last submitted patch, 36: limit_text_formats-784672-36.patch, failed testing.

floretan’s picture

Status: Needs work » Needs review
FileSize
12.71 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,617 pass(es), 13 fail(s), and 0 exception(s). View

Fixing two more failures. The remaining failures are related to FieldUiTestTrait and I'm not sure how to fix them yet. Any help on that would be appreciated.

Status: Needs review » Needs work

The last submitted patch, 38: limit_text_formats-784672-38.patch, failed testing.

floretan’s picture

Status: Needs work » Needs review
FileSize
12.71 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,612 pass(es), 13 fail(s), and 0 exception(s). View

Oh, the ordering of items in an array matters for comparison.

yannickoo’s picture

  1. +++ b/core/modules/block_content/block_content.module
    @@ -83,7 +83,10 @@ function block_content_add_body_field($block_type_id, $label = 'Body') {
    +        'available_formats' => array(),
    
    +++ b/core/modules/config_translation/src/Tests/ConfigTranslationListUiTest.php
    @@ -401,7 +401,10 @@ public function doFieldListTest() {
    +        'available_formats' => array(),
    
    +++ b/core/modules/migrate_drupal/src/Tests/d6/MigrateFieldInstanceTest.php
    @@ -90,7 +90,10 @@ public function testFieldInstanceSettings() {
    +      'allowed_formats' => array(),
    
    +++ b/core/modules/node/node.module
    @@ -328,7 +328,10 @@ function node_add_body_field(NodeTypeInterface $type, $label = 'Body') {
    +        'available_formats' => array(),
    

    Just a minor thing: We should use the new syntax here, just [] instead of array().

  2. +++ b/core/modules/text/src/Plugin/Field/FieldType/TextItemBase.php
    @@ -12,12 +12,37 @@
    +    $element['available_formats'] =  array(
    

    Another minor thing: We should remove that " " two spaces there.

Status: Needs review » Needs work

The last submitted patch, 40: limit_text_formats-784672-40.patch, failed testing.

floretan’s picture

FileSize
12.67 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,737 pass(es), 8 fail(s), and 0 exception(s). View

Fix the confusion between "available" and "allowed".

@yannickoo: I also fixed the double spacing in TextItemBase.php. Regarding the array notation, all of the modified files use the old array() notation, so I left it as is to not add unrelated changes with the patch.

floretan’s picture

Status: Needs work » Needs review
floretan’s picture

FileSize
12.79 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,195 pass(es). View

This new patch fixes the schema definition for the individual formats in allowed_formats fields in text.schema.yml, which should fix the remaining failures.

The last submitted patch, 43: limit_text_formats-784672-43.patch, failed testing.

yannickoo’s picture

Status: Needs review » Reviewed & tested by the community

Ah sequence property did the trick ;)

Patch works fine, code looks good and no more failing tests!

floretan’s picture

Status: Reviewed & tested by the community » Needs work

Actually, there are few more things that need to happen.

At the moment, the allowed formats get exported like this (because that's the way checkbox values are submitted):

  allowed_formats:
    basic_html: basic_html
    restricted_html: restricted_html
    full_html: 0
    plain_text: 0

What we want is something like

  allowed_formats:
    - basic_html
    - restricted_html

Once we have this, we can update the configuration schema in text.schema.yml. The allowed formats are a sequence, but their values are not just any strings. So we should replace

    allowed_formats:
      type: sequence
      label: 'Allowed text formats'
      sequence:
        type: string

with something like

    allowed_formats:
      type: sequence
      label: 'Allowed text formats'
      sequence:
        type: filter.format.[%value]

One final but important addition we would need is some test coverage. The #allowed_formats form API property is already covered, but we probably need to extend some more textfield-related tests.

One issue pointed out by @maijs is that when applying this patch and there are already existing fields without the allowed_formats setting we get "undefined index" notices. We don't do something similar for other settings that have always been there, so I'm not sure if this is needed since it's one case of beta-to-beta upgrade. Saving the field settings form would get rid of those notices.

rpayanm’s picture

Issue tags: -Needs reroll

Status: Needs work » Needs review
vpeltot’s picture

Status: Needs review » Needs work
woprrr’s picture

This patch work fine in my case !! Thanks @floretan

ps : It would just be more optimized to put as select.

vpeltot’s picture

Status: Needs work » Needs review
FileSize
12.79 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,750 pass(es). View

Reroll.

Migrate installation confiuration files (core/modules/migrate_drupal/config/install) was moved to core/modules/migrate_drupal/config/optionnal.

mgifford’s picture

Issue summary: View changes
FileSize
31.39 KB

You can see this here /admin/structure/types/manage/page/fields/node.page.body

shot of admin page

Looks good to me. What else, if anything, is needed before this is marked RTBC?

floretan’s picture

The schema change I mentioned in #48 is not necessarily needed, but a test should be included.

floretan’s picture

FileSize
14.4 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 98,150 pass(es), 2 fail(s), and 0 exception(s). View

While working on more real Drupal 8 projects, this functionality seems to be needed everywhere.

Here's the patch re-rolled, with some tests added. This should be good now.

Status: Needs review » Needs work

The last submitted patch, 56: allow_text_field_to-784672-56.patch, failed testing.

lokapujya’s picture

+++ b/core/modules/text/src/Tests/TextFieldTest.php
@@ -182,6 +182,31 @@ function _testTextfieldWidgetsFormatted($field_type, $widget_type) {
+    // Limit our field to only allow the newly created format.
+    $field_storage->set('settings', array('allowed_formats' => array($format_id)));

The set() is not working. I tried setSetting() and even calling save() after. The allowed_formats doesn't get saved to field storage.

floretan’s picture

Status: Needs work » Needs review
FileSize
15.56 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,184 pass(es). View

Thanks @lokapujya. The format ID should be in the key of the settings array, not in the value.

I also realised that the patch I uploaded somehow had an older version of the test. Here is the correct one (no functional change, only the test is different).

rcross’s picture

is there a d7 version of this patch (or one in the works)?

floretan’s picture

@rcross: it could be done, but #1423244: #allowed_formats property for #type 'text_format' to filter available formats would need to be committed first.

lokapujya’s picture

FileSize
16.67 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,204 pass(es). View
3.83 KB

Changed the test so that it first has 2 formats. Verify that the format select shows up. Change the field to only have 1 format. Then, verify that the format selector doesn't show up.

Also, made the following changes:

  1. +++ b/core/modules/text/src/Tests/TextFieldTest.php
    @@ -84,6 +84,69 @@ function testTextfieldWidgetsFormatted() {
    +    // Create a second text formats.
    

    format (singular.)

  2. +++ b/core/modules/text/src/Tests/TextFieldTest.php
    @@ -84,6 +84,69 @@ function testTextfieldWidgetsFormatted() {
    +      'settings' => array('allowed_formats' => array($format1->id() => TRUE)),
    

    This still wasn't working. The format selector was not showing up, but only because no formats were allowed.

  3. +++ b/core/modules/text/src/Tests/TextFieldTest.php
    @@ -84,6 +84,69 @@ function testTextfieldWidgetsFormatted() {
    +    $this->assertFieldByName("{$field_name}[0][value]", '', 'Widget is displayed');
    

    '' should be NULL.

jcnventura’s picture

FileSize
16.67 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,717 pass(es), 0 fail(s), and 1 exception(s). View

Re-rolling patch, due to changes made in #2533994: Move module-specific migration support into the block_content module

Skipping the interdiff, as it was a simple file rename from migrate_drupal to block_content

Status: Needs review » Needs work

The last submitted patch, 63: allow_text_field_to-784672-63.patch, failed testing.

jcnventura’s picture

Status: Needs work » Needs review
FileSize
16.82 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,714 pass(es). View
1.94 KB

Prior to the use of array_filter, check if the value to be filtered is an array.

lauriii’s picture

jcnventura’s picture

Issue summary: View changes
jcnventura’s picture

Issue summary: View changes
floretan’s picture

Issue summary: View changes

Updating the summary.

lauriii’s picture

Category: Feature request » Task

It seems more like a task than a complete new feature. Beta evaluation needs update too.

LewisNyman’s picture

Issue summary: View changes
Status: Needs review » Needs work
FileSize
391.93 KB
358.09 KB

Nice, this is a really cool feature. I reviewed the usability of the new UI. Only minor comments:

Can we move these options so they are above the default value display? It is quite big and has shown a tendency to be distracting in usability tests. If we move the options above the default value they have a better chance of being noticed.

Here's a screenshot displaying the changes to the content creation screen. When there are multiple choices it's the same UI. When there is only one choice the UI looks like this:

Can we remove the text formats help link if there are no options? It doesn't seem to provide any value.

floretan’s picture

Status: Needs work » Needs review

Thanks @LewisNyman for the review.

I agree that placing these options above the default value would make sense, but that would be different from what every other field configuration form does (the default value always comes first), so I'm not sure we want to do that.

I also agree that the link "About text formats" doesn't make sense if there is only one text format available. The changes in this issue make that problem apparent, but the problem is independent. I created #2544188: Hide the link "About text formats" if only one format is available to address that issue, it probably shouldn't be combined with this one.

jcnventura’s picture

fgm’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/text/src/Plugin/Field/FieldWidget/TextareaWidget.php
    @@ -35,6 +35,14 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
    +      if (!empty($allowed_formats) && !$this->isDefaultValueWidget($form_state)) {
    

    Probably want to avoid two function calls by refactoring to a temporary variable.

  2. +++ b/core/modules/text/src/Plugin/Field/FieldWidget/TextfieldWidget.php
    @@ -35,6 +35,14 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
    +      $allowed_formats = array_filter($this->getFieldSetting('allowed_formats'));
    

    Probably want to avoid two function calls by refactoring to a temporary variable, here too.

EDIT because DrEditor doesn't make it really clear, this is about the repeated calls to $this->getFieldSetting('allowed_formats')); in both cases.

bruvers’s picture

Status: Needs work » Needs review
FileSize
17.34 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,306 pass(es). View
3.92 KB

The latest patch from #65 did not apply correctly anymore because 2 files were moved by recently commited issues #2415335 and #2534010. The files that have been moved and/or renamed are

  • core/modules/block_content/migration_templates/d6_block_content_body_field.yml and
  • core/modules/migrate_drupal/src/Tests/d6/MigrateFieldInstanceTest.php .

The attached patch fixes that and implements the suggestions from #74.

yannickoo’s picture

+++ b/core/modules/field/src/Tests/Migrate/d6/MigrateFieldInstanceTest.php
@@ -78,7 +78,10 @@ public function testFieldInstanceSettings() {
-    $expected = array('max_length' => 255);
+    $expected = array(
+      'allowed_formats' => array(),
+      'max_length' => 255,
+    );

What do you think about using $expected = ['max_length' => 255] instead of array() syntax?

lokapujya’s picture

Does #71 & #72 qualify as the usability review?

jcnventura’s picture

FileSize
7.79 KB
18.72 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,631 pass(es), 2 fail(s), and 0 exception(s). View

Short array syntax FTW!

Status: Needs review » Needs work

The last submitted patch, 78: allow_text_field_to-784672-78.patch, failed testing.

lokapujya’s picture

What happens if the field allows only one format, but the user doesn't have access to that format?

The last submitted patch, 78: allow_text_field_to-784672-78.patch, failed testing.

jcnventura’s picture

Status: Needs work » Needs review
FileSize
776 bytes
18.72 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch allow_text_field_to-784672-82.patch. Unable to apply patch. See the log in the details link for more information. View

Stupid array comparison.

jcnventura’s picture

@lokapujya: Not sure how to reproduce that.. The plain text format is always available. If you find a way to disable or delete it, I can then answer your question. My guess is that the correct reply to your question is that, by design, Drupal will always allow you to use the plain text format.

The last submitted patch, 78: allow_text_field_to-784672-78.patch, failed testing.

lokapujya’s picture

My guess is that the correct reply to your question is that, by design, Drupal will always allow you to use the plain text format.

That's true, but, now with this patch, it can be configured that "plain text" is not allowed for the field.

What happens if the field allows only one format, but the user doesn't have access to that format?

The content editor will get this message in the input box when adding a new node, "This field has been disabled because you do not have sufficient permissions to edit it." That is the same message the content editor would get if he edited a piece of content and doesn't have access to the format.

Bojhan’s picture

Issue tags: -Needs usability review

Looks good!

Status: Needs review » Needs work

The last submitted patch, 82: allow_text_field_to-784672-82.patch, failed testing.

floretan’s picture

Version: 8.0.x-dev » 8.1.x-dev
Status: Needs work » Postponed

After discussing this with various people at the code sprint, it became clear that the best way to approach this issue is to implement it as a contrib module for now and eventually integrate it in Drupal 8.1. At this point the somewhat unclear logic for all fallback cases will be battle-tested.

I created a module here https://www.drupal.org/project/allowed_formats, we are working on converting the latest patch from this issue to a contrib module. Please join that module's issue queue if you want to contribute.

floretan’s picture

The Allowed Formats module (https://www.drupal.org/project/allowed_formats) is now functional.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

mlncn’s picture

Status: Postponed » Needs work

Nice that there's a contrib module, but can we go back to getting this into core, now targeting 8.2?

lokapujya’s picture

Status: Needs work » Needs review
FileSize
17.46 KB

not tested.

Status: Needs review » Needs work

The last submitted patch, 93: 784672-reroll-93.patch, failed testing.

lokapujya’s picture

Status: Needs work » Needs review
FileSize
17.33 KB

Status: Needs review » Needs work

The last submitted patch, 95: 784672-reroll-95.patch, failed testing.

lokapujya’s picture

Status: Needs work » Needs review
FileSize
583 bytes

this is the interdiff.

Status: Needs review » Needs work

The last submitted patch, 97: 784672-reroll-97.patch, failed testing.

lokapujya’s picture

Status: Needs work » Needs review
FileSize
17.27 KB

Status: Needs review » Needs work

The last submitted patch, 99: 784672-99.patch, failed testing.

floretan’s picture

FileSize
17.83 KB
386 bytes

Updated patch that addresses the DefaultConfigTest failures.

floretan’s picture

Also fix MigrateFieldInstanceTest failure.

colan’s picture

Anyone have thoughts on whether #1278886: Default text formats are not saved properly without accompanying values should be postponed until we get this one done? This might affect how that one is solved.

lokapujya’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

The last retest failed to apply.

Sonal.Sangale’s picture

Assigned: Unassigned » Sonal.Sangale
Status: Needs work » Needs review
FileSize
17.7 KB

Relolling the patch.

Sonal.Sangale’s picture

Assigned: Sonal.Sangale » Unassigned
eiriksm’s picture

Patch does not apply anymore (because forum config was renamed).

Code looks ok though.

Re-roll patch.

eiriksm’s picture

Hm, maybe we should add a description to the field. For example. it is not immediately obvious what happens if you select no allowed formats.

Here is a patch with only that added.

eiriksm’s picture

Issue tags: +DevDaysMilan
hchonov’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/text/src/Plugin/Field/FieldType/TextItemBase.php
    @@ -7,12 +7,36 @@
    +  public function fieldSettingsForm(array $form, FormStateInterface $form_state) {
    

    inheritdoc missing.

  2. +++ b/core/modules/text/src/Plugin/Field/FieldWidget/TextfieldWidget.php
    @@ -25,11 +25,20 @@ class TextfieldWidget extends StringTextfieldWidget {
    +    if (is_array($allowed_formats_setting)) {
    

    The is_array check is not needed as it impossible to get anything different than an array there.

  3. +++ b/core/modules/text/src/Plugin/Field/FieldWidget/TextfieldWidget.php
    @@ -25,11 +25,20 @@ class TextfieldWidget extends StringTextfieldWidget {
    +      $allowed_formats = array_filter($allowed_formats_setting);
    

    Why do we need to filter the array here?

mohit_aghera’s picture

Status: Needs work » Needs review
FileSize
17.9 KB
2.17 KB

@hchonov
I have fixed 1 and 2

I tested by removing point 3 $allowed_formats = array_filter($allowed_formats_setting);
and tested manually. It seems worked fine for me.
still we can get some inputs from @eiriksm

eiriksm’s picture

Feedback totally makes sense. As I said it was more or less a re-roll.

Nice points!

Status: Needs review » Needs work

The last submitted patch, 111: allow_text_field_to-784672-111.patch, failed testing.

hchonov’s picture

@mohit_aghera when you are posting an interdiff please make it so that it indicates all the changes that are included, in your case the third change is to see in the interdiff, but not in the posted patch.

  1. +++ b/core/modules/text/src/Plugin/Field/FieldType/TextItemBase.php
    @@ -16,6 +17,31 @@
    +      '#title' => t('Allowed text formats'),
    ...
    +      '#description' => t('Select the allowed text formats. If no formats are selected, all available text formats will be displayed to the user.'),
    

    Use $this->t() instead of t() whenever possible.

  2. +++ b/core/modules/text/src/Plugin/Field/FieldType/TextItemBase.php
    @@ -16,6 +17,31 @@
    +      '#options' => $this->getProperties()['format']->getPossibleOptions(),
    

    The property should be retrieved with $this->get('format').

  3. +++ b/core/modules/text/src/Tests/TextFieldTest.php
    @@ -142,6 +142,77 @@ function testTextfieldWidgetsFormatted() {
    +
    ...
    @@ -182,8 +253,8 @@ function _testTextfieldWidgetsFormatted($field_type, $widget_type) {
    

    An empty line is not needed here.

tstoeckler’s picture

Some high-level remarks on the current approach:

We need to implement calculateDependencies() to declare a dependency on the text formats that we are limiting the text field to. Then we also need to implement onDependencyRemoval() to remove a text format from the list of allowed formats in case it is removed. This then becomes problematic if we are removing the single text format that was allowed for a field. In that case the allowed_formats array is empty which means that all formats are allowed which would be a security problem.

I think that problem hints to the fact that the "no values means all values" pattern employed here - while being used elsewhere in Drupal as well - really is problematic and I think we would be wise to avoid it in this case. I would suggest to add a separate restrict_formats boolean setting that activates this behavior. Then the allowed_formats setting really is just that: the list of allowed formats. And if it is empty (and restrict_formats is TRUE) then it simply means that no text formats are allowed and thus text cannot be submitted until this issue is resolved either by allowing one or more formats or by deactivating restrict_formats altogether.

I by no means think that we always need to have a 1-1 relationship between our UI elements and our storage but in this particular case I think the separate setting would make the UI less confusing as well. We could have a simple Restrict text formats checkbox and only when it is checked does the list of text formats to allow appear.

Would love to hear your thoughts on this, but marking needs work because - as explained in the first paragraph - the current patch is not sufficient, even if we pursue a different approach than outlined here.

hchonov’s picture

@tstoeckler what about if we just name it restricted_formats or enforced_formats and in this case only use what is set there without the boolean flag.

tstoeckler’s picture

Issue tags: -Needs reroll

i.e. always restrict the list of text formats, and just have it default to all formats by default? I am not sure about that. Given the properly implemented dependencies, it would mean that every textfield has a dependency on every text format or a subset of them but at the very least on one. That could be seen as a benefit if you see it as already existing dependencies becoming more explicit or you could see it as a a problem if you argue that it increases coupling of different systems (text storage vs. text processing). Not sure where I stand on that myself.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

nerdstein’s picture

I am curious if this should be closed since the Allowed Formats module is offering this functionality (per: https://www.drupal.org/node/784672#comment-10373375)

Berdir’s picture

I think the point of that issue is to bring that functionality into core which would IMHO make sense as it is a very useful feature. We only started that contrib module because we didn't manage to get it into core for 8.0.

lokapujya’s picture

Issue tags: +Needs reroll

Doesn't apply to 8.4

Jo Fitzgerald’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
17.96 KB

Re-rolled.

Status: Needs review » Needs work

The last submitted patch, 123: allow_text_field_to-784672-123.patch, failed testing.

tstoeckler’s picture

Having thought about this for a while and having watched a bunch of other bugs being fixed with config dependencies in the meantime, I am convinced that we need to restructure the configuration that is currently being pursued by this patch. Feel free to disagree, of course, but this is what I think what we should do:

* Have a separate "restrict_formats" boolean flag, that defaults to FALSE.
* Have a "allowed_formats" array (just like now), that defaults to an empty array.

Then if "restrict_formats" is FALSE, then we do nothing. As soon as it is used then we use the "allowed_formats" array.
This is for the following reason:

We will have to add a config dependency on any text format that is being used. Then if a format is being deleted we will need to remove that format from the list of allowed formats. If that format happened to be the *last* format, then it must not be possible to use any format at all until the field is re-configured explicitly. Anything else would be a security problem. Currently if we remove the last format, the field is interpreted as not restricting formats as all.

I hope that was clear, this is kind of hard to explain... What do you think?

Berdir’s picture

Unexpected behavior, yes, security issue, no, IMHO. This is not a security feature, it's a user experience improvement. Security is handled through permissions and you'd still only have access to formats that you are allowed to use.

I think showing all would be OK, we don't really have an API/UI to handle having no allowed format, it would likely just fall back to the fallback format, which is most likely not what you want.

We could also force you to fix your configuration first by letting the field be deleted, but we all know that users don't really read those warnings (e.g. the related views deletion issue), at least not with the current UI.

mofdi’s picture

to resolve this problem, i used allowed_formats module

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.