Follow-up to #2296929: Remove system_requirements() SafeMarkup::set() use

Problem/Motivation

In system_requirements(), we mark the phpversion as safe becuase it includes a link. This link is put in the 'value' field, but the purpose of the 'value' is for things like versions and amounts, see docs. The link should not be in the 'value' field, and the SafeMarkup::set should be removed.

This is related to #2549803: hook_requirements() 'value' key is confusing and used inconsistently, and 'title' is only displayed during installation if the value is also set.

Proposed resolution

Put the link in the 'description' field and use a render array to handle the link. See patch in #28
OR
leave link in the value, fix that in #2549803: hook_requirements() 'value' key is confusing and used inconsistently, and 'title' is only displayed during installation if the value is also set, and use a placeholders in t instead of concatenation and ::set() See patch in #37

Remaining tasks

User interface changes

Manual testing steps

*for install*

  1. chmod a-wx sites/default
  2. hack requirement condition code around the message want to test, to make it show
  3. install

*for /admin/reports/status *

  1. install
  2. hack requirement condition code around the message want to test, to make it show
  3. go to see /admin/reports/status

API changes

CommentFileSizeAuthor
#39 t-patch-nophpinfo-mysql.png147.61 KBYesCT
#39 head-nophpinfo-mysql.png137.62 KBYesCT
#39 t-patch-nophpinfo.png164.13 KBYesCT
#39 head-nophpinfo.png159.96 KBYesCT
#37 2551725.37.patch884 bytesYesCT
#37 t-patch-install-minversion.png56.82 KBYesCT
#37 t-patch-install-phpinfonot.png74.49 KBYesCT
#37 t-patch-install-mysql.png66.31 KBYesCT
#37 t-patch-mysql.png55.29 KBYesCT
#37 t-patch-phpinfo-no.png48.27 KBYesCT
#36 phpinfo-mysql-patch.png165.41 KBYesCT
#36 phpinfo-mysql-head.png147.02 KBYesCT
#32 2551725.32.patch2.13 KBYesCT
#29 Status_report___Site-Install.png7.08 KBjoelpittet
#28 interdiff.2551725.25.27.txt2.09 KBYesCT
#28 2551725.27.patch3.6 KBYesCT
#26 info-moreinfo-patch.png118.31 KBYesCT
#26 status-moreinfo-head.png164.3 KBYesCT
#25 info-mysql-patch2.png64.33 KBYesCT
#25 info-phpinfo-patch2.png74.21 KBYesCT
#25 interdiff.2551725.21.25.txt1.59 KBYesCT
#25 2551725.25.patch3.61 KBYesCT
#24 info-msql-patch.png71.38 KBYesCT
#24 info-mysql-head.png65.42 KBYesCT
#23 error-version-patch.png46.11 KBYesCT
#23 error-version-head.png47.57 KBYesCT
#22 info-phpinfo-patch.png146.11 KBYesCT
#22 info-phpinfo.png133 KBYesCT
#16 interdiff-4-16.txt827 bytesdorficus
#16 2551725-16.patch3.66 KBdorficus
#11 interdiff-8-11.txt627 bytesdorficus
#11 2551725-11.patch3.65 KBdorficus
#8 interdiff-4-8.txt3.66 KBdorficus
#8 2551725-8.patch3.65 KBdorficus
#4 Screen Shot 2015-08-14 at 6.02.23 PM.png35.84 KBjosephdpurcell
#4 Screen Shot 2015-08-14 at 6.02.12 PM.png26.73 KBjosephdpurcell
#4 Screen Shot 2015-08-14 at 6.02.01 PM.png47.2 KBjosephdpurcell
#4 Screen Shot 2015-08-14 at 6.01.50 PM.png39.01 KBjosephdpurcell
#4 Screen Shot 2015-08-14 at 6.01.36 PM.png30.59 KBjosephdpurcell
#4 Screen Shot 2015-08-14 at 6.01.28 PM.png18.4 KBjosephdpurcell
#4 Screen Shot 2015-08-14 at 11.36.29 AM.png68.4 KBjosephdpurcell
#4 Screen Shot 2015-08-14 at 11.36.16 AM.png59.59 KBjosephdpurcell
#4 Screen Shot 2015-08-14 at 11.36.05 AM.png47.67 KBjosephdpurcell
#4 Screen Shot 2015-08-14 at 11.35.52 AM.png44.04 KBjosephdpurcell
#4 Screen Shot 2015-08-14 at 11.35.40 AM.png36.03 KBjosephdpurcell
#4 Screen Shot 2015-08-14 at 11.35.23 AM.png24.06 KBjosephdpurcell
#4 2551725-4.patch3.66 KBjosephdpurcell
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

josephdpurcell created an issue. See original summary.

josephdpurcell’s picture

Status: Needs review » Active
josephdpurcell’s picture

Title: Address system_requirements() link to phpinfo » Remove system_requirements() SafeMarkup::set() use
Issue summary: View changes
Parent issue: » #2280965: [meta] Remove every SafeMarkup::set() call
josephdpurcell’s picture

Status: Needs review » Needs work

The last submitted patch, 4: 2551725-4.patch, failed testing.

xjm’s picture

Title: Remove system_requirements() SafeMarkup::set() use » Remove system_requirements() SafeMarkup::set() use with 'value' key
dorficus’s picture

+++ b/core/modules/system/system.install
@@ -133,29 +133,29 @@ function system_requirements($phase) {
+    $requirements['php']['description'][] = [
+      '#markup' => t('For more information, view the <a href="@link">phpinfo</a>.', ['@link' => (new Url('system.php'))->toString()]),
+    ];

This snippet, specifically the new Url call, is causing the fail because the route to system.php is not yet initiallized. Is there a way to get to this link without needing the route initialized?

dorficus’s picture

For the time being, and i'm completely open to suggestions on how to fix this, I changed the Url call to a relative url. This may not be the optimal fix, but it corrected the error and provided the correct link.

UPDATE: ignore this interdiff. it's wrong.

dorficus’s picture

Status: Needs work » Needs review
dorficus’s picture

Status: Needs review » Needs work

Noticed an error. will fix.

dorficus’s picture

Status: Needs work » Needs review
FileSize
3.65 KB
627 bytes

The last submitted patch, 8: 2551725-8.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 11: 2551725-11.patch, failed testing.

YesCT’s picture

in head

  // Test PHP version and show link to phpinfo() if it's available
  $phpversion = $phpversion_label = phpversion();
  if (function_exists('phpinfo')) {
    // $phpversion is safe and output of l() is safe, so this value is safe.
    if ($phase === 'runtime') {
      $phpversion_label = SafeMarkup::set($phpversion . ' (' . \Drupal::l(t('more information'), new Url('system.php')) . ')');
    }
    $requirements['php'] = array(
      'title' => t('PHP'),
      'value' => $phpversion_label,
    );
  }
  else {
    $requirements['php'] = array(
      'title' => t('PHP'),
      'value' => $phpversion_label,
      'description' => t('The phpinfo() function has been disabled for security reasons. To see your server\'s phpinfo() information, change your PHP settings or contact your server administrator. For more information, <a href="@phpinfo">Enabling and disabling phpinfo()</a> handbook page.', array('@phpinfo' => 'https://www.drupal.org/node/243993')),
      'severity' => REQUIREMENT_INFO,
    );
  }

a php key for requirements is set up.
to do something about phpinfo.
if runtime it looks like it tries to make a link to phpinfo
and if not runtime ... no link is made

and the else for phpinfo existing, happens no matter runtime or not. but, at runtime, if you dont have phpinfo, you wont see this requirements warning ...
if you also have a slightly older version of php
cause
later in the code

  // Suggest to update to at least 5.5.21 or 5.6.5 for disabling multiple
  // statements.
  if (($phase === 'install' || \Drupal::database()->driver() === 'mysql') && !SystemRequirements::phpVersionWithPdoDisallowMultipleStatements($phpversion)) {
    $requirements['php'] = array(
      'title' => t('PHP (multiple statement disabling)'),
      'value' => $phpversion_label,
      'description' => t('PHP versions higher than 5.6.5 or 5.5.21 provide built-in SQL injection protection for mysql databases. It is recommended to update.'),
      'severity' => REQUIREMENT_INFO,
    );
  }

overwrites that same requirements php key.

I'm not sure head is behaving the way we intend it to.

maybe they should have two different requirement keys?

it seems like the logic in this could be improved. and might be out of scope for this issue...
need to look into this a bit more.

YesCT’s picture

ok.

I'll open up a separate issue about the different messages using the same requirements key (and thus all that could be relevant are not all shown).

for this issue,

@dorficus and I worked through this and found that the patch in #4 took out the if runtime conditional, and we need to keep that. they are making a new patch doing that.

make each of the requirement messages show on the installer requirements screen (renaming settings file is a good trick for making install not work) and/or the admin/reports/status page (after successful install)
and for each, in each place, make a screenshot and get the markup without the patch and with the patch

in this issue we want to see no difference between head and with the patch

dorficus’s picture

Rolled back to #4 as a base since I went about fixing this the completely wrong way. I fixed the missing "if" statement that was keeping the install from happening.

dorficus’s picture

Status: Needs work » Needs review
YesCT’s picture

Issue tags: +Needs manual testing

opened #2552061: system_requirements() PDO warning overwrites other PHP messages

that interdiff and patch look great!

just manual testing to do.

stefan.r’s picture

Issue summary: View changes

Maybe some before and after screenshots? Also the wording on the description feels a bit wonky ("For more information, view the phpinfo")

joelpittet’s picture

@stefan.r what do you think about this?

("For more information view the PHP configuration.")

YesCT’s picture

doing the manual testing.

also, I think it is a bug that we cannot see the requirements page if it only contains an error and also warnings, but the requirements page is skipped in the installer if there are only warnings. so... here is an issue: #2552091: information from install requirements cannot be seen

[edit: ah, the messages are actually info, not warnings. but note the
'#markup' => t('PHP versions higher than 5.6.5 or 5.5.21 provide built-in SQL injection protection for mysql databases. It is recommended to update.'),
is an info... might be worth git blaming that to see if it was discussed on the issue if that should be an info or a warning.

YesCT’s picture

Issue summary: View changes
FileSize
133 KB
146.11 KB

here is one of the messages (will do rest next)

phpinfo

diff --git a/core/modules/system/system.install b/core/modules/system/system.install
index f38af8a..9178e5a 100644
--- a/core/modules/system/system.install
+++ b/core/modules/system/system.install
@@ -135,7 +135,7 @@ function system_requirements($phase) {

   // Test PHP version and show link to phpinfo() if it's available
   $phpversion = $phpversion_label = phpversion();
-  if (function_exists('phpinfo')) {
+  if (!function_exists('phpinfo')) {
     // $phpversion is safe and output of l() is safe, so this value is safe.
     if ($phase === 'runtime') {
       $phpversion_label = SafeMarkup::set($phpversion . ' (' . \Drupal::l(t('more information'), new Url('system.php')) . ')');

head:

with patch:

---------

markup:

head:

            <tr class="system-status-report__entry color-info">
            <td class="system-status-report__status-icon system-status-report__status-icon--info">
          <div title="Info">
            <span class="visually-hidden">Info</span>
          </div>
        </td>
        <td class="system-status-report__status-title">PHP</td>
        <td>
          5.5.23
                      <div class="description">The phpinfo() function has been disabled for security reasons. To see your server's phpinfo() information, change your PHP settings or contact your server administrator. For more information, <a href="https://www.drupal.org/node/243993">Enabling and disabling phpinfo()</a> handbook page.</div>
                  </td>
      </tr>
 

with patch:

            <tr class="system-status-report__entry color-info">
            <td class="system-status-report__status-icon system-status-report__status-icon--info">
          <div title="Info">
            <span class="visually-hidden">Info</span>
          </div>
        </td>
        <td class="system-status-report__status-title">PHP</td>
        <td>
          5.5.23
                      <div class="description">The phpinfo() function has been disabled for security reasons. To see your server's phpinfo() information, change your PHP settings or contact your server administrator. For more information, <a href="https://www.drupal.org/node/243993">Enabling and disabling phpinfo()</a> handbook page.</div>
                  </td>
      </tr>
YesCT’s picture

Issue summary: View changes
Status: Needs review » Needs work
FileSize
47.57 KB
46.11 KB

php not minimum version

diff --git a/core/modules/system/system.install b/core/modules/system/system.install
index c33ffb6..1a24d93 100644
--- a/core/modules/system/system.install
+++ b/core/modules/system/system.install
@@ -153,7 +153,7 @@ function system_requirements($phase) {
     ];
   }

-  if (version_compare($phpversion, DRUPAL_MINIMUM_PHP) < 0) {
+  if (!(version_compare($phpversion, DRUPAL_MINIMUM_PHP) < 0)) {
     $requirements['php']['description'][] = [
       '#prefix' => '<br />',
       '#markup' => t('Your PHP installation is too old. Drupal requires at least PHP %version.', ['%version' => DRUPAL_MINIMUM_PHP]),

head

patch

-------

markup

head

            <tr class="system-status-report__entry color-error">
            <td class="system-status-report__status-icon system-status-report__status-icon--error">
          <div title="Error">
            <span class="visually-hidden">Error</span>
          </div>
        </td>
        <td class="system-status-report__status-title">PHP</td>
        <td>
          5.5.23
                      <div class="description">Your PHP installation is too old. Drupal requires at least PHP <em class="placeholder">5.5.9</em>.</div>
                  </td>
      </tr>

patch

            <tr class="system-status-report__entry color-error">
            <td class="system-status-report__status-icon system-status-report__status-icon--error">
          <div title="Error">
            <span class="visually-hidden">Error</span>
          </div>
        </td>
        <td class="system-status-report__status-title">PHP</td>
        <td>
          5.5.23
                      <div class="description"><br />Your PHP installation is too old. Drupal requires at least PHP <em class="placeholder">5.5.9</em>.</div>
                  </td>
      </tr>

-----------------

+++ b/core/modules/system/system.install
@@ -133,29 +133,31 @@ function system_requirements($phase) {
+      '#prefix' => '<br />',
+      '#markup' => t('Your PHP installation is too old. Drupal requires at least PHP %version.', ['%version' => DRUPAL_MINIMUM_PHP]),

looks like we do not want this br tag.

YesCT’s picture

FileSize
65.42 KB
71.38 KB

php version and mysql

diff --git a/core/modules/system/system.install b/core/modules/system/system.install
index f38af8a..ebfbb52 100644
--- a/core/modules/system/system.install
+++ b/core/modules/system/system.install
@@ -163,7 +163,7 @@ function system_requirements($phase) {

   // Suggest to update to at least 5.5.21 or 5.6.5 for disabling multiple
   // statements.
-  if (($phase === 'install' || \Drupal::database()->driver() === 'mysql') && !SystemRequirements::phpVersionWithPdoDisallowMultipleStatements($phpversion)) {
+  if (TRUE || ($phase === 'install' || \Drupal::database()->driver() === 'mysql') && !SystemRequirements::phpVersionWithPdoDisallowMultipleStatements($phpversion)) {
     $requirements['php'] = array(
       'title' => t('PHP (multiple statement disabling)'),
       'value' => $phpversion_label,

head

patch

markup

head

            <tr class="system-status-report__entry color-info">
            <td class="system-status-report__status-icon system-status-report__status-icon--info">
          <div title="Info">
            <span class="visually-hidden">Info</span>
          </div>
        </td>
        <td class="system-status-report__status-title">PHP (multiple statement disabling)</td>
        <td>
          5.5.23
                      <div class="description">PHP versions higher than 5.6.5 or 5.5.21 provide built-in SQL injection protection for mysql databases. It is recommended to update.</div>
                  </td>
      </tr>

patch

            <tr class="system-status-report__entry color-info">
            <td class="system-status-report__status-icon system-status-report__status-icon--info">
          <div title="Info">
            <span class="visually-hidden">Info</span>
          </div>
        </td>
        <td class="system-status-report__status-title">PHP (multiple statement disabling)</td>
        <td>
          5.5.23
                      <div class="description"><br />PHP versions higher than 5.6.5 or 5.5.21 provide built-in SQL injection protection for mysql databases. It is recommended to update.</div>
                  </td>
      </tr>

---------

to make it match head, we dont need this br either.

+++ b/core/modules/system/system.install
@@ -164,12 +166,11 @@ function system_requirements($phase) {
+      '#prefix' => '<br />',
+      '#markup' => t('PHP versions higher than 5.6.5 or 5.5.21 provide built-in SQL injection protection for mysql databases. It is recommended to update.'),
YesCT’s picture

Status: Needs work » Needs review
Issue tags: -Needs manual testing
FileSize
3.61 KB
1.59 KB
74.21 KB
64.33 KB

fixed the br's and changed the for more information wording

this needs review. I think it is ready.

YesCT’s picture

ah! here is testing of the report (and the change to the more information string)

head

patch

--------

markup

head

            <tr class="system-status-report__entry color-info">
            <td class="system-status-report__status-icon system-status-report__status-icon--info">
          <div title="Info">
            <span class="visually-hidden">Info</span>
          </div>
        </td>
        <td class="system-status-report__status-title">PHP</td>
        <td>
          5.5.23 (<a href="/drupal/admin/reports/status/php">more information</a>)
                  </td>
      </tr>

patch

            <tr class="system-status-report__entry color-info">
            <td class="system-status-report__status-icon system-status-report__status-icon--info">
          <div title="Info">
            <span class="visually-hidden">Info</span>
          </div>
        </td>
        <td class="system-status-report__status-title">PHP</td>
        <td>
          5.5.23
                      <div class="description">For more information <a href="/drupal2/admin/reports/status/php">view the PHP configuration</a>.</div>
                  </td>
      </tr>
joelpittet’s picture

Status: Needs review » Needs work

This looks really close just a couple of things I spotted while reviewing the patch:

  1. +++ b/core/modules/system/system.install
    @@ -133,29 +133,30 @@ function system_requirements($phase) {
    +    $requirements['php']['description'][] = [
    ...
    +    $requirements['php']['description'][] = [
    

    This looks like it may append instead of override the description.

  2. +++ b/core/modules/system/system.install
    @@ -133,29 +133,30 @@ function system_requirements($phase) {
    +        '#markup' => t('For more information <a href="@link">view the PHP configuration</a>.', ['@link' => (new Url('system.php'))->toString()]),
    

    @link should be @url.

YesCT’s picture

Status: Needs work » Needs review
FileSize
3.6 KB
2.09 KB

doh! it totally doubled up the messages. confirmed it with manual testing. and checked with this fix that the behavior with the patch is now back to head (in that the last message set is the one that gets shown).

also changed to @url.

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
7.08 KB

Awesome, gave this a manual test with the 4 different messages (and one in the status report page with extra info).

The screenshots still apply above except I also tested if multiple were to override each other now and they don't.

stefan.r’s picture

Yes if we don't want any markup in the value key this should be fine.

Were the last 2 chunks in that patch (about the minimum version and the multiple statement disabling) actually needed?

Also, a separate issue, but doesn't the multiple statement message overwrite the previous message, I don't think it should?

YesCT’s picture

Status: Reviewed & tested by the community » Needs work

@stefan.r ah, I'll try just setting description to a string that comes from t() and not using the markup key (cause the string doesn't have markup).

about the overwriting, yes, I opened #2552061: system_requirements() PDO warning overwrites other PHP messages in #18

YesCT’s picture

Status: Needs work » Needs review
FileSize
2.13 KB

so stricktly speaking, I think this is the minimum changes that were needed.

but I did really like some of the refactoring ...

joelpittet’s picture

It would have been nice to keep some of the duplication out of the if statement but this is minimal now.

We have a bit of a regression that I didn't spot before but the array would have helped with. Where if there is a version number was added at runtime in the description it will now be overridden and missing from the error cases.

Though in our docs we do say we should leave the 'value' key be a value only, and I'd have loved to remove the extra cruft from here we have an issue open to resolve that.
https://api.drupal.org/api/drupal/modules!system!system.api.php/function...
#2549803: hook_requirements() 'value' key is confusing and used inconsistently, and 'title' is only displayed during installation if the value is also set

So two ways forward, one continue with the 'value' key clean-up but get those render array #markup back to prepend the phpinfo link in the top of the description. Or just refactor the wording slightly and change the SafeMarkup::set() to a t() call with the version as a placeholder instead of the concatenation.

joelpittet’s picture

Status: Needs review » Needs work

Oh yeah and this is NW on #33

willzyx’s picture

additionally with changes in #33 the link to phpinfo() (if available) is not shown if PHP version allows multiple statements..

YesCT’s picture

FileSize
147.02 KB
165.41 KB

some screenshots of the problem.

head

patch

YesCT’s picture

this does "Or just refactor the wording slightly and change the SafeMarkup::set() to a t() call with the version as a placeholder instead of the concatenation." from #33

verified these are all the same in head and with this patch.

install

reports

---
[edit: but need to check the reports page... when there is no phpinfo and the mysql thing...]

Status: Needs review » Needs work

The last submitted patch, 37: 2551725.37.patch, failed testing.

YesCT’s picture

with just

diff --git a/core/modules/system/system.install b/core/modules/system/system.install
index f38af8a..9178e5a 100644
--- a/core/modules/system/system.install
+++ b/core/modules/system/system.install
@@ -135,7 +135,7 @@ function system_requirements($phase) {

   // Test PHP version and show link to phpinfo() if it's available
   $phpversion = $phpversion_label = phpversion();
-  if (function_exists('phpinfo')) {
+  if (!function_exists('phpinfo')) {
     // $phpversion is safe and output of l() is safe, so this value is safe.
     if ($phase === 'runtime') {
       $phpversion_label = SafeMarkup::set($phpversion . ' (' . \Drupal::l(t('more information'), new Url('system.php')) . ')');

same in head

and with the patch

==========

but with

diff --git a/core/modules/system/system.install b/core/modules/system/system.install
index f38af8a..b5de4a8 100644
--- a/core/modules/system/system.install
+++ b/core/modules/system/system.install
@@ -135,7 +135,7 @@ function system_requirements($phase) {

   // Test PHP version and show link to phpinfo() if it's available
   $phpversion = $phpversion_label = phpversion();
-  if (function_exists('phpinfo')) {
+  if (!function_exists('phpinfo')) {
     // $phpversion is safe and output of l() is safe, so this value is safe.
     if ($phase === 'runtime') {
       $phpversion_label = SafeMarkup::set($phpversion . ' (' . \Drupal::l(t('more information'), new Url('system.php')) . ')');
@@ -163,7 +163,7 @@ function system_requirements($phase) {

   // Suggest to update to at least 5.5.21 or 5.6.5 for disabling multiple
   // statements.
-  if (($phase === 'install' || \Drupal::database()->driver() === 'mysql') && !SystemRequirements::phpVersionWithPdoDisallowMultipleStatements($phpversion)) {
+  if (TRUE || ($phase === 'install' || \Drupal::database()->driver() === 'mysql') && !SystemRequirements::phpVersionWithPdoDisallowMultipleStatements($phpversion)) {
     $requirements['php'] = array(
       'title' => t('PHP (multiple statement disabling)'),
       'value' => $phpversion_label,

...ah, it is ok. this is the same in head and with the patch

head

patch

and this weirdness seems to me to be very unlikely to actually occur for real sites, and would be fixed by #2552061: system_requirements() PDO warning overwrites other PHP messages

YesCT’s picture

Status: Needs work » Needs review

fails seem like they could be unrelated. are only on the old testbot, not the new CI 2.0 one.

Here they are so we can see if they happen again:

Page title 'Site information | Drupal' is equal to 'Site information | Site name global conf override'.	Other	ConfigFormOverrideTest.php	38	Drupal\config\Tests\ConfigFormOverrideTest->testFormsWithOverrides()

Page title 'Site information | Drupal' is equal to 'Site information | Site name global conf override'.	Other	ConfigFormOverrideTest.php	47	Drupal\config\Tests\ConfigFormOverrideTest->testFormsWithOverrides()
HTTP response header container_rebuild_indicator with value new-identifier found, actual value:	Browser	ContainerRebuildWebTest.php	35	Drupal\system\Tests\DrupalKernel\ContainerRebuildWebTest->testSetContainerRebuildWithDifferentDeploymentIdentifier()	
HTTP response header container_rebuild_indicator with value new-identifier found, actual value:	Browser	ContainerRebuildWebTest.php	35	Drupal\system\Tests\DrupalKernel\ContainerRebuildWebTest->testSetContainerRebuildWithDifferentDeploymentIdentifier()	
HTTP response header container_rebuild_test_parameter with value rebuild_me_please found, actual value: Browser	ContainerRebuildWebTest.php	57	Drupal\system\Tests\DrupalKernel\ContainerRebuildWebTest->testContainerInvalidation()

I'll send it for a retest.

YesCT queued 37: 2551725.37.patch for re-testing.

stefan.r’s picture

Status: Needs review » Reviewed & tested by the community

This is simpler and still gets rid of the SafeMarkup::set() call, moving the link from the value to the description can be looked at in #2549803: hook_requirements() 'value' key is confusing and used inconsistently, and 'title' is only displayed during installation if the value is also set.

Tests are green and manual testing looks good, so RTBC.

alexpott’s picture

Assigned: Unassigned » xjm

Yep this works to resolve the immediate issue and we can continue to work on whether or not this is a correct value in #2549803: hook_requirements() 'value' key is confusing and used inconsistently, and 'title' is only displayed during installation if the value is also set.

I guess one consideration is that now translators have to provide a translation for this and...

$help = \Drupal::moduleHandler()->moduleExists('help') ? ' ' . \Drupal::l(t('More information'), new Url('help.page', ['name' => 'syslog'])) . '.' : NULL;
 

@xjm - are you happy to proceed here?

xjm’s picture

Assigned: xjm » Unassigned
Status: Reviewed & tested by the community » Patch (to be ported)

Works for me too. I talked to @Gábor Hojtsy and the string in #44 would be duplicated anyway already because they have different case.

So committed and pushed to 8.0.x!

  • xjm committed 337b58a on 8.0.x
    Issue #2551725 by YesCT, dorficus, josephdpurcell, joelpittet, stefan.r...
xjm’s picture

Status: Patch (to be ported) » Fixed

Pf. ;)

Status: Fixed » Closed (fixed)

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

hass’s picture

removed, sorry wrong case.