Problem/Motivation

The current way of deprecation functionality is the following: Add @deprecated as of ... to our function signatures/classes.

This has a couple of disadvantages:

  • Its really just documentation, nothing happens on runtime
  • Its not covering all usecases, like a different (maybe additional) parameter to a function/value of a parameter

Proposed resolution

Use @trigger_error($message, E_USER_DEPRECATED);. With the @ the error is hidden by default.
https://github.com/symfony/phpunit-bridge thought provides an error handler which makes those deprecation errors visible in tests.
With that we gain both the possiblity to slowly deprecate stuff, while migrating them over time, but still a way to ensure that those are actually thrown. The bridge adds some test helpers for it.

Remaining tasks

User interface changes

API changes

Comments

dawehner’s picture

dawehner’s picture

Meh, merged all the work together but will extract things into their own methods.
Let's first see how the code reacts, sadly my patch is too big atm :(

dawehner’s picture

dawehner’s picture

Status: Active » Needs review
FileSize
19.69 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch 2488860-4.patch. Unable to apply patch. See the log in the details link for more information. View

For now just phpunit-bridge + use it

jibran’s picture

Issue tags: +Needs beta evaluation

Should we remove this once we'll upgrade to symofony 2.7.0?

  1. +++ b/core/composer.json
    @@ -31,7 +33,8 @@
    +    "symfony/phpunit-bridge": "^3.0@dev"
    

    Can we move this up with rest of the symfony components?

  2. +++ b/core/phpunit.xml.dist
    @@ -10,6 +10,7 @@
    +    <ini name="error_reporting" value="-1" />
    

    I think this is not intentional.

dawehner’s picture

I think this is not intentional.

Well .. kinda yes ...

daffie’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll, +Needs issue summary update

This patch is adding a new library to drupal core. So there is a Dries approval necessary.

The issue summary need an update to better explain why this library is necessary.

+++ b/core/composer.json
@@ -31,7 +33,8 @@
+    "symfony/phpunit-bridge": "^3.0@dev"

Why do you want the dev-master and not regular version 2.7 or 2.8?

+++ b/core/phpunit.xml.dist
@@ -10,6 +10,7 @@
+    <ini name="error_reporting" value="-1" />

You say that this is intentional. The value is now set to 32767. Can you explain why you want it changed.

daffie’s picture

Component: simpletest.module » phpunit
dawehner’s picture

Assigned: dawehner » Unassigned

So there is a Dries approval necessary.

This is no longer true with the new core structure, gladly!!!!!!!!!!

daffie’s picture

@dawehner: Do you have a link for me to that info. :-)

dawehner’s picture

daffie’s picture

@dawehner: Thanks.

Status: Needs work » Needs review

googletorp queued 4: 2488860-4.patch for re-testing.

Status: Needs review » Needs work

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

jgeryk’s picture

Assigned: Unassigned » jgeryk

Working on reroll

jgeryk’s picture

Assigned: jgeryk » Unassigned
Issue tags: -Needs reroll
FileSize
7.7 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Failed to run tests: failed during invocation of run-tests.sh --clean. View
daffie’s picture

Status: Needs work » Needs review

Setting the status to "Needs review". Now the testbot will test the patch.

Status: Needs review » Needs work

The last submitted patch, 16: 2488860-16.patch, failed testing.

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.

alexpott’s picture

Version: 8.2.x-dev » 8.3.x-dev
Status: Needs work » Needs review
Issue tags: -Needs beta evaluation
FileSize
2.7 KB

Status: Needs review » Needs work

The last submitted patch, 21: 2488860-21.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
Issue tags: +Needs change record
FileSize
8.03 KB
8.57 KB

Here's a test and added the @group legacy to all the places in core where necessary. This change is going to need a change record and probably a core group announcement :)

alexpott’s picture

The reason I went for the dev version is the ErrorAssert class that is coming in Symfony 3.2 - it just makes writing tests so much easier.

dawehner’s picture

Its quite nice how the phpunit bridge solves the problem. Is it okay for now to just detect the deprecations in phpunit and not in webtestcase?

Here is a small expansion of the test coverage.

alexpott’s picture

Good idea - we can wrap all of that up in a data provider to make testing anything additional simple.

Given we're moving away from Simpletest I think at best the integration with that should be a follow-up.

dawehner’s picture

I'll work on a change record/better issue summary.

dawehner’s picture

Issue summary: View changes
dawehner’s picture

alexpott’s picture

alexpott’s picture

@dawehner nice change record, we probably need a docs page as well. And obviously before all of this we need to firm up the policy on #2575081: [policy, no patch] Use E_USER_DEPRECATED in Drupal 8 minor releases

dawehner’s picture

Status: Needs review » Postponed

Let's postpone this on the policy issue first.

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

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

xjm’s picture

Status: Postponed » Needs review

The policy issue has been adopted now, so we can continue work here. Yay!

alexpott’s picture

New patch reflecting the fact Symfony 3.2 is out and re-rolled on top of 8.4.x.

alexpott’s picture

The last submitted patch, 35: 2488860-35.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 36: 2488860-36.patch, failed testing.

dawehner’s picture

Interesting.

dawehner’s picture

Status: Needs work » Needs review
FileSize
8.41 KB

Here is a quick reroll

dawehner’s picture

Once this task is done we should update the change record to remember people to update the phpunit.xml file based upon the phpunit.xml.dist file. Could we maybe check that somehow as part of UnitTestCase or so that the listener is registered or so?

Status: Needs review » Needs work

The last submitted patch, 40: 2488860-40.patch, failed testing.

Mile23’s picture

Trying to add deprecation errors to deprecated hooks here: #2866779: Add a way to trigger_error() for deprecated hooks

But the path ahead is unclear because core doesn't really even know what to do with @trigger_error() at this point, so postponing on this issue.

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
8.44 KB

Here's a reroll

Status: Needs review » Needs work

The last submitted patch, 44: 2488860-44.patch, failed testing.

Mile23’s picture

Issue tags: +@deprecated, +Needs followup

The test results of #44: https://www.drupal.org/pift-ci-job/646796

They tell us there's deprecated code in core (edited):

assertNoPattern() is deprecated and scheduled for removal in Drupal 9.0.0. Use $this->assertSession()->responseNotMatches($pattern) instead. See https://www.drupal.org/node/2864262: 1x
    1x in AssertLegacyTraitTest::testAssertNoPattern from Drupal\Tests\Core\Assert

Twig loader "Drupal\Core\Template\Loader\StringLoader" should implement Twig_SourceContextLoaderInterface since version 1.27: 2x
    1x in TwigExtensionTest::testCreateAttribute from Drupal\Tests\Core\Template
    1x in TwigExtensionTest::testFormatDate from Drupal\Tests\Core\Template

Twig loader "Drupal\Core\Template\Loader\StringLoader" should implement Twig_SourceContextLoaderInterface since version 1.27: 7x
    7x in TwigSandboxTest::setUp from Drupal\Tests\Core\Template

CckFieldPluginBase is deprecated in Drupal 8.3.x and will be
be removed before Drupal 9.0.x. Use \Drupal\migrate_drupal\Plugin\migrate\field\FieldPluginBase
instead: 1x
    1x in TextFieldTest::setUp from Drupal\Tests\text\Unit\Migrate

(There's also a fail of Drupal\simpletest\Tests\UiPhpUnitOutputTest, which is probably related to one of these other deprecation failures.)

So we need child issues to remove usages of those deprecated items.

Also, at some point, we're going to mark TestBase and WebTestBase as deprecated. At that point, we'll be unable to test those classes using phpunit-based tests unless we just don't add @trigger_error() to the deprecation.

It'll also be interesting to see what explodes when, for instance, \Drupal::entityManager() throws this error as it should. That's every entity factory method.

Mile23’s picture

Plan for deprecating hooks: #2870058: Figure out how to deprecate hooks

Linked here because if a defined function is marked as @deprecated, then the IDE can show you that it's deprecated. If a hook is deprecated, we only have @trigger_error() at run time.

heddn’s picture

Title: Bring phpunit bridge into drupal and use it for unit tests and simpletest » Bring phpunit bridge into drupal and use it for unit tests and simpletest to handle Deprecation

Re-titling to focus on deprecation. The solution being proposed here doesn't do anything to help with things like the legitimate failure in #2705925-35: ImageItem presave() fails when pointing to a non-existing file entity. Where the node migration is throwing a simple warning and getting converted into an Exception, which is killing the migration.

alexpott’s picture

Status: Needs work » Needs review
FileSize
2.14 KB
10.25 KB

Let's mark the tests as legacy and then file followups to remove it in the case of the Twig and Migrate tests - at least we get the benefit of this sooner rather than later.

alexpott’s picture

+++ b/core/modules/simpletest/src/Tests/UiPhpUnitOutputTest.php
@@ -35,9 +35,9 @@ public function testOutput() {
-    $this->assertEqual($output[18], 'HTML output was generated<br />');
+    $this->assertEqual($output[19], 'HTML output was generated<br />');
     // Check that URLs are printed as HTML links.
-    $this->assertIdentical(strpos($output[19], '<a href="http'), 0);
+    $this->assertIdentical(strpos($output[20], '<a href="http'), 0);

This change is because the bridge adds an additional line of output.

    public function startTestSuite(\PHPUnit_Framework_TestSuite $suite)
    {
        $suiteName = $suite->getName();

        if (-1 === $this->state) {
            echo "Testing $suiteName\n";

It's the echo here...

Status: Needs review » Needs work

The last submitted patch, 49: 2488860-2-49.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
10.69 KB
457 bytes

Missed one.

alexpott’s picture

alexpott’s picture

Created #2870138: \Drupal\migrate_drupal\Plugin\migrate\cckfield\CckFieldPluginBase is deprecated - replace usages for the migrate one.

It also occurs to me that we need a followup to implement something for BrowserTestBase to catch deprecated usages in the system under test.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Nice!

Mile23’s picture

Status: Reviewed & tested by the community » Needs work
FileSize
2.04 KB

+1 on @group legacy, with follow-ups.

Here's a test you can add to the patch to prove that @trigger_error() gets swallowed somewhere in a functional test, so we do need a follow-up for BrowserTestBase.

Also, the change record references Symfony\Bridge\PhpUnit\ErrorAssert which I couldn't find documented, and which my IDE couldn't find.

alexpott’s picture

Status: Needs work » Needs review
FileSize
10.69 KB

Opened follow up #2870194: Ensure that browsertestbase tests can use Symfony's PHPunit bridge to catch usages of deprecated code and fixed CR. Re-uploading patch in #52 since the last patch on the issue should be the one to commit.

Mile23’s picture

Status: Needs review » Reviewed & tested by the community

Kewl. Sorry about the extra patch.

alexpott’s picture

@Mile23 np - it's useful! I linked it from the new issue.

alexpott’s picture

Mile23’s picture

Issue tags: -Needs followup

Go go go. :-)

Note that symfony/phpunit-bridge also gives us some handy mocks: https://symfony.com/doc/current/components/phpunit_bridge.html

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 57: 2488860-2-52.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
946 bytes
11.62 KB

Yay for legacy testing and the @group legacy... also adding a deprecation notice to a test class is interesting.

Mile23’s picture

Status: Needs review » Reviewed & tested by the community

Deprecation trigger comes from #2824610: Rename DedupeBase/DedupeEntity process plugins to MakeUnique and add documentation

Provisionally RTBCing, because I can run --testsuite unit faster than the testbot.

Update: Never post something like that when it only says 90% complete.

Remaining deprecation notices (2)

The Drupal\Tests\migrate\Unit\process\MigrationTest is deprecated in
Drupal 8.4.0 and will be removed before Drupal 9.0.0. Instead, use Drupal\Tests\migrate\Unit\process\MigrationLookupTest: 1x
    1x in UnitTestSuite::suite from Drupal\Tests\TestSuites

The Drupal\migrate\Plugin\migrate\process\Migration is deprecated in
Drupal 8.4.0 and will be removed before Drupal 9.0.0. Instead, use Drupal\migrate\Plugin\migrate\process\MigrationLookup: 1x
    1x in MigrationTest::testTransformWithStubSkipping from Drupal\Tests\migrate\Unit\process
alexpott’s picture

Thanks @Mile23!

Mile23’s picture

Updated deprecation policy to tell people not to deprecate tests: https://www.drupal.org/core/deprecation

alexpott’s picture

Once this lands we should update the policy with a note about testing.

The last submitted patch, 63: 2488860-2-63.patch, failed testing.

Mile23’s picture

Still RTBC since #65 fixes all the things.

jibran’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

#2712647: Update Symfony components to ~3.2 is in so the patch needs a reroll.

+++ b/composer.lock
@@ -4316,6 +4316,68 @@
+            "version": "v3.2.7",

This can be updated to 3.2.8 now.

alexpott’s picture

Status: Needs work » Needs review
FileSize
4.94 KB
13.7 KB

Rerolled because of #2712647: Update Symfony components to ~3.2. Also that added some more deprecated code paths so fixed them or added @group legacy as necessary.

jibran’s picture

Status: Needs review » Reviewed & tested by the community

Thanks back to RTBC.

jibran’s picture

Issue tags: -Needs reroll

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 71: 2488860-2-71.patch, failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
15.27 KB
1.73 KB

Fixing those three YAML files, let's see how this one does.

alexpott’s picture

Status: Needs review » Reviewed & tested by the community

I've reviewed the changes @catch has made in #75. The fun fact about the YamlTest is that it existing to detect where the duplicate keys are not the same. One of the supported parsers trusts the first and the other trusts the last. Which is good because removal is therefore for correct.

+++ b/core/modules/views/config/schema/views.display.schema.yml
@@ -49,9 +49,6 @@ views.display.page:
-        expanded:
-          type: boolean
-          label: 'Expanded'

This is a non root schema duplicate so order is relevant. The only config I found that has this key is core/modules/content_moderation/tests/modules/content_moderation_test_views/config/install/views.view.latest.yml and the order is the as after this patch so @catch has removed the correct one.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Added review credit for @daffie and @jibran.

Committed/pushed to 8.4.x, thanks!

  • catch committed e63e65f on 8.4.x
    Issue #2488860 by alexpott, dawehner, catch, Mile23, jgeryk, claudiu....
xjm’s picture

Issue tags: +8.4.0 release notes
Mile23’s picture

@trigger_error() conflicts with DrupalStandardsListener: #2878248: DrupalStandardsListener improper handling of @trigger_error() deprecation