Problem/Motivation

Drupal core deprecation standards say we should:

a) @trigger_error() for classes and files on file load, right after the namespace declaration.

b) Produce a test which proves that @trigger_error() will be caught by the phpunit-bridge

See https://www.drupal.org/core/deprecation in the 'How We Deprecate' section.

Just one snag: We also have a test listener which enforces the validity of @covers and @coversDefaultClasses annotations. This is the DrupalStandardsListener.

DrupalStandardsListener calls class_exists() on annotated classes, which can cause the file to be loaded by the class loader.

This means that outside of the context of the test itself, we can @trigger_error() for deprecation. Needless to say, symfony/phpunit-bridge does not account for our use-case.

This leads to deprecation errors for tests of deprecated classes which are also annotated as @covers.

Proposed resolution

Current hacky solution: Add @group legacy to DrupalStandardsListener::endTest() annotation.

Add your better idea here.

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#23 interdiff.txt1.12 KBMile23
#23 2878248_23.patch4.15 KBMile23
#20 interdiff.txt1.9 KBMile23
#20 2878248_20.patch4.13 KBMile23
#20 2878248_20_test_only.patch3.12 KBMile23
#15 interdiff.txt2.2 KBMile23
#15 2878248_15.patch3.54 KBMile23
#12 interdiff.txt1 KBMile23
#12 2878248_12.patch2.67 KBMile23
#9 interdiff.txt719 bytesMile23
#9 2878248_9.patch2.25 KBMile23
#9 2878248_9_test_only.patch1.66 KBMile23
#6 interdiff_full.txt700 bytesMile23
#6 2878248_6.patch2.23 KBMile23
#6 2878248_6_test_only.patch1.64 KBMile23
#2 2878248_2.patch2.2 KBMile23
#2 2878248_2_test_only.patch1.62 KBMile23
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Mile23 created an issue. See original summary.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented
Version: 8.3.x-dev » 8.4.x-dev
FileSize
2878248_2_test_only.patch1.62 KB
2878248_2.patch2.2 KB

Changing to 8.4.x because phpunit-bridge is only in 8.4.x. Ewps.

Here are two patches, test only and test passing.

The location of the fixture class is just the first one I thought of for PoC, since it's likely that a module's class file would need to be loaded by the classloader during a unit test.

And here's the kind of error I see without the fix:

PHPUnit 4.8.35 by Sebastian Bergmann and contributors.

Testing 
.

Time: 3.58 seconds, Memory: 78.00MB

OK (1 test, 1 assertion)

Remaining deprecation notices (1)

Drupal\system\Deprecation\FixtureDeprecatedClass is deprecated: 1x
    1x in DrupalStandardsListener::endTest from Drupal\Tests\Listeners

Note that this error is being triggered within DrupalStandardsListener::endTest().

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented
Status: Active » Needs review

The last submitted patch, 2: 2878248_2_test_only.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 2: 2878248_2.patch, failed testing.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented
Status: Needs work » Needs review
FileSize
2878248_6_test_only.patch1.64 KB
2878248_6.patch2.23 KB
interdiff_full.txt700 bytes

Forgot @group annotation.

Interdiff is for the full patch, but applies either way.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented

The patches in #2 and #6 add a test called Drupal\Tests\Listeners\DrupalStandardsListenerDeprecationTest.

So here's a question: Why is phpunit --testsuite unit able to run it, but run-tests.sh is not? Look in the console log, it's not there: https://dispatcher.drupalci.org/job/drupal_patches/15702/consoleFull

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented

And here's the answer: #2878269: Modify TestDiscovery so the testbot runs all the tests

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented
FileSize
2878248_9_test_only.patch1.66 KB
2878248_9.patch2.25 KB
interdiff.txt719 bytes

Moved the test to \Drupal\Tests\Core\Listeners\ as a workaround for #2878269: Modify TestDiscovery so the testbot runs all the tests

The last submitted patch, 9: 2878248_9_test_only.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer commented 
+++ b/core/tests/Drupal/Tests/Listeners/DrupalStandardsListener.php
@@ -142,6 +142,8 @@ public function checkValidCoversForTest(TestCase $test) {
    * {@inheritdoc}
+   *
+   * @group legacy
    */

It would be nice to explain in a common why this is needed.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented
FileSize
2878248_12.patch2.67 KB
interdiff.txt1 KB

Good idea.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer commented
Status: Needs review » Reviewed & tested by the community

Thank you @Mile23!

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Needs work

Sorry coding standards checks don't like the empty documentation:


FILE: ...x/core/modules/system/src/Deprecation/FixtureDeprecatedClass.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 7 | ERROR | Doc comment is empty
----------------------------------------------------------------------

'This docblock has been intentionally left blank"?

edit: or whatever the coding standard skip annotation is for that file?

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented
Status: Needs work » Needs review
FileSize
2878248_15.patch3.54 KB
interdiff.txt2.2 KB

I got sidetracked with that phpunit problem and never got back to documentation...

Added and expanded docblock comments, since this is a complex test case.

Note that the test added here reveals a similar problems during code coverage report generation, where the tests pass but the coverage process also ends up throwing the error. Whether this matters or not is outside the scope of this issue, I think. We might need to re-think the placement of the @trigger_error(), unless we don't. :-)

$ ./vendor/bin/phpunit -c core/ --testsuite unit --coverage-html ../coverage --filter DrupalStandardsListenerDeprecationTest
PHPUnit 4.8.35 by Sebastian Bergmann and contributors.

Testing 
.

Time: 27.83 seconds, Memory: 106.00MB

OK (1 test, 1 assertion)

Generating code coverage report in HTML format ... done

Remaining deprecation notices (1)

Drupal\system\Deprecation\FixtureDeprecatedClass is deprecated: 1x
    1x in ClassLoader::loadClass from Composer\Autoload
Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented

Re-running test.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer commented 
index 0000000000..c3fc24a396
--- /dev/null

--- /dev/null
+++ b/core/modules/system/src/Deprecation/FixtureDeprecatedClass.php

+++ b/core/modules/system/src/Deprecation/FixtureDeprecatedClass.php
@@ -0,0 +1,28 @@

@@ -0,0 +1,28 @@
+class FixtureDeprecatedClass {

Is there a reason this cannot be placed inside the tests somehow? It feels weird to have that in system module.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented

If you can think of a better place, please suggest. :-)

It has to be autoloadable but not loaded at that point in testing.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer commented

So a hidden test module in core/modules would not work?

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented
FileSize
2878248_20_test_only.patch3.12 KB
2878248_20.patch4.13 KB
interdiff.txt1.9 KB

Moved to core/modules/system/tests/modules/deprecation_test/.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer commented
Status: Needs review » Reviewed & tested by the community

Thank you @mile23, this is much better

The last submitted patch, 20: 2878248_20_test_only.patch, failed testing.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented
Status: Reviewed & tested by the community » Needs review
FileSize
2878248_23.patch4.15 KB
interdiff.txt1.12 KB

Missed a doc reference to the class we just moved...

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented
Status: Needs review » Reviewed & tested by the community

Moving back to RTBC because it's only a doc change, and the tests will likely run before a maintainer ends up here.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Fixed

Committed 1160fbb and pushed to 8.4.x. Thanks!

  • alexpott committed 1160fbb on 8.4.x 
    Issue #2878248 by Mile23, dawehner: DrupalStandardsListener improper...

Status: Fixed » Closed (fixed)

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