The filter "Convert line breaks into HTML (i.e. <br> and <p>)" turns line breaks inside <li> tags into unmatched closing </p> tags

peter törnstrand → made their first commit to this issue’s fork.

Was able to reproduce this with core 11.2.3. Fix attached.

Markup after fix applied:

<ul>
<li>Here is a list Item.</li>
<li><p>Here is another list item, this time containing a line break which is converted by the filter to a br tag.<br />
This is some more text followed by two line breaks, which the filter converts to a p tag.</p>
<p>And now some more text.</p></li>
</ul>

Don't see anything to review

Was previously tagged for summary update and tests too so those will need to be completed.

Sorry, forgot MR. Will have a look at creating a test. Question, can I include the test in the same MR?

I'm working on writing a test for this.

So this was a bit more complex than I imagined as the test requires a call to the global function _filter_autop(). I looked at the documentation at https://www.drupal.org/docs/automated-testing/phpunit-in-drupal/unit-tes... → but I am not sure I got this right. I tried looking at how this is solved in other core modules and found what I believe is relevant examples in system/tests/fixtures.

I also posted in the Drupal Slack channel #contribute but have not gotten any answers yet.

I did however have lots of fun doing this.

From @berdir in #contribute slack channel

we want to move all functions to classes anyway. with underscored ones, it's a bit unclear if we want to provide BC or not, but you could play it safe and add a deprecated message. and then depending on where it is used, move it to the class, or a trait or static method.

My gut feeling was to put _filter_autop() in the Drupal\filter\Plugin\Filter\FilterAutoP class .. perhaps directly in the ::process() method.

And then just add a deprecation note on _filter_autop() in filter.module:

@deprecated in drupal:11.x.x and is removed from drupal:12.x.x. Use Drupal\filter\Plugin\FilterAutoP::process() instead.

I did however have lots of fun doing this.

That's awesome!

But looking at the MR not sure we need fixtures but instead could use a data provider.

No hard rule of thumb but I typically only see fixtures used in migrations or update hooks

Yeah, I tried having the input/output directly in the test case but it was completely unreadable. Concatenated strings with lots of PHP_EOL. It was difficult to spot and fix errors caused by formatting.

I'm using a data provider but the provider reads the input/output from the fixture files.

Regarding the function _filter_autop() in a separate file; that was just something i noticed in the System module.

Should I move the input/output into the test case again?

And perhaps I should go with @berdir's input and move the _filter_autop() to the FilterAutoP plugin and deprecate the global function?

Think moving the function is out of scope of this issue.

There's already a kernel test checking this function testLineBreakFilter, would just look into expanding that test.

Yes. That is probably true. I will have a look at the test you mentioned.

Wish I had found that test before. Was a minor fix adding a test for this scenario. Well, I learned something at least.

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.