Instructions

This is part of #2093143: [meta] Remove calls to @deprecated and "backwards compatibility" procedural functions from core

Writing patches

We want to replace calls to check_plain(), and any references in the documentation, to Drupal\Component\Utility\String::checkPlain()

To do this, we don't want to have to write the whole "Drupal\Component\Utility\String::checkPlain()" every time so we want to use namespace aliases so that we can simply write String::checkPlain() - see the PHP documentation http://www.php.net/manual/en/language.namespaces.importing.php

We need to be sure that each file that needs to use String::checkPlain() has the statement use Drupal\Component\Utility\String; at the top of it.

Automatic patching

Instead of doing taking the manual approach I decided to create a tool d8codetools .

~/dev/drupal $ ~/d8codetools/function_replace.php 'check_plain' 'Drupal\Component\Utility\String' 'UtilityString' 'checkPlain'

Converting existing patches

IMPORTANT: Most of the work for this issue has already been done in #2089465: Convert all calls to check_plain() in core to Drupal\Component\Utility\String::checkPlain() in core/modules/A-L and #2089471: Convert all calls to check_plain() in core to Drupal\Component\Utility\String::checkPlain() in core/modules/M-Z (except system/simpletest) but because the patches touched basically all of core they needed to keep being re-written. For that reason we are splitting those issues into per-module issues but the code changes in the patches from the large issues should be re-cycled as much as possible.

Issues

Needs work:

- #2089351: Convert all calls to check_plain() in core to Drupal\Component\Utility\String::checkPlain() in core/includes

Postponed on "Disruptive Day":

- #2089471: Convert all calls to check_plain() in core to Drupal\Component\Utility\String::checkPlain() in core/modules/M-Z (except system/simpletest)
- #2196817: Convert all calls & docs references to check_plain() in core to Drupal\Component\Utility\String::checkPlain() in comment module
- #2196835: Convert all calls & docs references to check_plain() in core to Drupal\Component\Utility\String::checkPlain() in entity_reference module
- #2196839: Convert all calls & docs references to check_plain() in core to Drupal\Component\Utility\String::checkPlain() in field/field_ui module
- #2196841: Convert all calls & docs references to check_plain() in core to Drupal\Component\Utility\String::checkPlain() in file module
- #2196893: Convert all calls & docs references to check_plain() in core to Drupal\Component\Utility\String::checkPlain() in filter module

Done!

- #2187829: Convert all calls to check_plain() in core to Drupal\Component\Utility\String::checkPlain() in core/modules/views
- #2093167: Deprecate check_plain(), call \Drupal\Component\Utility\String::checkPlain() directly instead
- #2089461: Convert all calls to check_plain() in core to Drupal\Component\Utility\String::checkPlain() in core/lib
- #2089465: Convert all calls to check_plain() in core to Drupal\Component\Utility\String::checkPlain() in core/modules/A-L Assigned to: thedavidmeister
- #2196797: Convert all calls to check_plain() in core to Drupal\Component\Utility\String::checkPlain() in action module
- #2196801: Convert all calls to check_plain() in core to Drupal\Component\Utility\String::checkPlain() in aggregator module
- #2196805: Convert all calls to check_plain() in core to Drupal\Component\Utility\String::checkPlain() in ban module
- #2196807: Convert all calls to check_plain() in core to Drupal\Component\Utility\String::checkPlain() in block module
- #2196811: Convert all calls to check_plain() in core to Drupal\Component\Utility\String::checkPlain() in book module
- #2196815: Convert all calls & docs references to check_plain() in core to Drupal\Component\Utility\String::checkPlain() in color module
- #2196895: Convert all calls & docs references to check_plain() in core to Drupal\Component\Utility\String::checkPlain() in forum module
- #2196897: Convert all calls & docs references to check_plain() in core to Drupal\Component\Utility\String::checkPlain() in image module
- #2196901: Convert all calls & docs references to check_plain() in core to Drupal\Component\Utility\String::checkPlain() in language module
- #2196907: Convert all calls & docs references to check_plain() in core to Drupal\Component\Utility\String::checkPlain() in link module
- #2196909: Convert all calls & docs references to check_plain() in core to Drupal\Component\Utility\String::checkPlain() in node module
- #2196951: Convert all calls & docs references to check_plain() in core to Drupal\Component\Utility\String::checkPlain() in picture module
- #2196995: Convert all calls & docs references to check_plain() in core to Drupal\Component\Utility\String::checkPlain() in rest module
- #2196821: Convert all calls & docs references to check_plain() in core to Drupal\Component\Utility\String::checkPlain() in contact module
- #2196825: Convert all calls & docs references to check_plain() in core to Drupal\Component\Utility\String::checkPlain() in content_translation module
- #2196831: Convert all calls & docs references to check_plain() in core to Drupal\Component\Utility\String::checkPlain() in edit module

CommentFileSizeAuthor
#99 2089331-99-checkplain.patch99.91 KBgrom358
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,270 pass(es). View
#94 2089331-94-checkplain.patch99.87 KBgrom358
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2089331-94-checkplain.patch. Unable to apply patch. See the log in the details link for more information. View
#92 2089331-92-checkplain.patch99.87 KBgrom358
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,688 pass(es), 216 fail(s), and 15 exception(s). View
#88 2089331-88-checkplain.patch99.86 KBgrom358
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2089331-88-checkplain_0.patch. Unable to apply patch. See the log in the details link for more information. View
#87 2089331-87-checkplain.patch103.32 KBgrom358
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,397 pass(es). View
#81 2089331-81-checkplain.patch109.81 KBgrom358
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,415 pass(es). View
#76 2089331-76-checkplain.patch100.55 KBgrom358
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,331 pass(es). View
#59 2089331-6-checkplain.patch123.42 KBgrom358
PASSED: [[SimpleTest]]: [MySQL] 64,566 pass(es). View
#57 2089331-5-checkplain.patch154.74 KBgrom358
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/node/lib/Drupal/node/Plugin/views/argument/Type.php. View
#55 2089331-4-checkplain.patch154.75 KBgrom358
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/field/lib/Drupal/field/Plugin/views/argument/ListString.php. View
#50 2089331-3-checkplain.patch151.71 KBgrom358
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 2089331-3-checkplain.patch. Unable to apply patch. See the log in the details link for more information. View
#49 2089331-2-checkplain.patch151.94 KBgrom358
#47 2089331-1-checkplain.patch152.23 KBgrom358
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/entity_reference/lib/Drupal/entity_reference/ConfigurableEntityReferenceItem.php. View
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

thedavidmeister’s picture

Title: Convert all calls to check_plain() in core to Drupal\Component\Utility\String::checkPlain() » [meta] Convert all calls to check_plain() in core to Drupal\Component\Utility\String::checkPlain()

There are a *lot* of calls to check_plain() so I thought I'd make this a meta and we can split out a bunch of novice conversions.

thedavidmeister’s picture

Issue summary: View changes

Updated issue summary.

jibran’s picture

Issue tags: +Novice

It is a novice task.

jibran’s picture

Issue summary: View changes

Updated issue summary.

thedavidmeister’s picture

Issue summary: View changes

Updated issue summary.

thedavidmeister’s picture

Issue summary: View changes

Updated issue summary.

thedavidmeister’s picture

Issue summary: View changes

Updated issue summary.

thedavidmeister’s picture

Title: [meta] Convert all calls to check_plain() in core to Drupal\Component\Utility\String::checkPlain() » [meta] Remove & Deprecate check_plain(), use Drupal\Component\Utility\String::checkPlain()
thedavidmeister’s picture

Issue summary: View changes

Updated issue summary.

rpsu’s picture

netsensei’s picture

thedavidmeister’s picture

#5 - I'd be happy for that to happen, but whoever splits one issue into multiple should also be the same person to split the existing patches appropriately. Let's not throw out work that's already been done.

netsensei’s picture

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.

netsensei’s picture

Issue summary: View changes

Updated issue summary.

sun’s picture

Issue summary: View changes
Issue tags: +@deprecated
ianthomas_uk’s picture

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.

InternetDevels’s picture

thedavidmeister’s picture

I'll start splitting this per-module.

thedavidmeister’s picture

Issue summary: View changes
thedavidmeister’s picture

Issue summary: View changes
thedavidmeister’s picture

Issue summary: View changes
thedavidmeister’s picture

Issue summary: View changes
thedavidmeister’s picture

Issue summary: View changes
thedavidmeister’s picture

Issue summary: View changes
thedavidmeister’s picture

Issue summary: View changes
thedavidmeister’s picture

Issue summary: View changes
thedavidmeister’s picture

Issue summary: View changes
thedavidmeister’s picture

Issue summary: View changes
thedavidmeister’s picture

Issue summary: View changes
thedavidmeister’s picture

Issue summary: View changes
thedavidmeister’s picture

Issue summary: View changes
thedavidmeister’s picture

Issue summary: View changes
thedavidmeister’s picture

Issue summary: View changes
thedavidmeister’s picture

Issue summary: View changes
thedavidmeister’s picture

Issue summary: View changes
thedavidmeister’s picture

Issue summary: View changes
thedavidmeister’s picture

Issue summary: View changes
thedavidmeister’s picture

Issue summary: View changes
thedavidmeister’s picture

Issue summary: View changes
ianthomas_uk’s picture

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.

thedavidmeister’s picture

Issue summary: View changes
thedavidmeister’s picture

Issue summary: View changes
xjm’s picture

Title: [meta] Remove & Deprecate check_plain(), use Drupal\Component\Utility\String::checkPlain() » [meta] Deprecate check_plain(), use Drupal\Component\Utility\String::checkPlain()
Issue tags: +Will cause commit conflicts

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.)

xjm’s picture

@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.

sun’s picture

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’s picture

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.)

Last time i checked with the core maintainers it was deprecated and so should not be called internally by core, but the check_plain() function itself could be left in place, or moved to a deprecated module #2042165: Add a 'deprecated' module that includes deprecated functions only when enabled.

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. :)

I disagree with parts of the document, I can't comment on it, there's nothing open for discussion on d.o, it looks like the document is owned by a proprietary company... Is there somewhere this meta issue about scoping and what is "best for the community" can be discussed in an open way (not IRC please!).

xjm’s picture

@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.

xjm’s picture

@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. :)

thedavidmeister’s picture

@xjm - that would be great. I would certainly love to discuss this meta meta issue somewhere public, even if it's clearly just a draft. I agree that this is not a good place to have this discussion, but it's relevant to the immediate task at hand so I'll leave my 2c and copy and paste to the policy issue when it becomes available :)

I personally believe that keeping "scriptable" issues as atomic as possible may increase effort from core maintainers but decreases effort overall from the community as it leads to re-rolls of smaller, easier to understand patches. Creating "disruptive patch windows" then causing many (likely more complex) patches elsewhere on d.o to need re-rolls is just moving the problem to somewhere less immediately visible.

More importantly, from my perspective, is that enforcing minimum sizes on patches is a policy that is exclusive to some people in some situations who want to contribute but may only have time/skills to post/review small, atomic patches. Maybe they're in a social situation like a casual core sprint and just want to get something up, maybe they're busy and only have 15-30 minutes before their train comes and want to smash something small, maybe they just feel like doing something really tiny because that's what they're in the mood to do - "first timers" aren't the only people who benefit from being exposed to quick and easy patches.

I would value inclusiveness, linearly scaling difficulty of metas, minimising disruptiveness of individual commits and accessibility over minimising administrative overhead (it's not like we couldn't find a new core maintainer who just focuses on "atomics" if we thought there was value in it).

I like the idea proposed by @xjm in #37 of keeping the issues and patches atomic, then someone who is helping manage the meta issue makes a "commit plan" with a core maintainer that merges multiple patches together into a single commit *at commit time* - that sounds like a smart, non-arbitrary (like "50k patch size") balance between competing needs - the size/composition of the patches can be determined on a case by case basis. Any patches that need re-roll at commit time can simply be excluded from the roll-up and re-scheduled when the conflict has been resolved. I'd be much happier to arrange something like that than multiple re-rolls of one big patch with an unknown commit horizon and low chance of avoiding merge conflicts that i know will likely disrupt the whole RTBC queue when it finally lands.

thedavidmeister’s picture

Issue summary: View changes
thedavidmeister’s picture

Issue summary: View changes
thedavidmeister’s picture

Issue summary: View changes
thedavidmeister’s picture

Issue summary: View changes
grom358’s picture

Status: Active » Needs review
FileSize
152.23 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/entity_reference/lib/Drupal/entity_reference/ConfigurableEntityReferenceItem.php. View

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

Status: Needs review » Needs work

The last submitted patch, 47: 2089331-1-checkplain.patch, failed testing.

grom358’s picture

FileSize
151.94 KB

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

grom358’s picture

FileSize
151.71 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 2089331-3-checkplain.patch. Unable to apply patch. See the log in the details link for more information. View

Another fix to the patch

grom358’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 50: 2089331-3-checkplain.patch, failed testing.

grom358’s picture

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.

ianthomas_uk’s picture

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).

grom358’s picture

Status: Needs work » Needs review
FileSize
154.75 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/field/lib/Drupal/field/Plugin/views/argument/ListString.php. View

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

Status: Needs review » Needs work

The last submitted patch, 55: 2089331-4-checkplain.patch, failed testing.

grom358’s picture

Status: Needs work » Needs review
FileSize
154.74 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/node/lib/Drupal/node/Plugin/views/argument/Type.php. View

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

Status: Needs review » Needs work

The last submitted patch, 57: 2089331-5-checkplain.patch, failed testing.

grom358’s picture

Status: Needs work » Needs review
FileSize
123.42 KB
PASSED: [[SimpleTest]]: [MySQL] 64,566 pass(es). View

Yet another attempt at scripting the patch.

grom358’s picture

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.

grom358’s picture

Issue summary: View changes
longwave’s picture

Status: Needs review » Needs work
+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.

grom358’s picture

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

thedavidmeister’s picture

We should run the script when a Core Maintainer is actually ready to commit, and we should leave out any patches that have conflicts in them when the script is run.

That way the patches being committed are minimally disruptive and we don't have to guess some "golden patch size" ahead of time.

thedavidmeister’s picture

never mind my last comment, @catch has committed many of the smaller patches already :)

webchick’s picture

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?

thedavidmeister’s picture

Here's an update on where we're at for this. Basically, someone (I'm happy to do this) has to look at the issues that were recently marked "CNW" by @webchick and see which ones can be postponed until disruptive patch day (because they're essentially RTBC) and which ones do need a bit more work before they could be committed.

webchick
5:32 so basically we need:
5:32 1) A date for the next alpha
5:32 then
5:32 2) a comment in the issue saying "The next alpha is March Whatever"
5:32 and then probably a quick round of emails/IRC tells (I should be available pretty much whenever in March)
5:32 sine I'm not going to Szeged
5:33 and then 3) We wait until then, do the find/replace, and voila. :)
thedavidmeister
5:33 webchick: cool, in the meantime i'll set those issues as postponed, except for the ones that actually need work
webchick
5:33 okay
thedavidmeister
5:33 webchick: one or two of them aren't just "find and replace"
webchick
5:33 cool. if they're not, those are fine as separate issues.
5:34 i'm confused on why that would be though
thedavidmeister
5:35 webchick: https://drupal.org/node/2089351
Druplicon
5:35 https://drupal.org/node/2089351 => Convert all calls to check_plain() in core to Drupal\Component\Utility\String::checkPlain() in core/includes #2089351: Convert all calls to check_plain() in core to Drupal\Component\Utility\String::checkPlain() in core/includes => 15 comments, 3 IRC mentions
webchick
5:35 ah. includes vs. modules imght make sense to split out, indeed.
thedavidmeister
5:35 webchick: there's some logic in there that references 'check_plain' that needs a bit more investigation
webchick
5:36 hm. hopefully that has some test coverage which would be blowing up if it were not working
thedavidmeister
5:36 webchick: originally committed in 2008… who knows?
webchick
5:36 git blame would. :)
thedavidmeister
5:36 webchick: either way, legitimately needs a bit more than find and replace
5:36 webchick: so i'll leave that open
webchick
5:36 yep
thedavidmeister
5:37 webchick: so the goal is to be 100% that "find and replace" is true by the disruption date
webchick
5:37 cool, that seems reasonable if that's the only tricky spot
thedavidmeister
5:38 webchick: well, there were some other minor things, but i think most of them have been sussed already
5:39 webchick: i'll drop this plan into the issue thread
webchick
5:39 sounds good
thedavidmeister
5:39 webchick: then i'll review what we've got and what we need and take it from there
webchick
5:39 great!
thedavidmeister’s picture

Issue summary: View changes
thedavidmeister’s picture

Issue summary: View changes
thedavidmeister’s picture

Issue summary: View changes
ianthomas_uk’s picture

Title: [meta] Deprecate check_plain(), use Drupal\Component\Utility\String::checkPlain() » [meta] Replace calls to check_plain() with Drupal\Component\Utility\String::checkPlain()
thedavidmeister’s picture

Incidentally, am I the only one who sees this issue has severe layout/css issues?

ianthomas_uk’s picture

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.

ianthomas_uk’s picture

grom358’s picture

grom358’s picture

FileSize
100.55 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,331 pass(es). View

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

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

ianthomas_uk’s picture

Status: Postponed » Needs review
JiriK’s picture

Status: Needs review » Reviewed & tested by the community

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.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

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.

grom358’s picture

@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()

grom358’s picture

Status: Needs work » Needs review
FileSize
109.81 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,415 pass(es). View

Updated as per alexpott review

ianthomas_uk’s picture

Status: Needs review » Postponed
Issue tags: -Needs reroll
+++ 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.

ianthomas_uk’s picture

Status: Postponed » Needs work

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?

grom358’s picture

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

grom358’s picture

delete me

grom358’s picture

Issue summary: View changes
grom358’s picture

Assigned: Unassigned » grom358
Status: Needs work » Needs review
FileSize
103.32 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,397 pass(es). View
grom358’s picture

FileSize
99.86 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2089331-88-checkplain_0.patch. Unable to apply patch. See the log in the details link for more information. View

Reroll PSR4

ianthomas_uk’s picture

Status: Needs review » Reviewed & tested by the community

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

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 88: 2089331-88-checkplain.patch, failed testing.

Xano’s picture

Issue tags: +needs-reroll
grom358’s picture

Status: Needs work » Needs review
FileSize
99.87 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,688 pass(es), 216 fail(s), and 15 exception(s). View

reroll

Status: Needs review » Needs work

The last submitted patch, 92: 2089331-92-checkplain.patch, failed testing.

grom358’s picture

FileSize
99.87 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2089331-94-checkplain.patch. Unable to apply patch. See the log in the details link for more information. View

Typo when rerolling. fixed.

grom358’s picture

Status: Needs work » Needs review
grom358’s picture

Status: Needs review » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 94: 2089331-94-checkplain.patch, failed testing.

alansaviolobo’s picture

Issue tags: -needs-reroll +Needs reroll
grom358’s picture

Status: Needs work » Needs review
FileSize
99.91 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,270 pass(es). View
grom358’s picture

Status: Needs review » Reviewed & tested by the community

simple reroll

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Getting this in while it's hot!

Committed and pushed to 8.x. Thanks!

  • Commit 3a42e91 on 8.x by webchick:
    Issue #2089331 by grom358 | thedavidmeister: [meta] Replace calls to...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.