Problem/Motivation

Follow-up to #2551725-14: Remove system_requirements() SafeMarkup::set() use with 'value' key

system_requirements() php warnings and errors use the same key, so only one is ever shown

steps to reproduce

Get an old version of php (or hack core to make it think it is old) (either pre 5.6.5 or 5.5.21)

Steps for installer phase:

  1. make it so the install will fail (rename settings.php)
  2. make warnings or errors show
  3. attempt to install
  4. look at the requirements page

Steps for runtime phase:

  1. install
  2. make warnings or errors show
  3. look at admin/reports/status

Proposed resolution

Use different keys for each message

Remaining tasks

Review patch
Manual testing

User interface changes

When more than one requirement message is relevant, they will both show.

Installer - before:

Installer - after:

Similarly at runtime.

API changes

No.

Data model changes

CommentFileSizeAuthor
#25 2552061-25.patch986 bytesShubham Chandra
#22 reroll_diff_6-22.txt2.12 KBpooja saraah
#22 2552061-22.patch980 bytespooja saraah
#9 before.png54.46 KBstefan.r
#9 after.png110.82 KBstefan.r
#6 2552061-6.patch1.26 KBstefan.r
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

YesCT created an issue. See original summary.

cilefen’s picture

cilefen CreditAttribution: cilefen commented

Is this block the issue? If not, the summary isn't clear enough for me to understand the problem.

  // 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,
    );
  }

  if (version_compare($phpversion, DRUPAL_MINIMUM_PHP) < 0) {
    $requirements['php']['description'] = t('Your PHP installation is too old. Drupal requires at least PHP %version.', array('%version' => DRUPAL_MINIMUM_PHP));
    $requirements['php']['severity'] = REQUIREMENT_ERROR;
    // If PHP is old, it's not safe to continue with the requirements check.
    return $requirements;
  }
YesCT’s picture

YesCT CreditAttribution: YesCT commented

I think we might as well wait on #2551725: Remove system_requirements() SafeMarkup::set() use with 'value' key but we dont strictly need to.

the problem is that, we cannot, for example, see the warning about both phpinfo and the phpversion, cause whatever the last description is, overwrites other (php) messages that might be relevant.

cilefen’s picture

cilefen CreditAttribution: cilefen commented

I think those sections ought to be smushed together.

stefan.r’s picture

stefan.r CreditAttribution: stefan.r commented
Issue tags: -Needs tests +Needs manual testing
Related issues: +#2388255: (followup) Limit PDO MySQL to executing single statements if PHP supports it

This is not really something that can be automatically tested unless we wrap the call to phpversion(), or have testbots with those outdated PHP versions, which I don't think we do.

stefan.r’s picture

stefan.r CreditAttribution: stefan.r commented
Status: Active » Needs review
FileSize
2552061-6.patch1.26 KB

Updated the wording as well.

stefan.r’s picture

stefan.r CreditAttribution: stefan.r commented
Issue summary: View changes
stefan.r’s picture

stefan.r CreditAttribution: stefan.r commented
Title: system_requirements() php warnings and errors use the same key, so only one is ever shown » system_requirements() PDO warning overwrites other PHP messages
Issue summary: View changes

Updating issue summary to reflect this only affects the PDO warning, for the others I think we _want_ them to override the existing description.

stefan.r’s picture

stefan.r CreditAttribution: stefan.r commented
Issue summary: View changes
FileSize
after.png110.82 KB
before.png54.46 KB

Adding before & after screenshots to IS

stefan.r’s picture

stefan.r CreditAttribution: stefan.r commented
Issue summary: View changes
dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Tag1 Consulting commented 
+++ b/core/modules/system/system.install
@@ -164,10 +164,10 @@ function system_requirements($phase) {
+      'value' => t('Not supported'),

It is more about not recommended right? I mean this is not an error, just INFO

stefan.r’s picture

stefan.r CreditAttribution: stefan.r commented

What is "not supported" is the PDO multiple statement disabling. The value will only ever appear along with the description so it's probably fine like this:

I don't think "Not recommended" would be correct either as the value describes the current state of the title (multiple statement disabling).

Maybe "not available"?

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Version: 8.9.x-dev » 9.4.x-dev
Status: Needs review » Needs work
Issue tags: +Needs reroll

no longer applies

patching file core/modules/system/system.install
Hunk #1 FAILED at 164.
1 out of 1 hunk FAILED -- saving rejects to file core/modules/system/system.install.rej
pooja saraah’s picture

pooja saraah CreditAttribution: pooja saraah at Srijan | A Material+ Company for Drupal India Association commented
Status: Needs work » Needs review
FileSize
2552061-22.patch980 bytes
reroll_diff_6-22.txt2.12 KB

Rerolled patch against 9.4.x
Attached rerolled diff

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Shubham Chandra’s picture

Shubham Chandra CreditAttribution: Shubham Chandra as a volunteer and at Dotsquares Ltd. commented
Issue tags: -Needs reroll
FileSize
2552061-25.patch986 bytes

Re-rolled patch against #22 in drupal 10.1.x

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented

Filed bug report as failures are unrelated #3320483: Remove unused variable $pos in system.install

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Parent issue: #2551725: Remove system_requirements() SafeMarkup::set() use with 'value' key » #2002650: [meta, no patch] improve maintainability by removing unused local variables
Related issues: +#2551725: Remove system_requirements() SafeMarkup::set() use with 'value' key
andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Parent issue: #2002650: [meta, no patch] improve maintainability by removing unused local variables » #2551725: Remove system_requirements() SafeMarkup::set() use with 'value' key
Related issues: -#2551725: Remove system_requirements() SafeMarkup::set() use with 'value' key

messed with tabs, sorry

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Status: Needs review » Closed (outdated)
Issue tags: -Needs manual testing +Bug Smash Initiative

Closing it as the issue only possible on PHP 5.5 but 9.x and 10.x require 7 and 8