Problem/Motivation

The ui_patterns_element_info_alter() implementation adds WysiwygWidget::textFormat as #pre_render callback for all text_format elements.

Unfortunately this breaks core's functionality to limit a text fields allowed text formats. In my case, there is only Full HTML allowed, but with ui_patterns module installed, the text format selection is visible again and also allows to select Plain text.

Additionally the code of WysiwygWidget::textFormat looks quite outdated and e.g. names Drupal\filter\Element\TextFormat::processTextFormat() which does not exist anymore (it is TextFormat::processFormat() now and has a lot more code to allow limiting formats).

Steps to reproduce

  • Make sure ui_patterns module is not installed
  • Add a Formatted text field to any fieldable node type and only allow e.g. a single text format for that field in its settings
  • Go to the entity form of that entity type and see the formatted text field with no Text format selection available (or only allowing to select one of the allowed text formats, if more than one was allowed in field config)
  • Install ui_patterns module
  • Go to same entity form and see the formatted text field WITH Text format selection and too many options (not only the ones, that were allowed in field config)

Proposed resolution

  • Check if code in WysiwygWidget::textFormat is needed at all
  • Fix or remove code, so allowed formats settings are also respected, when ui_patterns module is installed

Remaining tasks

  • Create issue fork and MR to fix this issue

User interface changes

n/a

API changes

n/a

Data model changes

n/a

Issue fork ui_patterns-3465517

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

hctom created an issue. See original summary.

hctom’s picture

hctom
Primary language German
commented
Issue summary: View changes

Fixed some typos in the issue description ;)

pdureau’s picture

pdureau commented
Assigned: Unassigned » just_like_good_vibes

Thanks a lot Tom for using an early version of UI Patterns 2.x and helping us making the module ready for beta.

Additionally the code of WysiwygWidget::textFormat looks quite outdated and e.g. names Drupal\filter\Element\TextFormat::processTextFormat() which does not exist anymore (it is TextFormat::processFormat() now and has a lot more code to allow limiting formats).

Indeed. It was not caught by our quality tools because it was in a comment.

The ui_patterns_element_info_alter() implementation adds the WysiwygWidget::textFormat #pre_render callback for all text_format elements. Unfortunately this breaks core's functionality to limit a text fields available text formats. In my case, there is only Full HTML allowed, but with ui_patterns module installed, the text format selection is visible again and also allows to select Plain text.

@just_like_good_vibes can you have a look? It seems this is the follow-up of #3456083: [2.0.0-alpha3] WysiwygWidget bug with one text format

pdureau’s picture

pdureau commented
Title: ui_patterns_element_info_alter() implementation overrides allowed formats for all text_format elements » [2.0.0-beta1] ui_patterns_element_info_alter() implementation overrides allowed formats for all text_format elements
just_like_good_vibes’s picture

just_like_good_vibes
Location PARIS
commented

Hello, yes i will take the issue. Thank you for reporting the bug hctom.
i will try to fix this very soon.

hctom’s picture

hctom
Primary language German
commented
Related issues: +#3456083: [2.0.0-alpha3] WysiwygWidget bug with one text format

You're welcome - I'm currently evaluating the state of ui_patterns and I am quite impressed so far! ;) Thanks a lot and keep up the good work.

just_like_good_vibes opened merge request !167

just_like_good_vibes’s picture

just_like_good_vibes
Location PARIS
commented
Status: Active » Needs review

thank you for your nice comment :)
i have posted a fix, please review.

The reason why. we use pre-render, is that we override the default behavior proposed by the form element to always propose the option "Plain text" and to allow all site-wide text formats.
Maybe we could renovate that in the future and simplify.

please review if you have some time to do it

hctom’s picture

hctom
Primary language German
commented
Status: Needs review » Reviewed & tested by the community

Thanks for your fast response and fix. This looks good to me. In my node forms etc. the Text format select element is gone again, when only a single text format is allowed and the Wysiwyg source still has the Text format select element available.

I'd also say moving the #pre_render callback assignement to the actual source plugin is definitely better, so that code does not run for all text_format elements.

Thanks a lot again and: RTBC from my side ;)

just_like_good_vibes’s picture

just_like_good_vibes
Location PARIS
commented

thanks for the quick review. Indeed this was clearly a bug the way it was coded.

And yes we keep the select list in the Wysiwyg source form, even if only one format is available.
And we actually have two possible selections with the "Plain text" :)

just_like_good_vibes’s picture

just_like_good_vibes
Location PARIS
commented
Assigned: just_like_good_vibes » Unassigned
Status: Reviewed & tested by the community » Fixed
pdureau’s picture

pdureau commented
Status: Fixed » Closed (fixed)