Rename "check_markup()"

check_plain - sanitizes text
check_markup - filters text, may sanitize, may not
filter_xss_admin - sanitizes text more liberally

I agree, this is a naming inconsistency and a DX issue that leads to a potential security issue.

Maybe check_markup could be changed to something about "running the filter" or filter_text or apply_filter.

As filtertext and filterformat are always needed both, why not make a richtext object offered by filter module with a method that outputs HTML?

I think in 7.x we are moving to "[verb]_[noun]" style, which is where I got my suggestions. filter_output is in that format as well, and does a good job of communicating the idea that Drupal filters on output, but we aren't really filtering "output" we're filtering input and making it output.

If we do this, then perhaps we should reevaluate filter_xss and any other helper functions (maybe there aren't any, but I don't know) as well.

check_plain converts plain text to HTML.
check_markup converts richtext (RT) to HTML.

filter_output doesn't tell me that either.

check_markup does a ton of other things as well.

Another related function worth reconsidering:

drupal_html_to_text

Heine pointed out in irc (and I somewhat agree) that check_markup will always return HTML. That's kind of true, though it's possible that a chunk of php would return an empty string or that an input format which has no filters assigned to it can return any text instead of HTML. To be really accurate the new name for check_markup should either be agnostic or we should change the way it works to always return HTML (I prefer the first solution).

I'd be happy with something like:

drupal_html_to_text -> leave alone
check_plain -> drupal_text_to_html

filter_xss, filter_xss_admin leave alone.
check_markup -> filter_text

This follows the logic that the functions with known output be called drupal_[something]_to_[otherthingthing] and that functions which do a more complex filtering or which have a specific filtering purpose are called filter_*

That's kind of true, though it's possible that a chunk of php would return an empty string or that an input format which has no filters assigned to it can return any text instead of HTML.

That's kind of beside the point as the output is slung into the content as if it _was_ HTML.

filter_text($text, $format) is really tidy.

And we have 'text formats' now instead of input formats so it's pretty self explanatory there as well.

Good.

This API change would be pretty nonsense on its own, but luckily we can accompany this change with some real value: #446518: Remove $check argument from check_markup()

Most probably, however, this means that we need to merge this patch into the other issue.

Check_plain doesn't check if something is plain - it converts it.

And?

(Somewhat off-topic: newer versions of PHP sports some filter mechanisms. I haven't looked at them recently, but I wonder if it makes sense to bring ours closer to PHP's? It could help developers.)

Semantically, I suppose I'd like to see some consistent use of filter_[name] where the second element indicates what will be output. That might give us:

filter_text() --> check_plain()
filter_html() --> check_markup()
filter_xss_safe() --> filter_xss() and filter_xss_admin()

And so forth. Not sure about the last function, since it effectively returns HTML.

to me, filter_X implies that I want the input to be X (filter text means I want to do some filtering on a piece of text, not that I want to output text).

/me almost concedes the point.

PHP uses filter_* for these functions -- http://us.php.net/manual/en/book.filter.php -- which effectively negates dmitri's argument.

If we did not have Drupal-specific functions for this, we would use something like:

// Print a user name securely.
global $user;
print filter_var($user->name, FILTER_SANITIZE_STRING);

See -- http://us.php.net/manual/en/filter.filters.sanitize.php -- for the sanitize tokens.

Would it make sense to use secure_ as the function namespace?

e.g.

secure_plain()
secure_html()
secure_xss()

-or-

secure_output_plain()
secure_output_html()
secure_prevent_xss()

This namespace is not used by PHP or Drupal ATM.

The other option, it seems to me is to use the new PHP 5 functions and dustbin our old Drupal-specific ones.

Mixing check_plain() in here and trying to unify these function names is wrong. check_plain() and check_markup() are completely different functions. The resulting content of a variable processed by check_markup() can be anything but not necessarily safe for HTML. What you really mean are the filter_xss*() functions, which are about to be moved out of filter.module: #470632: Move filter_xss*() into common.inc

That's why I agree that check_markup() is poorly named and should using a prefix of filter_ instead. It applies a configurable set of filters - or none at all - to some arbitrary input. The input - as well as the output - can be plain text, ASCII, raw HTML, safe HTML, XML, PHP, BBCode or whatever. The result can be safe HTML, but can also be the original, unaltered input. There are modules that use filters to not output HTML, but plain-text that is safe for sending via e-mail.

@sun -- cross post while I was gathering info. Does #22 change anything for you?

Nope.

1) Renaming of filter_xss*() as well as check_plain(), check_url(), check_file(), valid_email_address(), valid_url(), and possible more should happen in a separate issue. Those functions validate and/or sanitize (ensure) a certain syntax or format. check_markup() has nothing to do with them.

2) Which also means that we'll simply move the filter_xss*() functions in #470632: Move filter_xss*() into common.inc without renaming, so the renaming may or may not happen later on.

3) This issue is solely about renaming check_markup() in filter.module.

I am in agreement with sun.

On this patch, I like filter_text, so this is a vote for RTBC.

Other issues moved to #471184: Reconcile Drupal's input security functions with PHP filter_*, including the renaming of other functions and possibly replacing this function with filter_var.

Renaming is now: #471264: Consistently name validation/sanitation functions

No - I repeat: check_markup() is not a validation/sanitation function. It is a tool to apply a set of configurable filters to a string.

check_plain is also not just a validation/sanitation function. It is a tool to convert plain text strings (Ladies & Gentlemen) to HTML (Ladies & Gentlemen).

While not apparant in the PHPDoc for check_markup, its intended output is HTML. See Handle text in a secure fashion. I consider it a hack that some modules use this in another way.

Instead of renaming check_markup to a very ambiguous filter_text, it is time for a good look at how Drupal handles different string types, how to make the relation between function and the conversion being done can be made obvious. and how this all can be made easier for developers to prevent foe #1: XSS.

as per http://acko.net/blog/safe-string-theory-for-the-web what about filter($context, $string, $param) which would do everything?

We are also moving away from $context style functions (replace $context with $op to get a clear idea). So having filter('email', $input_string, $param); seems like a step backwards to me.

Just some more brainstorming on this... we also have db_escape_table and db_escape_string that will sanitize strings for use in different contexts.

Regardless of whether the $context is in the function name or the first argument to the function, there is a potential solidify a whole lot of different "escape strings for use in context X" functions.

Good catches! However, those should go into #471264: Consistently name validation/sanitation functions.

Even if the purpose of check_markup() was (or is) to produce HTML, it still applies a configurable set of filters to the value (the input format). The major difference to all other functions is that a developer cannot rely on an expected, validated and sanitized result in a certain syntax or format.

Let's keep this issue on focus. I would be very happy to discuss potential ideas for changes to check_markup() in a separate issue (please leave a pointer here).

Even if the purpose of check_markup() was (or is) to produce HTML, it still applies a configurable set of filters to the value (the input format).

Right, it converts richtext (with an associated format) to HTML.

The major difference to all other functions is that a developer cannot rely on an expected, validated and sanitized result in a certain syntax or format.

Nor does the developer have to.

Changing check_markup to filter_text doesn't add a thing to understanding its function. And change for the sake of change is IMO never a good argument for an API change.

That's why I referred to #446518: Remove $check argument from check_markup() in #15.

If we can find a better name than filter_text(), sure. However, putting the function into the namespace of filter.module makes a lot sense to me.

Well, thinking about this, I would call it filter_output(), since it is designed to sanitize data before outputting markup.

Bumping to critical, since #446518: Remove $check argument from check_markup() has been committed.

We need a new function name for check_markup() ASAP now.

Now is not the time to introduce new OO, IMHO, so I would rule #2 out.

So let's just check for namespace collisions on filter_foo() --> http://us2.php.net/manual/en/book.filter.php

filter_markup() is fine by me, and legal. Patch attached.

See also #557148: Rename check_plain() to drupal_htmlspecialchars() for discussion of check_plain() and a patch.

I thought about this and I think that the proposed change is very good, because

- "markup" in filter_markup() can mean both input and output, which is actually the case for this function. You can filter text to HTML, other markup languages to HTML, and HTML to HTML; and even if it was not intended to allow for it, it also allows to filter HTML to text.

- filter_markup() is in the namespace of the implementing module.

- filter_markup() is both understandable by existing developers and new developers, while it's an easily recallable API change for existing developers.

I'm with Heine on the richtext_to_html() proposal (http://drupal.org/node/557148#comment-1959718).

But the function is _in_ filter module.

So namespaces without filter_ are no good.

This is a horrible bikeshed debate, IMO. filter_markup is a perfectly good incremental improvement. If you want a name that doesn't start with filter_, then you have to move the function to common.inc.

ok, seems like we need a bit more discussion.

richtext_to_html() is wrong, because

- it is not guaranteed that it returns HTML; whether it returns HTML or not depends on the enabled filters in a format

- you can pass plain text, HTML, or other markup languages in; whether the input is properly converted into the intended output language depends on the enabled filters in a format

- the function is, unlike all other filter_* and check_* functions, a highly configurable converter that applies multiple string/text processors to the input; only each (enabled) text processor is supposed to return some expected result, while not all processors need to be enabled.

- it's not in the namespace of filter.module

I like filter_markup().

That's 3 votes for filter_markup().

Any other filter_* suggestions?

This is not a bikeshed debate. Proper text handling appears to be one of the things most taxing on the Drupal developer's brain. Just count the XSS issues reported in the last two years.

I'm disappointed by the focus on the namespace and the imo callous disregard for central role of the check_* and filter_* functions in string handling. Getting these function names right could minimize developer confusion and prevent countless XSS issues in the coming years.

Even in this issue I see confusion about what check_markup does: check_markup takes richtext (an 'arbitrary' format defined by the associated INPUT FORMAT) and converts it to HTML. That the resulting string doesn't have to contain a single tag doesn't mean the resultant string type isn't HTML; the function implicitly returns a string that is destined to be used as HTML. If check_markup did not return HTML, how could we show nodes and comments to visitors?

(BTW Filter use by the messaging module is a hack; the module will be rewritten.)

You seem to insist on the mysterious assumption that check_markup() always returns HTML that is safe for output.

Please have one more look at http://api.drupal.org/api/function/check_markup/7 and tell me where filter_xss() or any other sanitation function is invoked there. It is not.

check_markup() applies an arbitrary set of text processors to some string. That set can contain some processors or can be empty. Whatever string or formatted text you pass in may or may not be converted to something else.

Sanitation only happens if at least one security filter is enabled in the given text format, for example "HTML filter" (but there are more advanced security filters in contrib).

check_markup() is able to process many markup languages, especially lightweight markup languages, such as Markdown or BBCode. It is processing markup and is supposed to return HTML. The result may or may not be sanitized, depending on whether any security filter is additionally enabled in the given format.

For that sake, the "check" in check_markup() referred to the user access check for the given format that was previously contained. The $check parameter is gone now, so all what this function does now is to apply arbitrary filters to some potentially formatted text in some markup language and potentially convert that to another markup language (HTML is supposed). Therefore, filter_markup().

sun made the argument much better than I ever could. This is RTBC still.

You seem to insist on the mysterious assumption that check_markup() always returns HTML that is safe for output.

Did I?

That is absolutely NOT my "assumption". Check_markup returns HTML. That's it. Whether or not it is "safe" is immaterial and something for filter_access to resolve.

check_markup() is able to process many markup languages, especially lightweight markup languages, such as Markdown or BBCode. It is processing markup and is supposed to return HTML. The result may or may not be sanitized, depending on whether any security filter is additionally enabled in the given format.

Excellent so we agree. check_markup processes a markup language defined by the input format to HTML.

For that sake, the "check" in check_markup() referred to the user access check for the given format that was previously contained. The $check parameter is gone now, so all what this function does now is to apply arbitrary filters to some potentially formatted text in some markup language and potentially convert that to another markup language (HTML is supposed). Therefore, filter_markup().

I didn't think I pledged for keeping the check_* function names. Those are pretty lame. filter_markup IMO sucks just as bad.

Is there a filter_* named function you thinks sucks less?

On this question:

Is there a filter_* named function you thinks sucks less?

I can only answer with a quote from my earlier post (emphasis added):

I'm disappointed by the focus on the namespace and the imo callous disregard for central role of the check_* and filter_* functions in string handling. Getting these function names right could minimize developer confusion and prevent countless XSS issues in the coming years.

Personally, I still like one of the following:

filter_output
filter_markup
drupal_filter
drupal_sanitize
drupal_secure_text
drupal_secure_output
drupal_filter_markup
filter_markup

Any chance we can pick something and move forward? And if the namespace is not filter_*, do we have to move the function to common.inc?

I personally do not like *_secure_* *_sanitize because the output may not be secure at all.

We can also specify in the PHPDoc that the output will be valid (perhaps insecure) HTML format (even if no tags are present in the output). Security depends on the actual filters that were applied based on access permissions.

The documentation says "Run all the enabled filters on a piece of text."

What about drupal_apply_filters(), apply_filters() or filters_apply()?

This is very technical name and refer to the implementation details. It performs its job by applying the necessary filters.

As mentioned in the original description, this is a very high-level functions.
Why do we need to apply all the filters anyway? To prepare the input and making it ready to display/output. But why? What is the ultimate goal from a user's perspective? Remove this function and the input will not be formatted the way you intended. It seems to me this is more about formatting than filtering. Filtering is only a mean to an end.

format_output()? format_input()?

There are other format_* function in common.inc which takes an input and sometimes some formatting option and return an altered input based on the formatting options. Surprisingly, they (almost) all take the $langcode as well.

Just my 2 cents.

D8.

No such thing as a critical task.

How about "filter_apply" ?

Even "major" tasks are now preventing development of new features per the new policy at http://drupal.org/node/1201874. This would be a nice change, but largely just to fulfill OCD naming conventions. Considering we don't even have a consensus here, this shouldn't hold up development of new features.

just to fulfill OCD naming conventions.

I disagree, it's about a consistent API being easier to understand when first encountered which makes it easier to apply _safely_.

That said, I'm fine with the new priority.

I think we could add the newly named function in a minor release and leave check_markup() as a deprecated wrapper if we wanted to.

We've halved the calls to check_markup() in core since 7.x - 16 down to 9 https://api.drupal.org/api/drupal/core%21modules%21filter%21filter.modul...

So contrib interaction with this should be minimal now.

I think no longer postponed.

Very unlikely we'd rename this to another procedural function now, but the problem is still there 13 years later.

Reading back through some of the terminology discussions, a big problem is that filter module is called filter module. Filter module now provides 'text formats' which are configured collections of 'filter plugins', so it should be called 'format' module or 'text_format' module. You could then have a service like format.formatter or similar with a ::formatMarkup() or ::applyFormat() method.

a big problem is that filter module is called filter module

Amen!

And a related big problem is: #3231354: [PP-1] [META] Discuss: merge the Editor config entity into the FilterFormat config entity.

IMHO we should finally do that, and merge filter and editor module into text_format module. Neither https://www.drupal.org/project/format nor https://www.drupal.org/project/text_format are taken. Perhaps I should pre-emptively register both? 🤓