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

Files: 
CommentFileSizeAuthor
#40 2488860-40.patch8.41 KBdawehner
#36 2488860-36.patch8.44 KBalexpott
#36 35-36-interdiff.txt2.3 KBalexpott
#35 2488860-35.patch9.14 KBalexpott
#26 2488860-26.patch9.34 KBalexpott
#26 25-26-interdiff.txt2.01 KBalexpott
#25 interdiff.txt652 bytesdawehner
#25 2488860-25.patch8.88 KBdawehner
#23 2488860-23.patch8.57 KBalexpott
#23 21-23-interdiff.txt8.03 KBalexpott
#21 2488860-21.patch2.7 KBalexpott
#16 2488860-16.patch7.7 KBjgeryk
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Failed to run tests: failed during invocation of run-tests.sh --clean. View
#4 2488860-4.patch19.69 KBdawehner
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

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.