Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Filtered / Basic HTML format should disable filter_autop #6429

Open
leeksoup opened this issue Mar 19, 2024 · 6 comments · May be fixed by backdrop/backdrop#4679
Open

Filtered / Basic HTML format should disable filter_autop #6429

leeksoup opened this issue Mar 19, 2024 · 6 comments · May be fixed by backdrop/backdrop#4679

Comments

@leeksoup
Copy link

leeksoup commented Mar 19, 2024

Description of the need

The "Convert line breaks into HTML" / filter_autop filter breaks text_summary() when combined with CKEditor. See #6423

It is also redundant in an Editor configuration, as is "Convert URLs into links."

Idk whether to consider this a bug report or a feature request, honestly.

Proposed solution

Disable the following filters in the default Filtered HTML and Full HTML configurations:

  • Convert URLs into links
  • Convert line breaks into HTML (i.e. <br> and <p>)

Alternatives that have been considered

Each site owner who uses trimmed / auto-generated summary with text_summary() can disable these filters.

Additional information

Edit: I picked out the format names from the documentation at https://docs.backdropcms.org/documentation/text-formats-editors-filters but it appears that that is out of date and needs to be updated wrt format names and settings. I will update issue backdrop-ops/docs.backdropcms.org#233 accordingly.

Draft of feature description for Press Release (1 paragraph at most)

Backdrop now disables redundant filters in the default Editor configurations.

@indigoxela
Copy link
Member

Idk whether to consider this a bug report or a feature request, honestly.

Neither me, honestly. 😉

FTR: the first thing I always turn off when configuring filters for a new project, are exactly these two.
We inherited that filter combo from Drupal, but with a WYSIWYG editor enabled (which we do by default on Basic format), these filters make no sense at all.

I'm voting for disabling filter_url and filter_autop in standard_install() (core/profiles/standard/standard.install) - for the Basic format.

leeksoup added a commit to leeksoup/backdrop that referenced this issue Mar 20, 2024
Removes the redundant and problematic filter_url and filter_autop from default formats that have the Text Editor enabled. (Only the Basic format.)

backdrop/backdrop-issues#6429
@leeksoup leeksoup changed the title Filtered / Full HTML formats should disable filter_autop Filtered / Basic HTML format should disable filter_autop Mar 20, 2024
@indigoxela
Copy link
Member

indigoxela commented Mar 22, 2024

@leeksoup many thanks for providing this pull request! 🙏

However, there's a little TODO left. One test (ViewsHandlerAreaTextTest) fails in all three PHP versions. It seems that this test relies on the filter being on. Probably unrelated, but still the test needs some update.
I'm guessing, the test expects some <br> being there, but there aren't. (But I didn't dig deeper, yet.)

@leeksoup
Copy link
Author

@indigoxela you're welcome. :)

Thanks for the explanation. I saw that some tests failed but had no idea what to do with that information.

@indigoxela
Copy link
Member

I saw that some tests failed but had no idea what to do with that information.

They need fixing. 😉

Two tests have failures, both because of expected <p>, which isn't there anymore, as the default format has the autop filter off.

  1. LocaleMultilingualFieldsFunctionalTest

The xpath search fails, as it currently seeks for the paragraph inside the div.
File: core/modules/locale/tests/locale.test

  1. ViewsHandlerAreaTextTest

The test actually compares the output of different filters, check_markup() uses the fallback format by default, but the test should actually correctly compare with the same format applied - default_format().
File: core/modules/views/tests/handlers/views_handler_area_text.test

@leeksoup this issue turns out to be a bit more of a deep-dive into our testing framework for you. 😉
Are you familiar with Simpletest and/or xpath? Are you willing to accept the challenge to fix the failing functional tests?
It's no witchcraft, but still a bit more, than you might have expected. 😉

@leeksoup
Copy link
Author

leeksoup commented Mar 23, 2024

They need fixing. 😉

I should have guessed that. 😏

Are you familiar with Simpletest and/or xpath? Are you willing to accept the challenge to fix the failing functional tests?
It's no witchcraft, but still a bit more, than you might have expected. 😉

No, I have no familiarity with those. Yes, more than I expected for sure. I don't think I can invest the time right now to figure it out and fix them. Sorry. 😢

@indigoxela
Copy link
Member

I don't think I can invest the time right now to figure it out and fix them.

That's OK, I already suspected, that this might be a little over the top. 😉

Still, many thanks for starting this topic by opening an issue. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants