Add test for enabling modules with incompatible PHP version dependency

This is sheer wonderful, thanks so much. I recognize and appreciate the nod towards the 6502 :) did you have an Apple ][ or a Commodore 64?

Yay, it's passing.

+++ b/core/modules/system/src/Tests/Module/DependencyTest.php
@@ -96,6 +96,21 @@ function testIncompatibleCoreVersionDependency() {
+   * Tests enabling a module that depends on a module with an incompatible PHP version.

Exceedec 80 characters.
https://www.drupal.org/coding-standards#linelength

Fixed line length issue

Found one comment was modified due to my last patch updated as per earlier.

Thanks for the comment fixes. "Checks functionality of project name spaces for dependencies." this one was correct in the original: it's one word, "namespace" both in PHP and programming in general

Oh! yes, I agree
I have updated my patch accordingly.

Looks great.

Great!

1. +++ b/core/modules/system/src/Tests/Module/DependencyTest.php
   @@ -58,8 +58,8 @@ function testEnableWithoutDependency() {
   -    // Test that the system_dependencies_test module is marked
   -    // as missing a dependency.
   +    // Test that the system_dependencies_test module is marked as missing a
   +    // dependency.
   
   @@ -67,7 +67,8 @@ function testMissingModules() {
   -   * Tests enabling a module that depends on an incompatible version of a module.
   +   * Tests enabling a module that depends on an incompatible version of a
   +   * module.
   
   @@ -82,7 +83,8 @@ function testIncompatibleModuleVersionDependency() {
   -   * Tests enabling a module that depends on a module with an incompatible core version.
   +   * Tests enabling a module that depends on a module with an incompatible core
   +   * version.
   
   @@ -96,7 +98,24 @@ function testIncompatibleCoreVersionDependency() {
   +  /**
   +   * Tests enabling a module that depends on a module which fails
   +   * hook_requirements().
   

   These are out-of-scope changes that are not part of adding the test.

2. +++ b/core/modules/system/src/Tests/Module/DependencyTest.php
   @@ -96,7 +98,24 @@ function testIncompatibleCoreVersionDependency() {
      /**
   -   * Tests enabling a module that depends on a module which fails hook_requirements().
   +   * Tests enabling a module that depends on a module which fails
   +   * hook_requirements().
   

   I think this test description should be different from the one below.

3. +++ b/core/modules/system/src/Tests/Module/DependencyTest.php
   @@ -96,7 +98,24 @@ function testIncompatibleCoreVersionDependency() {
   +    $this->assertRaw(t('This module requires PHP version @php_required and is incompatible with PHP version @php_version.', array(
   +      '@php_required' => '6502.*',
   +      '@php_version' => phpversion(),
   +    )), 'A module that depends on a module with an incompatible PHP version is marked as such.');
   

   I don't think that the assertion message 'A module that depends on a module with an incompatible PHP version is marked as such.' adds much. Also looking at the test is it even necessary to have the module depending on a module with an impossible PHP version. I think even if that module was not there this test would pass - no?

I've made the changes suggested in #23.

1. Removed out of scope comment changes
2. Changed doc block comment
3. Changed test message and removed the additional dependent module

Sorry, patch with a better test message attached.

Updated Version. Trying to work on this as part of Finnish monthly Drupal sprint.

Added new patch removing one more out-of-scope comment change. See interdiff.

And of course the patch file I attached was the wrong one. Sorry about that. Redoing...

Here is the correct patch and the correct interdiff.

Rerolled the patch.

@Munavijayalakshmi I also got the same patch as #40.

Code review is ok to me and all tests are passing (on my local and CI), should we finally close the issue? :)

1. +++ b/core/modules/system/src/Tests/Module/DependencyTest.php
   @@ -91,6 +91,20 @@ public function testIncompatibleCoreVersionDependency() {
   +   * Tests users are informed if the PHP dependency requirements of
   +   * a module are not met.
   

   This should be a single line (but still less than 80 characters).

2. +++ b/core/modules/system/tests/modules/system_incompatible_php_version_test/system_incompatible_php_version_test.info.yml
   @@ -0,0 +1,7 @@
   +php: 6502
   \ No newline at end of file
   

   Let's add a newline.

Made changes stated in #44

+++ b/core/modules/system/src/Tests/Module/DependencyTest.php
@@ -91,6 +91,19 @@ public function testIncompatibleCoreVersionDependency() {
+   * Tests users are informed if the PHP dependency requirements of a module are not met.

Sorry to be annoying but this is now longer than 80 characters. Maybe something like: "Tests that modules with unmet PHP version dependencies cannot be installed." ?

New patch with shorter comment.

My latest patch was missing the info file, sorry for that. Re-rolling with the info file onboard.

+++ b/core/modules/system/src/Tests/Module/DependencyTest.php
@@ -91,6 +91,19 @@ public function testIncompatibleCoreVersionDependency() {
+    $this->assertRaw(t('This module requires PHP version @php_required and is incompatible with PHP version @php_version.', [
+      '@php_required' => '6502.*',
+      '@php_version' => phpversion(),
+    ]), 'User is informed when the PHP dependency requirement of a module is not met.');

This would be better to not use t() and just assert on the text rather then the actual html. See #2875148: BrowserTestBase: Steer new test development away from translation

+++ b/core/modules/system/tests/modules/system_test/system_test.module
@@ -71,6 +71,7 @@ function system_test_system_info_alter(&$info, Extension $file, $type) {
+    'system_incompatible_php_version_test',

Is this actually necessary? I don't think it is.

Changes as per comment #52 & also added interdiff.

+++ b/core/modules/system/tests/modules/system_test/system_test.module
@@ -71,6 +71,7 @@ function system_test_system_info_alter(&$info, Extension $file, $type) {
+    'system_incompatible_php_version_test',

I am also think this is not necessary so removed it from current patch.

Added the format_string() function to allow using the placeholders that broke when removing t().

+++ b/core/modules/system/src/Tests/Module/DependencyTest.php
@@ -91,6 +91,19 @@ public function testIncompatibleCoreVersionDependency() {
+    $this->assertRaw(format_string('This module requires PHP version @php_required and is incompatible with PHP version @php_version.', [
+      '@php_required' => '6502.*',
+      '@php_version' => phpversion(),
+    ]), 'User is informed when the PHP dependency requirement of a module is not met.');

format_string() is deprecated and we shouldn't be adding new usages of it. You can use the FormattableMarkup class instead. Or you could do something like this:

$this->assertText('This module requires PHP version 6502.* and is incompatible with PHP version ' . phpversion());

Good catch, attached is a patch for this.

Assuming the bot comes back green, this looks good. Thanks!

I checked some boxes to update credit.

Committed 05ffa70 and pushed to 8.4.x. Please create interdiffs when changing patches. Thank you!