Remove SafeMarkup::set in drupal_check_module()

Without this patch, an evil module could cause XSS errors.

With this patch, we're much better off. So this is not just a refactor, but also safer.

Very nice. Thank you for the manual testing.

I reviewed the code and everything looks above board;)

Nice cleanup with the placeholders!

Looking at this, I'm actually thinking it's wrong for the parentheses to be outside the translatable string. Punctuation is very much a language-specific thing.

I grepped to see if this exact string was translated elsewhere such that we'd be adding work for translators:

[mandelbrot:maintainer | Thu 20:32:10] $ grep -r "Currently using" *
core/includes/install.inc:          $message .= ' (' . t('Currently using !item !version', array('!item' => $requirement['title'], '!version' => $requirement['value'])) . ')';
core/modules/system/system.install:            'description' => t('@name requires this module and version. Currently using @required_name version @version', array('@name' => $name, '@required_name' => $required_name, '@version' => $version)),

Interesting and close (and no parentheses in that one...), but not the same thing, so it has to be translated separately anyway. So I think we should move the parentheses inside the t().

I looked in drupal_check_module() to see what $message was:

 $message = SafeMarkup::escape($requirement ['description']);

Can we nuke that now that we're using an @ placeholder? See in drupal_set_message(); it does its own escaping/safe check, so the code path where only $message is used is also still safe.

Finally, I asked myself if it would be appropriate to do one t('@module_requirement_message (Currently using @required_name version @version.)'), and I actually think that would be helpful context for translators, because I can imagine cases where it might subtly affect the translation. It's probably worth confirming that that is acceptable with a multilingual contributor, but as far as I can see it's not making anything worse, and it simplifies the whole thing and reduces double sanitization. (I would like feedback on this point though.)

Per #6 I've addressed the first 2 points, however waiting for feedback on the last point from a multilingual contributor.

1. Moved parentheses inside t()
2. Removed the SafeMarkup::escape from $message since drupal_set_message(); escapes this

Working on a patch for this with the last change, then the multilingual contributor can decide if it should be included or not, but you'll have the patch either way.

This patch should implement the suggestion in #6 point three.

Finally, I asked myself if it would be appropriate to do one t('@module_requirement_message (Currently using @required_name version @version.)'), and I actually think that would be helpful context for translators, because I can imagine cases where it might subtly affect the translation...

Thanks @crowdcg. I pinged @Gábor Hojtsy to ask him whether this is an appropriate use of t() or not.

Edit: Note that #10 has the parentheses twice now, so if #10 turns out to be the better choice than #12, we'll want to fix that. And also we'll want to be sure to manually test the before-and-after.

+++ b/core/includes/install.inc
@@ -980,14 +980,11 @@ function drupal_check_module($module) {
+          $message = t('@message (Currently using @item @version)', array('@message' => $requirement['description'], '@item' => $requirement['title'], '@version' => $requirement['value']));

I agree this gives better context to the generic (Currently using @item @version). I also agree maybe made more specific, eg. @requirements_message sounds fine.

@Gábor Hojtsy, great, thanks!

So let's use a more specific placeholder label and then this is probably ready.

Example steps to test:

1. Clean install of Drupal 8.
2. Adjust a module install to make its requirements be FALSE, for instance in aggregator.install change $has_curl = function_exists('curl_init'); to $has_curl = !function_exists('curl_init');
3. Now, attempt to install the Aggregator module and it will show the error as seen below.

I tried it also, and I dont see the double parens mentioned in #11.
And I agree, adding the word version to the message helps.

Thanks @Gábor Hojtsy and @xjm! I've added the more specific placeholder label as well as the word "version". I realized I hadn't included that in the previous patch and it definitely makes the message make more sense. Here is a screenshot with this patch applied. Also, updated the issue summary.

I checked the interdiff and the patch and it looks like this addresses all the concerns so far. rtbc.

Okay awesome! Yay for simpler code and better translator experience to boot.

This patch is a required part of a critical issue and so is allowed per https://www.drupal.org/core/beta-changes. Committed and pushed to 8.0.x. Thanks!

Removed unused use on commit:

diff --git a/core/includes/install.inc b/core/includes/install.inc
index a464762..31d472a 100644
--- a/core/includes/install.inc
+++ b/core/includes/install.inc
@@ -9,7 +9,6 @@
 use Symfony\Component\HttpFoundation\RedirectResponse;
 use Drupal\Component\Utility\Crypt;
 use Drupal\Component\Utility\OpCodeCache;
-use Drupal\Component\Utility\SafeMarkup;
 use Drupal\Component\Utility\UrlHelper;
 use Drupal\Core\Extension\ExtensionDiscovery;
 use Drupal\Core\Site\Settings;