[meta] Replace calls to check_plain() with Drupal\Component\Utility\String::checkPlain()

It is a novice task.

Updated issue summary.

#2089471: Convert all calls to check_plain() in core to Drupal\Component\Utility\String::checkPlain() in core/modules/M-Z (except system/simpletest) needs non-novice review, see comment #15

We might need to split the module list into even smaller chunks or we'll risk chasing head forever. The two patches in the #2089471: Convert all calls to check_plain() in core to Drupal\Component\Utility\String::checkPlain() in core/modules/M-Z (except system/simpletest) and #2089465: Convert all calls to check_plain() in core to Drupal\Component\Utility\String::checkPlain() in core/modules/A-L get outdated pretty fast.

You're right. I'm also wary about splitting those up at this point. We should try and get both patches passed by the testbot first and see what happens next.

Updated issue summary.

Do we intend to remove check_plain before Drupal 8.0? It's listed in https://docs.google.com/spreadsheet/ccc?key=0AusehVccVSq2dEttQ3k5WWNYVHB... as staying around until Drupal 9, but I don't think that's correct.

#2089465: Convert all calls to check_plain() in core to Drupal\Component\Utility\String::checkPlain() in core/modules/A-L
#2089471: Convert all calls to check_plain() in core to Drupal\Component\Utility\String::checkPlain() in core/modules/M-Z (except system/simpletest)

Those two issues are too big to be done at once.
Seem that while patches will be created and reviewed they already will need a re-roll :)

For this moment I have created separated task for views/views_ui modules:
#2187829: Convert all calls to check_plain() in core to Drupal\Component\Utility\String::checkPlain() in core/modules/views

Slow down a sec, do we really need separate issues for each module? Views was a special case because it had a big patch on it's own, but I thought we were looking at 50k patches each for A-L and N-Z? That's much more manageable than a load of 1k patches.

Yes, please do not create separate issues on a per-module basis here. This is problematic issue scoping. While there's some value in trying a small change for someone's first patch, on the whole, the added overhead isn't appropriate, especially if the same person rolls more than one of the patches.

We have a draft post that attempts to explain how to address issues like these:
https://docs.google.com/a/acquia.com/document/d/1zYDnu45djDBcU87sNgKk15C...

If someone is tackling these issues at DrupalSouth, I'd appreciate it if someone onsite there could get everyone working on those issues together and take a look at that document. :)

Also, these patches would be better targeted for the disruptive patch window.

Finally, I rescoped this meta. We should leave check_plain() in as a BC wrapper for the full 8.x release. (This was discussed recently on a call with all four core maintainers.)

@thedavidmeister, maybe you can help out here by getting these patches all rolled into one (up to a patch size of 50-100K), reviewing with git diff --color-words or finding reviewers, and getting a list of everyone who contributed to all the sub-issues for the commit message credit? Thanks!

Edit: Also, see https://groups.drupal.org/node/394763 for more information on the post-alpha "disruptive patch window", which is when it's best to schedule scriptable patches like these with a core maintainer. That way, they can be rerolled and committed without having to chase HEAD forever.

We should leave check_plain() in as a BC wrapper for the full 8.x release due to its high usage. (This was discussed recently on a call with all four core maintainers.)

If that is really the case, then we can mark this issue won't fix (or move it to D9).

I vehemently disagree. There's no point in keeping @deprecated stuff around.

@thedavidmeister, apologies! I was just sharing the draft thoughts because we haven't made a post yet, and I want to make sure contributors don't sink time into rolling 900-byte patches that will be rejected. It was co-authored by @webchick and myself in response to a problem the core maintainers identified with a recent misunderstanding around meta issue management. I'll post a policy issue later and link here so we don't derail this issue.

@sun, in most cases I agree, but for a handful of very common functions it makes sense to keep in valid, tested one-line wrappers. But let's discuss further on the @deprecated meta. :)

Hey I created a script to automate this and here is the patch

My bad with that patch. A duplicate use Drupal\Component\Utility\String

Another fix to the patch

Cause HEAD is still moving, I'm gonna improve on this script and submit it here, so the change can be made to core directly.

We also need a commit message for this (#2089471: Convert all calls to check_plain() in core to Drupal\Component\Utility\String::checkPlain() in core/modules/M-Z (except system/simpletest) probably has most people, but there might be some who've only worked on the little issues).

Work in progress script that created this patch, https://gist.github.com/grom358/9106837

Manual fix to core/modules/field/lib/Drupal/field/Plugin/views/argument/ListString.php since it had already imported a String class from different namespace

Yet another attempt at scripting the patch.

As I mentioned in previous comment I used a script to generate the patch, which is here https://gist.github.com/grom358/9106837

Its very much a prototype, but worked well enough to apply this check_plain change. I ran the command
~/dev/drupal $ ~/code_upgrade.php 'Drupal\Component\Utility\String' 'StringUtil' 'check_plain' 'checkPlain'

It will sort the use declarations inserting the use declaration given by the first argument. If there a conflicting use declaration it will use an alias (second arg). The third argument is the function call to replace and the fourth argument is the method name on the class for the new function call.

+use Drupal\Component\Utility\String as StringUtil;

The coding standards specify that UtilityString would be the correct alias here.

The reason this was split up in the first place is that chasing HEAD across a large number of files is too difficult; we are already repeatedly rerolling some of the smaller patches spawned from this issue.

@longwave this is why I decided todo an automatic solution. Can easily re-execute the script to change the alias.

I'm personally happy to set up a time to commit all of these as one big patch in order to minimize bloat to the RTBC queue and endless re-rolling work. The best time is generally right after an alpha release, where we set aside "disruption days" specifically for this purpose. I don't think we have an alpha scheduled for March yet, but it's generally roughly the middle of the month. Can we please postpone this effort until then, so the core committers can stay focused on beta blockers and other bug fixes?

Do you mean the issue page, or are you reviewing the patch?

If you're talking about the issue page, that's something to possibly file a bug in the d7qa project, but it looks fine for me so you probably just need to do a shift-reload to re-fetch the broken CSS.

Postponing on #2208607: Write script to automate replacement of deprecated functions where possible

https://drupal.org/comment/8582935#comment-8582935

Patch created with function_replace.php at https://github.com/grom358/d8codetools

function_replace.php check_plain 'Drupal\Component\Utility\String' UtilityString checkPlain

Patch #76 applies cleanly, no check_plain() find when applied. Reviewed patch, it does not contain anything unexpected. No errors spotted after installation on screen or in log. Site seems to be working as before patching. Script used to generate the changes in the patch obviously works.

Plenty of check_plain still in the code base and not just comments :)

$role_options = array_map('check_plain', user_role_names());

// There's some overhead in calling check_plain() so only call it if the label

* check_plain() is not tested here.

$types = array_map('check_plain', node_type_get_names());

// Enable check_plain() for 'Basic HTML' text format.

 *     not be passed through check_plain().

    // Avoid running check_markup() or check_plain() on empty strings.

    // Default to a simple check_plain().

   * @see check_plain()

 *   this result must ensure that check_plain() is called on it before it is

 * check_plain() or filter_xss().

  // We do not want the l() function to check_plain() a second time.

    $roles = array_map('check_plain', user_role_names(TRUE));

      '#options' => array_map('check_plain', user_role_names(TRUE)),

    // with check_plain().

    $variables['site_name'] = check_plain($site_config->get('name'));

      $content = '<span class="label">' . check_plain($type->name) . '</span>';

      $content = '<span class="label">' . check_plain($type['title']) . '</span>';

And needs a reroll.

@alexpott working on handling the array_map calls.. in regards to the comments.. what do we want todo there? Change it to \Drupal\Component\Utility\String::checkPlain()

Updated as per alexpott review

+++ b/core/modules/user/user.module
@@ -569,7 +570,7 @@ function user_preprocess_block(&$variables) {
- *   this result must ensure that check_plain() is called on it before it is
+ *   this result must ensure that \Drupal\Component\Utility\String::checkPlain is called on it before it is

checkPlain should be followed by brackets even in comments: checkPlain().

user.module has a few comments that are now over 80 characters.

core/modules/system/templates/links.html.twig still has a reference to check_plain()

Other than that I would RTBC this, but I noticed that it is going to conflict with #2089433: Remove uses of deprecated XSS filter functions which has some manual changes so is harder to reroll. Let's get that in, then come back and reroll this.

This doesn't need to be postponed any more. @grom358 did you do any manual changes or is the command in #76 enough to reroll this?

The only manual changes I have been doing is wrapping the comments to 80 characters (but also have in pipeline to automate this).

delete me

Reroll PSR4

Looks good. All earlier comments addressed, applies without conflict and the only remaining mention of check_plain is the function itself.

reroll

Typo when rerolling. fixed.

simple reroll

Getting this in while it's hot!

Committed and pushed to 8.x. Thanks!