text_summary() ignores filter status

Yeah, things here are pretty broken.

In Drupal 8, at least, I think we can actually go further and get rid of these filter checks entirely. As far as I can tell, they serve no purpose... I just filed an issue at #1347920: text_summary() checks for specific filters, but it should be agnostic to filters with a patch for that. It turns out to be closely related to this issue.

+++ b/modules/field/modules/text/text.module
@@ -437,7 +437,7 @@ function text_summary($text, $format = NULL, $size = NULL) {
-  if (isset($filters['filter_htmlcorrector'])) {
+  if (isset($filters['filter_htmlcorrector']) && $filters['filter_htmlcorrector']->status) {

As per the explanation in #1347920: text_summary() checks for specific filters, but it should be agnostic to filters, I don't think we want this change, since text_summary() should fix its own breakages regardless of whether this filter was used prior to this function being called.

I'm tempted to mark this issue a duplicate of that one, and assume that a backport of that patch to leave the $format parameter in the signature, but not use it in the function body, is the correct D7 fix for this issue. Any objections?

Extracting the fix from #1868772-63: Convert filters to plugins

I reviewed this and everything in this patch is already included. Most things are implemented in the same way, some other minor things had extra checks.

So I think this can be closed, or does this issue exist to have a the backport?

This was committed as part of #1868772: Convert filters to plugins, moving back to D7

Issue #1868772: Convert filters to plugins does not seem to have tag for backporting & it is in closed status now.

Is it still valid to recreate the patch in comment #10 for d7?

Yes, I just ran into this bug on a 7.x site. Very annoying, it needs to be fixed.

Here's the actual patch to fix the bug in 7.x. I didn't have time to figure out all the test conversions from 8.x (or even sure if backports get updated tests). So if it "needs" tests for 7.x, someone else will have to do it.

I've used #14, and updated the test.
The main difference is that I think the _filter_htmlcorrector() function should always be run.
text_summary can break perfectly valid html, so it should fix it as well, regardless of the filter settings.
I'm trying to make the same point for d8 (https://www.drupal.org/node/1347920#comment-9652059) :)

#16 is out of scope for this issue. This issue is only to deal with what the issue title already states: "text_summary() ignores filter status".

The current version of text_summary always cleaned up the html (as it didn't check for the filter status).
Now we are checking for the status, it could potentially result in invalid html.
I'm not seeing how this could get committed without the htmlcorrector being run, so should we change the title / issue summary?

I understand that, but that IS the bug/issue title and it remains the same: if the htmlcorrector filter is disabled on a text format, it is still executed.

IIRC, this filter is enabled by default, however disabling it does absolutely nothing. If people don't want "invalid html" as you claim, then they should leave it enabled... obviously. There are use cases where htmlcorrector shouldn't be ran (i.e. when it's not being ran against HTML).

The tests will have to be altered for sure (as evidence by the tests failing in #14). It looks like the majority of the fix in #10 is there actually, but will need to be modified for 7.x.

There are use cases where htmlcorrector shouldn't be ran (i.e. when it's not being ran against HTML).

Good point.

If people don't want "invalid html" as you claim, then they should leave it enabled... obviously.

Although I agree with you, they should leave it enabled, I also agree with point 3 of this issue's summary (https://www.drupal.org/node/1347920): if text_summary breaks your html, it should fix it as well.

I would be perfectly happy if #14 gets in, as it's way better then what is currently running, but shouldn't we be worried about people's sites breaking (not having some closing tags, messing up the html structure) if they're using the summary of a field with a format without the html corrector filter?

shouldn't we be worried about people's sites breaking

I really don't see this being an issue. Most sites _should_ have the htmlcorrector (Correct faulty and chopped off HTML) filter enabled. If they don't, wouldn't that mean they actually don't want it in the first place? What is the point of having a filter you can "toggle" if it's always executed regardless? That make absolutely no sense.

if text_summary breaks your html, it should fix it as well

The reason I posted #14 above is because I had a real world use case where we were manually using the function (not via a field instance) to summarize some text and run it through special custom filters (so it didn't wrap the text in unnecessary <p> tags). These filters already handled the text as needed, but this is how I found out that htmlcorrector was always ran (even after I disabled it) because it was adding in the additional elements.

I think the "fix" is to introduce an update hook that loops through and enables this filter regardless if it was turned on/off. This way there is no actual "regression" since it was always ran before, but it can be disabled after the update if needed. I think this is a good compromise.

I think there is a difference between the format for the full textfield and what happens if you want to make a summary of it.
You generally set the format for the normal rendering of the field I'd say.

I found the issue in exactly the same use case as you did (except it is the new line filter that bugs me most :)).

Regarding the update hook fix: turning it on in all formats would 'correct' all the full versions of the text as well.. this is probably not what they wanted (as they turned it off :)). It wasn't always ran on the full text (right?!).

You are correct, update hook approach isn't right then. I forgot that the full text via check_markup() handles the filter status appropriately.

Again, generally speaking, the htmlcorrector filter _will/should_ be enabled by default on most text formats.

The fact that the filter statuses are completely ignored and alters the markup is still a bug, one that needs to get fixed. So, I'm not sure there is any "good" regression fix.