Problem/Motivation

The new deprecation policy adds an expectation that @deprecation's will also trigger an error. Core should comply.

Proposed resolution

Add trigger_errors for all deprecations and ensure that they are tested.

Remaining tasks

User interface changes

None

API changes

None

Data model changes

None

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

xjm’s picture

xjm’s picture

Priority: Normal » Major

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

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now 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.

larowlan’s picture

Does this need to remain postponed on #2856742: [meta] Adopt trigger_error() for deprecation messages where it is missing ?
I understand this one is about updating existing deprecations, but is there any reason we can't be fixing those that don't have trigger_error at the same time?

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

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now 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.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now 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.

Berdir’s picture

Status: Postponed » Active

One year later without comment... I don't think so :)

Not sure what the testing part is about, but I think the idea is that we test that they are *not* triggered, so the test support we have is sufficient.

alexpott’s picture

@Berdir the testing part is that we ideally should have at least one legacy test to prove the code still works and is not broken by later changes - until we move to Drupal 9 and can finally get rid of it.

Mile23’s picture

Here are two patches:

One is a test you should run manually and which shouldn't be part of core. It loads every non-test PHP file, checks if it's a class with @deprecated annotations, requires the file, and then verifies that it triggered an E_USER_DEPRECATED error. It requires symfony/finder. This is updated from #3007329-7: [META] Drupal 8 core must not use any deprecated code, must be enforced by automated testing which I should have posted over here.

The other patch adds all the @trigger_error() calls that the test says we should add.

Let's see what technical debt is revealed.

The last submitted patch, 11: 2856744_11.patch, failed testing. View results

Mile23’s picture

There will be approximately eleventy-billion fails on that last patch.

That's because EntityHandlerBase hasn't been replaced in core: #2471663: Undeprecate EntityHandlerBase

This patch undoes the @trigger_error() for EntityHandlerBase.

Don't commit FindDeprecatedTest. :-)

Status: Needs review » Needs work

The last submitted patch, 13: 2856744_12.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Mile23’s picture

So, uh... golly. The messenger service is a very weird deprecation. There should be a followup where someone either explains why \Drupal::messenger() says new LegacyMessenger() or changes core.services.yml to use LegacyMessenger.

Anyway, we skip LegacyMessenger because of the complexity of the deprecation.

Mile23’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 15: 2856744_15.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Status: Needs review » Needs work

The last submitted patch, 18: 2856744_18.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Mile23’s picture

Status: Needs work » Needs review
FileSize
13.21 KB
533 bytes

Does TestBase actually need its deprecated TestServiceProvider? Let's find out.

Mile23’s picture

OK.

This patch splits out the test and stores it here for posterity, and also gives us a patch that we can use.

The caveats here include:

This test only finds classes with @deprecated but no @trigger_error().

It excludes searches for plugins, because plugins are supposed to say @trigger_error() in their constructor. That will require a different test.

It doesn't find anything else that could be further deprecated. So that leaves all the other stuff from https://www.drupal.org/core/deprecation

Therefore this patch is incremental.

alexpott’s picture

+++ b/core/modules/simpletest/src/TestBase.php
@@ -12,6 +12,7 @@
+use Drupal\KernelTests\TestServiceProvider;

This needs an as KernelTestServiceProvider - we've had problems before with specific PHP versions and namespace clashes.

Mile23’s picture

Berdir’s picture

+++ b/core/modules/simpletest/src/TestBase.php
@@ -12,6 +12,7 @@
 use Drupal\Core\Test\TestSetupTrait;
 use Drupal\Core\Utility\Error;
+use Drupal\KernelTests\TestServiceProvider as KernelTestServiceProvider;
 use Drupal\Tests\AssertHelperTrait as BaseAssertHelperTrait;
 use Drupal\Tests\ConfigTestTrait;
 use Drupal\Tests\RandomGeneratorTrait;
@@ -890,7 +891,7 @@ public function run(array $methods = []) {

@@ -890,7 +891,7 @@ public function run(array $methods = []) {
       return;
     }
 
-    TestServiceProvider::$currentTest = $this;
+    KernelTestServiceProvider::$currentTest = $this;
     $simpletest_config = $this->config('simpletest.settings');
 
     // Unless preset from run-tests.sh, retrieve the current verbose setting.
@@ -996,7 +997,7 @@ public function run(array $methods = []) {

@@ -996,7 +997,7 @@ public function run(array $methods = []) {
       TestBase::deleteAssert($test_completion_check_id);
     }
 
-    TestServiceProvider::$currentTest = NULL;
+    KernelTestServiceProvider::$currentTest = NULL;
     // Clear out the error messages and restore error handler.

Does this actually make sense?

Isn't the new one only for phpunit-based tests?

TestBase and its children will at some point get the @deprecated message themself and will be removed completely?

Mile23’s picture

Does this actually make sense?

It makes sense because the goal is to add @trigger_error() everywhere it belongs:

+++ b/core/modules/simpletest/src/TestServiceProvider.php
@@ -2,6 +2,8 @@
+@trigger_error(__NAMESPACE__ . '\TestServiceProvider is deprecated in 8.6.0 for removal before Drupal 9.0.0. Use Drupal\KernelTests\TestServiceProvider instead. See https://www.drupal.org/node/2943146', E_USER_DEPRECATED);
Berdir’s picture

Ok, I see, I was confused because you're doing a "use as DifferentClassName" and KernelTestBase doesn't do that and still references TestServiceProvider. Kinda weird that this setter is in TestBase but only KernelTestBase uses it?

Are you sure the "use as" is really necessary as apparently KernelTestBase, which is in the same namespace, doesn't need it?

Berdir’s picture

Also, this currently "only" adds @trigger_error() to completely deprecated classes, but it says "to code". Which I guess is going to be a lot of places, so maybe we need this to be a META and your patch would be a "to deprecated classes" issue?

Mile23’s picture

Are you sure the "use as" is really necessary as apparently KernelTestBase, which is in the same namespace, doesn't need it?

See #22.

And yah, this should be a meta with different scopes below it.

Berdir’s picture

Title: Add trigger_error(..., E_USER_DEPRECATED) to deprecated code » [META] Add trigger_error(..., E_USER_DEPRECATED) to deprecated code
Mile23’s picture

Gábor Hojtsy’s picture

Hm, how is this different from #2856742: [meta] Adopt trigger_error() for deprecation messages where it is missing ? Should we close that one in favor of this?

Gábor Hojtsy’s picture

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

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.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.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). 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.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now 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.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

quietone’s picture

Status: Needs review » Closed (outdated)

Thanks to everyone who worked on this.

This is for deprecations that were removed in Drupal 9.0.0. 'Tis now outdated. Therefore, closing as outdated.

Thanks!