Problem/Motivation

\Drupal\system\SystemRequirements::phpVersionWithPdoDisallowMultipleStatements() is pointless now PHP5 is not supported.

Proposed resolution

Deprecate \Drupal\system\SystemRequirements as phpVersionWithPdoDisallowMultipleStatements is the only method in the class.

Remaining tasks

  • Create a change record
  • Add an @trigger_error to main in the class
  • Convert \Drupal\Tests\system\Unit\SystemRequirementsTest to a legacy test

User interface changes

None

API changes

\Drupal\system\SystemRequirements is deprecated

Data model changes

None

Release notes snippet

N/a

CommentFileSizeAuthor
#11 3054060-11.patch4.84 KBalexpott
#2 Deprecate_phpVersionWithPdoDisallowMultipleState()-3054060-2.patch3.52 KBneeravbm

Comments

alexpott created an issue. See original summary.

neeravbm’s picture

neeravbm commented
StatusFileSize
new Deprecate_phpVersionWithPdoDisallowMultipleState()-3054060-2.patch3.52 KB

I have attached the patch that does the following:

1) Adds @trigger_error to main class.
2) Adds @deprecated and @see to phpVersionWithPdoDisallowMultipleStatements() method.
3) Adds @group legacy to SystemRequirementsTest.

Can you let me know how to create a change record?

Also according to Drupal core deprecation policy, it seems that I need to create a new issue to fix the test so that the deprecated code is not exercised. Here's the related issue: https://www.drupal.org/project/drupal/issues/3054243.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

@neeravbm thanks for working on this! You can create change records here - http://drupal.org/list-changes/drupal

Also reviews of parent issue #3053363: Remove support for PHP 5 in Drupal 8.8 are very welcome because then we can unpostpone this one.

I'm interested in why you thought you needed to open #3054243: Remove \Drupal\system\SystemRequirements after reading https://www.drupal.org/core/deprecation - I don't think that it is necessary - this issue should remove all runtime and non-legacy test usages.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented 
  1. +++ core/modules/system/tests/src/Functional/System/StatusTest.php
@@ -54,12 +54,7 @@
-    if (\Drupal::database()->driver() === 'mysql' && !SystemRequirements::phpVersionWithPdoDisallowMultipleStatements($phpversion)) {
-      $this->assertText(t('PHP (multiple statement disabling)'));
-    }
-    else {
-      $this->assertNoText(t('PHP (multiple statement disabling)'));
-    }
+    $this->assertNoText(t('PHP (multiple statement disabling)'));

    This change is not quite right - we use other DB drivers than mysql so we need to leave the first part of the if there. 

  2. +++ core/modules/system/src/SystemRequirements.php
@@ -2,6 +2,8 @@
+@trigger_error('The ' . __NAMESPACE__ . '\SystemRequirements is deprecated in Drupal 8.8.0 and will be removed before Drupal 9.0.0 because all supported versions of PHP return TRUE for the method phpVersionWithPdoDisallowMultipleStatements(), which is the only method in this class. Instead, replace the return value of this method by TRUE. See https://www.drupal.org/project/drupal/issues/3054060.', E_USER_DEPRECATED);

@@ -13,6 +15,13 @@
+   * @see https://www.drupal.org/project/drupal/issues/3054060

    The links here need to be to the change record - not the issue. 

  3. +++ core/modules/system/src/SystemRequirements.php
@@ -13,6 +15,13 @@
+   * @deprecated in Drupal 8.8.0 and will be removed before Drupal 9.0.0.
+   *   Replace the return value of phpVersionWithPdoDisallowMultipleStatements()
+   *   method with TRUE since all supported versions of PHP return TRUE from
+   *   this method.

    The deprecation message can be finessed. All supported PHP versions support disabling multi-statement queries in MySQL making the method irrelevant. So we could say:

    @deprecated in Drupal 8.8.0 and will be removed before Drupal 9.0.0. All supported PHP versions support disabling multi-statement queries in MySQL.

    The @trigger_error() text should be updated too. 

  4. +++ core/modules/system/tests/src/Unit/SystemRequirementsTest.php
@@ -16,7 +17,7 @@
   public function testPhpVersionWithPdoDisallowMultipleStatements($version, $expected) {

    This test needs an @expectedDeprecation annotation. 

  5. +++ core/modules/system/tests/src/Unit/SystemRequirementsTest.php
@@ -16,7 +17,7 @@
-    $this->assertEquals($expected, SystemRequirements::phpVersionWithPdoDisallowMultipleStatements($version));
+    $this->assertTrue($expected);

    The test still needs to call the deprecated code. Legacy tests test the deprecated code.

neeravbm’s picture

neeravbm commented

@alexpott, thanks for the feedback! I'll work on the changes and upload a new patch.

neeravbm’s picture

neeravbm commented

@alexpott I have a question regarding the following code:

if (\Drupal::database()->driver() === 'mysql' && !SystemRequirements::phpVersionWithPdoDisallowMultipleStatements($phpversion)) {
  $this->assertText(t('PHP (multiple statement disabling)'));
}
else {
  $this->assertNoText(t('PHP (multiple statement disabling)'));
}

With PHP 7 and above, SystemRequirements::phpVersionWithPdoDisallowMultipleStatements($phpversion) is always TRUE. Which means that the if condition will always return FALSE irrespective of the database driver. So why do we need the first condition if this code is going to run on PHP 7 and above only?

neeravbm’s picture

neeravbm commented

@alexpott I created https://www.drupal.org/project/drupal/issues/3054243 because https://www.drupal.org/core/deprecation states the following:

Tests which exercise deprecated code can be annotated with @group legacy in order to avoid deprecation notices during testing, but a follow-up issue should be filed in order to fix such tests to not exercise deprecated code.

neeravbm’s picture

neeravbm commented

@alexpott Regarding your comment:

Also reviews of parent issue #3053363: Remove support for PHP 5 in Drupal 8.8 are very welcome because then we can unpostpone this one.

I am new to core contribution and it's very difficult to start on any issue that already has numerous comments with differing opinions. Any suggestion on what should I start looking at?

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Postponed » Needs work

The blocker is in!

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented

There is a now obsolete reference to \PDO::MYSQL_ATTR_MULTI_STATEMENTS in \Drupal\KernelTests\Core\Database\ConnectionTest::testMultipleStatementsForNewPhp(), as well, should that be removed as part of this patch or do we need a separate issue for that?

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs work » Needs review
StatusFileSize
new 3054060-11.patch4.84 KB

@tstoeckler nice find I think it makes sense to do that here. It's all related.

I was wrong about #4.1 - we can completely remove that assertion - there no longer is a requirement warning for PHP (multiple statement disabling) in the code base.

Creating an interdiff is tricky because the original patch was created in an interesting way.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Title: \Drupal\system\SystemRequirements::phpVersionWithPdoDisallowMultipleStatements() should be deprecated as all supported versions of PHP return TRUE » Deprecate \Drupal\system\SystemRequirements::phpVersionWithPdoDisallowMultipleStatements() as all supported versions of PHP return TRUE
mondrake’s picture

mondrake
Location 🇮🇹
commented 
+++ b/core/modules/system/tests/src/Unit/SystemRequirementsTest.php
@@ -8,12 +8,14 @@
 /**
  * @coversDefaultClass \Drupal\system\SystemRequirements
  * @group system
+ * @group legacy
  */
 class SystemRequirementsTest extends UnitTestCase {
 
   /**
    * @covers ::phpVersionWithPdoDisallowMultipleStatements
    * @dataProvider providerTestPhpVersionWithPdoDisallowMultipleStatements
+   * @expectedDeprecation Drupal\system\SystemRequirements::phpVersionWithPdoDisallowMultipleStatements() is deprecated in Drupal 8.8.0 and will be removed before Drupal 9.0.0. All supported PHP versions support disabling multi-statement queries in MySQL. See https://www.drupal.org/node/3054692
    */
   public function testPhpVersionWithPdoDisallowMultipleStatements($version, $expected) {
     $this->assertEquals($expected, SystemRequirements::phpVersionWithPdoDisallowMultipleStatements($version));

I think we could also @expectDeprecation for the SystemRequirements class itself here?

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

@mondrake nope well not unless we run the test in isolation - because PHP only loads the class once for all the tests. I felt that that was not worth it. As there is not logic in triggering the class level deprecation.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs review » Reviewed & tested by the community

OK, RTBC

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed 1305cbe and pushed to 8.8.x. Thanks!

  • catch committed 1305cbe on 8.8.x 
    Issue #3054060 by alexpott, neeravbm, tstoeckler: Deprecate \Drupal\...

Status: Fixed » Closed (fixed)

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