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

Comments

Mile23 created an issue. See original summary.

Mile23’s picture

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

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

Status: Needs work » Needs review
FileSize
1.64 KB
2.23 KB
700 bytes

Forgot @group annotation.

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

Mile23’s picture

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’s picture

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

dawehner’s picture

+++ 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

Good idea.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Thank you @Mile23!

catch’s picture

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

Status: Needs work » Needs review
FileSize
3.54 KB
2.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

Re-running test.

dawehner’s picture

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

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

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

Mile23’s picture

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

dawehner’s picture

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

Status: Reviewed & tested by the community » Needs review
FileSize
4.15 KB
1.12 KB

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

Mile23’s picture

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

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.