Fix dynamic property deprecations and other unit test failures for PHP 8.2

Fixed the entity unit tests. Needs cleanup, but there's just no way around creating subclasses with the properties, maybe phpunit will in the future add that annotation to mock classes, until then, this is what we need to do. That said, I'm not fond of these entity unit tests at all.

I'm almost certain that they are not adding any test coverage over the existing kernel and functional tests and contain a lot of complexity, i'd vote to just drop EntityUnitTest and EntityUrlTest at least, but that's for later.

Coding style fixes.

thanks, not sure what our codiing standards are for such mock classes and whether or not we need to document them and their properties properly.

+++ b/core/tests/Drupal/Tests/Component/Utility/VariableTest.php
@@ -161,12 +161,22 @@ public function providerTestExport() {
+    ];
+    if (version_compare(PHP_VERSION, '8.2', '>=')) {
+      $result[] = [
+        // A not-stdClass object.
+        "\Drupal\Tests\Component\Utility\StubVariableTestClass::__set_state(array(\n))",
+        new StubVariableTestClass(),
+      ];
+    }
+    else {
+      $result[] = [
         // A not-stdClass object.
         "Drupal\Tests\Component\Utility\StubVariableTestClass::__set_state(array(\n))",
         new StubVariableTestClass(),
-      ],
-    ];
+      ];
+    }
+    return $result;
   }
 

Unsure about this one. The only difference is that PHP 8.2 now apparently starts the class with a \. I'm wondering if we should instead ensure a consistent output, but I'm not sure in which direction that should even go. Always with \ or never with \?

Alternatively, we could ltrim() the output of Variable::export() for now in the test.

Still not sure if we should document the helper classes, core is quite inconsistent in that regard. I feel like at least a class docblock that explains why we are doing it ("Mock class with required properties defined to avoid dynamic property deprecations") would be good?

Yeah, I'd say that makes sense. See comment #5.4 on the randomValue properties, we can change the test instead of adding those.

1. +++ b/core/lib/Drupal/Component/Utility/Variable.php
   @@ -86,6 +86,10 @@ public static function export($var, $prefix = '') {
        else {
          $output = var_export($var, TRUE);
   +      if (version_compare(PHP_VERSION, '8.2', '>=')) {
   +        // @todo Make consistent with PHP 8.2+ https://www.drupal.org/i/3312079.
   +        $output = ltrim($output, '\\');
   +      }
        }
   
   +++ b/core/tests/Drupal/Tests/Component/Utility/VariableTest.php
   @@ -161,22 +161,12 @@ public function providerTestExport() {
   -    ];
   -    if (version_compare(PHP_VERSION, '8.2', '>=')) {
   -      $result[] = [
   -        // A not-stdClass object.
   -        "\Drupal\Tests\Component\Utility\StubVariableTestClass::__set_state(array(\n))",
   -        new StubVariableTestClass(),
   -      ];
   -    }
   -    else {
   -      $result[] = [
   +      [
            // A not-stdClass object.
            "Drupal\Tests\Component\Utility\StubVariableTestClass::__set_state(array(\n))",
            new StubVariableTestClass(),
   -      ];
   -    }
   -    return $result;
   +      ],
   +    ];
   

   as mentioned in slack, would love to have some input on this part, see also previous comment.

2. +++ b/core/modules/system/tests/src/Functional/Database/FakeRecord.php
   @@ -17,6 +17,8 @@ class FakeRecord {
       */
      public $fakeArg;
   +  public string $name;
   +  public string $job;
   
   +++ b/core/tests/Drupal/Tests/Component/EventDispatcher/ContainerAwareEventDispatcherTest.php
   @@ -553,6 +553,7 @@ public function __invoke() {
    class TestEventListener {
    
   +  public $name;
      public $preFooInvoked = FALSE;
   

   lets add a basic docblock to those three.

Re #27 - I think this change is correct for the same reasons that the upstream change is correct - see https://github.com/php/php-src/pull/8233 - this makes the code generated portable to working regardless of namespace the code is used it.

1. +++ b/core/modules/field_ui/tests/src/Kernel/EntityDisplayTest.php
   @@ -262,20 +262,16 @@ public function testFieldComponent() {
   +    $this->assertTrue($formatter === $display->getRenderer($field_name));
   

   assertSame?

2. +++ b/core/modules/field_ui/tests/src/Kernel/EntityDisplayTest.php
   @@ -262,20 +262,16 @@ public function testFieldComponent() {
   +    $this->assertTrue($formatter !== $renderer);
   

   assertNotSame?

3. +++ b/core/modules/field_ui/tests/src/Kernel/EntityFormDisplayTest.php
   @@ -103,20 +103,16 @@ public function testFieldComponent() {
   +    $this->assertTrue($widget === $form_display->getRenderer($field_name));
   

   assertSame?

4. +++ b/core/modules/field_ui/tests/src/Kernel/EntityFormDisplayTest.php
   @@ -103,20 +103,16 @@ public function testFieldComponent() {
   +    $this->assertTrue($widget !== $renderer);
   

   assertNotSame?

5. +++ b/core/modules/system/tests/src/Functional/Database/FakeRecord.php
   @@ -18,6 +18,24 @@ class FakeRecord {
   +  public $name;
   

   typehint property?

6. +++ b/core/modules/system/tests/src/Functional/Database/FakeRecord.php
   @@ -18,6 +18,24 @@ class FakeRecord {
   +  public $job;
   

   typehint property?

7. +++ b/core/tests/Drupal/Tests/Component/DependencyInjection/ContainerTest.php
   @@ -1106,6 +1106,11 @@ class MockService {
   +  public $someProperty;
   

   typehint property?

Testing with

$ php -v
PHP 8.2.0-dev (cli) (built: Oct  2 2022 00:49:17) (NTS)
Copyright (c) The PHP Group
Zend Engine v4.2.0-dev, Copyright (c) Zend Technologies
    with Zend OPcache v8.2.0-dev, Copyright (c), by Zend Technologies

👇

1. +++ b/core/modules/contact/tests/src/Unit/MailHandlerTest.php
   @@ -292,7 +293,7 @@ public function getSendMailMessages() {
   -    $sender = $this->createMock('\Drupal\Core\Session\AccountInterface');
   +    $sender = $this->createMock(User::class);
   

   This test passes fine on PHP 8.2 without this change? 🤔

2. +++ b/core/modules/field_ui/tests/src/Kernel/EntityDisplayTest.php
   @@ -262,20 +262,16 @@ public function testFieldComponent() {
   -    // Check that the formatter is statically persisted, by assigning an
   -    // arbitrary property and reading it back.
   -    $random_value = $this->randomString();
   -    $formatter->randomValue = $random_value;
   -    $formatter = $display->getRenderer($field_name);
   -    $this->assertEquals($random_value, $formatter->randomValue);
   +    // Check that the formatter is statically persisted.
   +    $this->assertSame($formatter, $display->getRenderer($field_name));
   

   As does this test.

3. +++ b/core/modules/field_ui/tests/src/Kernel/EntityFormDisplayTest.php
   @@ -103,20 +103,16 @@ public function testFieldComponent() {
   -    // Check that the widget is statically persisted, by assigning an
   -    // arbitrary property and reading it back.
   -    $random_value = $this->randomString();
   -    $widget->randomValue = $random_value;
   -    $widget = $form_display->getRenderer($field_name);
   -    $this->assertEquals($random_value, $widget->randomValue);
   +    // Check that the widget is statically persisted.
   +    $this->assertSame($widget, $form_display->getRenderer($field_name));
   

   And this test…

Re-reading the issue summary, I see See #3295821-91: Ignore: patch testing issue for PHP 8.2 attributes and Use classes instead of interfaces when creating mocks to make tests pass, but I don't yet see how I can reproduce those failures? 🤔🙈

All those should result in deprecation messages. Make sure that you have deprecations enabled in your phpunit.xml, otherwise you might not see them.

#40 I literally can't get it to trigger deprecation errors…

If I run \Drupal\Tests\contact\Unit\MailHandlerTest but make core/.deprecation-ignore.txt empty, I can see deprecation errors just fine.

I really wonder what I'm doing wrong 🙈

1.

+++ b/core/tests/Drupal/Tests/Core/Entity/EntityUnitTest.php
@@ -618,3 +619,11 @@ public function testCacheMaxAge() {
+class EntityBaseChild extends EntityBase {

+++ b/core/tests/Drupal/Tests/Core/Entity/EntityUrlTest.php
@@ -505,4 +505,12 @@ protected function registerBundleInfo($bundle_info) {
+class EntityBaseTest extends EntityBase {

+++ b/core/tests/Drupal/Tests/Core/Entity/KeyValueStore/KeyValueEntityStorageTest.php
@@ -625,6 +626,15 @@ public function getMockEntity($class = 'Drupal\Core\Entity\EntityBase', array $a
+class EntityBaseTest extends EntityBase {

So, one EntityBaseChild and two EntityBaseTest. Is it on purpose?

2. Taking another look at EntityBase class, and it just uses undefined properties like $id, for example:

  /**
   * {@inheritdoc}
   */
  public function id() {
    return $this->id ?? NULL;
  }

So, mocking those via a test subclass just hides the real issue, no? Am I missing something or those properties belong to EntityBase?

> So, one EntityBaseChild and two EntityBaseTest. Is it on purpose?

The problem is that loading all test classes (which phpunit does by default) will result in duplicate test classes otherwise. It's OK to have two duplicates because they live in a different namespace. But for consistency (or non-consistency if you will) we could also give all 3 of them a unique class name.

> So, mocking those via a test subclass just hides the real issue, no?

There is no real issue here. Entity types are expected to define the properties that they are "defining" (they are defined as part of mocking the entity type definition). Since we mock directly from the base classes and not real entity types, they don't have properties defined, so we have to do it.

There is one notable exception, and that's the original property for one of them. We work around that for config entities with the special annotation to allow undefined properties, but this is not a config entity. (and content entities rely on magic for the same). We can remove that again once #2839195: Add a method to access the original property is in.

EntityBase::id() is kinda weird, but I believe it does not cause actual issues because it uses ??. Those methods should maybe be abstract/and/or use entity keys, config entities currently have to override that method if they use a different property for the id.

But for consistency (or non-consistency if you will) we could also give all 3 of them a unique class name.

Yup, like make them all the same or three totally different :)

EntityBase::id() is kinda weird, but I believe it does not cause actual issues because it uses ??. Those methods should maybe be abstract/and/or use entity keys, config entities currently have to override that method if they use a different property for the id.

Okay, but I think all of the four id/uuid/langcode/label are being used by EntityBase => thus imo they should be declared there. For me, it feels that 8.2 has just unhidden an issue here, and this code will hide it again.

Alternatively, EntityBase should be refactored to use entity keys (or do this in a follow-up)

Okay, but I think all of the four id/uuid/langcode/label are being used by EntityBase => thus imo they should be declared there. For me, it feels that 8.2 has just unhidden an issue here, and this code will hide it again.

Alternatively, EntityBase should be refactored to use entity keys (or do this in a follow-up)

Yes and no. EntityBase is weird. But even if we change it to use entity keys or abstract (which would be an API change for subclasses), we still need subclasses that define those because we need them for these tests and we would set it to the same keys as they are implicitly set to right now

As mentioned earlier, I'm no fan at all of these unit tests, they are not testing much and have to mock a metric ton of stuff to test the little things that they do. But that's for another day.

setting to needs work to have different test class names, lets use something that matches the test for each.

I think #51 is because EntityBaseTest is declared twice in the same namespace:

core/tests/Drupal/Tests/Core/Entity/EntityUnitTest.php
623:class EntityBaseTest extends EntityBase {

core/tests/Drupal/Tests/Core/Entity/EntityUrlTest.php
508:class EntityBaseTest extends EntityBase {

Not sure why this isn't failing on PHP 8.2 though.

+++ b/core/modules/node/tests/src/Unit/NodeOperationAccessTest.php
@@ -74,7 +74,7 @@ public function testRevisionOperations($operation, array $hasPermissionMap, $ass
-    $node = $this->createMock(NodeInterface::class);
+    $node = $this->createMock(Node::class);

This test passes locally if I remove this entire block, which is the trigger for the deprecation:

    $nodeType = $this->createMock(RevisionableEntityBundleInterface::class);
    $typeProperty = new \stdClass();
    $typeProperty->entity = $nodeType;
    $node->type = $typeProperty;

Can we just remove this instead?

Otherwise all the changes look good. I reviewed the patch before reading all the comments and had the same question as @Taran2L about the EntityBase properties. I can see the argument both ways - id() and friends probably should be abstract, but we can't easily change that now, so fixing the tests is the easier thing to do. I also agree that the unit tests are mocking way too much.

Addressed #52; renamed the EntityUrlTest test entity, and removed what appears to be dead code from NodeOperationAccessTest.

1. +++ b/core/modules/system/tests/modules/entity_test/src/TypedData/ComputedString.php
   @@ -11,6 +11,13 @@
    class ComputedString extends TypedData implements CacheableDependencyInterface {
    
   +  /**
   +   * The data value.
   +   *
   +   * @var mixed
   +   */
   +  protected $value;
   

   🤔 Shouldn't this be set on \Drupal\Core\TypedData\TypedData instead, to ensure all abstract class TypedData subclasses get it? To ensure contrib/custom subclasses are fixed too?

   See \Drupal\Core\TypedData\TypedData::getValue() and the setter.

   It's pretty weird that these all define that property:

   1. \Drupal\Core\Config\Schema\Element::$value
   2. \Drupal\Core\TypedData\Plugin\DataType\Any::$value
   3. \Drupal\Core\TypedData\PrimitiveBase::$value
   4. \Drupal\layout_builder\Plugin\DataType\SectionData::$value
   5. \Drupal\Tests\Core\Plugin\Fixtures\Plugin\DataType\TestDataType::$value

   (And that's just core.)

   Ah … this is intentional:

    * Classes deriving from this base class have to declare $value
    * or override getValue() or setValue().
   

   … presumably to specify type hints with as narrowly a type as possible. 👍

   ALL GOOD! ✅

2. +++ b/core/tests/Drupal/Tests/Component/Utility/VariableTest.php
   @@ -162,7 +162,10 @@ public function providerTestExport() {
   +        // A not-stdClass object. Since PHP 8.2 exported namespace is prefixed,
   +        // see https://github.com/php/php-src/pull/8233 for reasons.
   +        PHP_VERSION_ID >= 80200 ?
   +        "\Drupal\Tests\Component\Utility\StubVariableTestClass::__set_state(array(\n))" :
   

   🤯

3. +++ b/core/tests/Drupal/Tests/Core/Entity/EntityUnitTest.php
   @@ -618,3 +619,11 @@ public function testCacheMaxAge() {
   +  public $label;
   +
   +}
   
   +++ b/core/tests/Drupal/Tests/Core/Entity/EntityUrlTest.php
   @@ -505,4 +505,12 @@ protected function registerBundleInfo($bundle_info) {
   +  public $label;
   +
   +}
   
   +++ b/core/tests/Drupal/Tests/Core/Entity/KeyValueStore/KeyValueEntityStorageTest.php
   @@ -625,6 +626,15 @@ public function getMockEntity($class = 'Drupal\Core\Entity\EntityBase', array $a
   +  public $original;
   +
   +}
   
   +++ b/core/tests/Drupal/Tests/Core/Form/ConfigFormBaseTraitTest.php
   @@ -69,3 +69,11 @@ public function testConfigFactoryExceptionInvalidProperty() {
   +  public $configFactory;
   +
   +  protected function getEditableConfigNames() {}
   +
   +}
   

   🤔 Übernit: why these empty lines?

I don't think that last point should prevent this from getting committed 🤓 Leaving the RTBC honor to @Berdir, who knows this problem space far better 😊

Re #56.1 similar to the EntityBase properties discussed above, TypedData assumes $this->value for convenience but you do not have to use it. For example EntityAdapter declares $entity instead and overrides getValue() to return it.

Re #56.3 this is a PHPCS rule, without the blank lines you get

 628 | ERROR | [x] The closing brace for the class must have an empty line before it

AFAIK our coding standards requires an empty line before the closing } of a class, no? (crosspost with #57 which confirmed that)

I feel like I have worked a bit too much on this myself to RTBC, but others have reviewed it too including the parts that I did, so it's probably OK?

This looks good to me, really nice to be all green before a stable release especially given the number of non-trivial changes to deal with in PHP 8.2

Committed/pushed to 10.1.x and cherry-picked to 9.5.x, thanks!