Rename check_plain() to drupal_htmlspecialchars()

I don't think filter_plain makes what the function does any less confusing. check_plain doesn't filter anything; it converts plain text (x < 2) to HTML ( x < 2) by escaping certain characters.

I'd rather see something like:

check_plain: converts "plaintext" to HTML: plaintext_to_html()
check_markup: converts "richtext" to HTML: richtext_to_html()

(BTW: filter_xss operates on HTML and limits tags used to a whitelist. It also strips attributes that can cause an XSS attack.)

Unlike #455724: Deprecate check_markup(), I am rather opposed to this change. It's a change for consistency, but check_markup() and check_plain() are not at all comparable, and therefore, I rather think it's wrong to name them consistently.

Additionally, check_markup() is provided by and bound to filter module, and thereby using a wrong function namespace. check_plain(), however, lives in bootstrap.inc.

check_plain() is a rather simple, UTF-8-safe wrapper around htmlspecialchars(), so if there was a better name, then it would probably be drupal_htmlspecialchars() - following the pattern of many other wrappers around PHP functions in Drupal.

Not sure... it's the first time I thought about this function name, and changing

check_plain() => drupal_htmlspecialchars()

would most probably help many Drupal newcomers to understand why they want to use it. They would normally use htmlspecialchars(), because they are used to it from their own PHP code and other web applications. "check_plain" is very confusing for them (and I remember when I started with Drupal, was used to htmlspecialchars(), and found it hard to recall the purpose of check_plain()).

A change to drupal_htmlspecialchars() would nicely map to other utf-8-related Drupal wrappers:

drupal_strlen()
drupal_strtolower()
drupal_strtoupper()
drupal_substr()
drupal_ucfirst()

Ok, this makes sense. +1 for drupal_htmlspecialchars().

+++ includes/path.inc	23 Aug 2009 19:54:58 -0000
@@ -278,19 +278,19 @@ function drupal_get_title() {
- *   Optional flag - normally should be left as CHECK_PLAIN. Only set to
+ *   Optional flag - normally should be left as DRUPAL_HTMLSPECIALCHARS. Only set to
  *   PASS_THROUGH if you have already removed any possibly dangerous code
- *   from $title using a function like check_plain() or filter_xss(). With this
+ *   from $title using a function like drupal_htmlspecialchars() or filter_xss(). With this

Some PHPDoc is exceeding 80 chars due to the longer name now.

+++ includes/path.inc	23 Aug 2009 19:54:58 -0000
@@ -278,19 +278,19 @@ function drupal_get_title() {
-function drupal_set_title($title = NULL, $output = CHECK_PLAIN) {
+function drupal_set_title($title = NULL, $output = DRUPAL_HTMLSPECIALCHARS) {
   $stored_title = &drupal_static(__FUNCTION__);
 
   if (isset($title)) {
-    $stored_title = ($output == PASS_THROUGH) ? $title : check_plain($title);
+    $stored_title = ($output == PASS_THROUGH) ? $title : drupal_htmlspecialchars($title);
   }

Not sure about this change... The other define is PASS_THROUGH, which does not map to a function name, so I guess this wouldn't have to be that lengthy...?

That said, why on earth didn't we go with a simple Boolean here?

7 days to code freeze. Better review yourself.

I thin kthis would help *some* drupal newcomers who already know php. but others will immediately understand the association to plain text better than specialchars. i like the 'check' functions to all be in the same namespace. for me, filter_markup and filter_plain work.

I'm wondering how high the priority is on refactoring API calls at this point as we are looking to the finish line for a release of D7. Changing this is going to break a lot of contrib modules & site installation custom mods and I think we should making things as easy as possible for mod maintainers to get their modules ported to D7 as quickly as possible at this point. This is particularly so the first alpha has been released.

I'm in favor of refactoring the API, but I would vote for postponing it for D8. I think this is a useful conversation, but the lack of a clear-cut naming convention and no obvious consensus would suggest to me that more time to really think about it would be preferable. We don't want a situation where the naming of these function changes in D7, and then again in D8.

[edit] - check_markup() has already changed the function prototype between D6 and D7. However, the API on check_plain() has so far not changed.

No more non-critical API changes.

Since the patch is a little old, and rebasing from the point it was done requires many manual fixes, I tried to do it again.

Basically it's raw replacing and adding the function phpdoc description change manually. For reference:

git grep -l check_plain | xargs perl -pi -e 's/check_plain/drupal_htmlspecialchars/g'
git grep -l CHECK_PLAIN | xargs perl -pi -e 's/CHECK_PLAIN/DRUPAL_HTMLSPECIALCHARS/g'

Let see what testbot thinks.

PS: I guess the comments in 8 still applies, but let see if tests pass ok first.

Now with fixed comments.

BTW dreditor was a huge help to identify those lines in the patch :-)

Not sure about this change... The other define is PASS_THROUGH, which does not map to a function name, so I guess this wouldn't have to be that lengthy...?

That said, why on earth didn't we go with a simple Boolean here?

I have been looking at vcs history, and I end up in #242873: make drupal_set_title() use check_plain() by default., that's where that change got in, and it seems like the original patches were using boolean, but it's a long thread and I did not finish to read it.
In the other side, IMHO that deserves another issue.

So future newcomers to the issue know what it's being renamed *to*.

#17: 0001-Issue-557148-by-agentrickard-marvil07-Rename-check_p.patch queued for re-testing.

Trying again.

Uploading to review comments on dreditor

Re-rolling patch to work with HEAD as of now.

Please review.

wrote issue summary, removing tag.

Adding Issue summary

assigning to myself; writing API change notification

Change record added - API Change record

Updated issue summary.

does it really make sense to create a change record before the change is committed yet ?

#25: issue-557148-by-agentrickard-dellintosh-rename-checkplain-to-drupalhtmlspecialchars.patch queued for re-testing.

I've unpublished the change record.

updating issue description to include API Change record link

Superseded by #2302363: FormattableMarkup and HTML utility components replace check_plain(), format_string(), drupal_placeholder() (and SafeMarkup)