Use of @internal in RenderElementBase property attributes yields phpstan errors

Personally I think it is fine as is for a couple of reasons.

1. That is a warning that can be ignored by the code extending it, that is precisely the point of @internal as far as I see it. (Also why I advocate against final so strongly, if this had been final you wouldn't be able to extend it. At least here you have a choice and get a warning)
2. Level 8 is significantly higher than most people are running php stan, what level do you start getting earnings about @internal.

#2.1 That's not the point, RenderElementBase is NOT internal, only the property has_garbage_value is, but because of how the PHPDocs were written it is being parsed as the whole class being internal.

#2.2 Project Browser runs on Level 8, and also provides a Render Element, and will start failing on this.

I was thinking of:

  /**
   * The storage.
   *
   * @internal
   */
  protected array $storage = [];

But this seems to be about the properties documented on the class itself.

PHPStan interpreting the property being set to internal as the whole class seems like a bug in phpstan, probably worth an upstream issue to resolve.

In the short term I think we can do one of two things.

1. Change it back to how we defined it as internal before with just the comment Internal
2. Move that code block somewhere else in the class until they fix the upstream issue.

I would strongly prefer 2 since that property is internal.

Regarding your second point (separately from the bug that the whole class is being interpreted as internal), my point wasn't that other modules don't use level 8, but when it comes to @internal and @final phpstan is more about notifying you rather than something you have to fix.

In my mind if phpstan throws an error for something like a variable is not defined, that is something you need to address because it will throw errors.

On the other hand when phpstan says something like you're extending @internal code, you have to look and evaluate if you're willing to take on the risk that entails, but it's not a bug in your code, phpstan is just warning you here. The solution downstream is to put an ignore rule acknowledging that risk.

One is about potential bugs in your code, the other is a warning to ensure you did something intentionally.

The same problem is now with FormElementBase:

------ -------------------------------------------------------------------------------------------------------------------------------------
Line src/Element/ToolsLibrary.php
------ -------------------------------------------------------------------------------------------------------------------------------------
28 Class Drupal\ai\Element\ToolsLibrary extends @internal class
Drupal\Core\Render\Element\FormElementBase.
🪪 classExtendsInternalClass.classExtendsInternalClass
💡 Read the Drupal core backwards compatibility and internal API
policy:
https://www.drupal.org/about/core/policies/core-change-policies/drupal-8... →
------ -------------------------------------------------------------------------------------------------------------------------------------

I have this issue now in AI Core module pipeline with level 1.

Just chiming in to confirm the bug is affecting Commerce tests as well, we have 5 fresh PHPSTAN errors due to that:

Class Drupal\commerce_price\Element\Number extends @internal class
Drupal\Core\Render\Element\FormElementBase.
🪪 classExtendsInternalClass.classExtendsInternalClass

Class Drupal\commerce_price\Element\Price extends @internal class
Drupal\Core\Render\Element\FormElementBase.
🪪 classExtendsInternalClass.classExtendsInternalClass

See https://git.drupalcode.org/project/commerce/-/jobs/5854079.

As a temporary workaround I have added the following lines to phpstan config:

ignoreErrors:
# new static() is a best practice in Drupal, so we cannot fix that.
- "#^Unsafe usage of new static#"
-
message: "#extends @internal class Drupal\\\\Core\\\\Render\\\\Element\\\\FormElementBase#"
reportUnmatched: false

I created an issue in PHPStan: https://github.com/phpstan/phpstan/issues/13242

The @internal annotation seems to be for classes only, there isn't such a thing for properties AFAIK.

I wonder if we should keep the non-internal properties in the PHPDoc and move the internal ones out of it and declare those as private or protected variables instead?

Moved them down to protected properties so we can see what that looks like.

A protected property that is considered @internal should probably be private instead? Whether we can change this now, however...

Moved them down to protected properties so we can see what that looks like.

This seems like it should be the answer but it actually doesn't make sense. They are never accessed like properties. They shouldn't be annotated as @property nor should they be defined as a property, because at the times they are accessed we only have an array.

For example, see \Drupal\Tests\Core\Render\Element\RenderElementTest::testPreRenderAjaxForm. That passes an array to \Drupal\Core\Render\Element\RenderElementBase::preRenderAjaxForm, and in that function we look at $element['#has_garbage_value']. Putting a property on the class doesn't do anything because we don't have an object, we have an array.

This just demonstrates that we're abusing @property annotations to document the Array PI keys. This is consistent with the doc comment right above it:

 * Here is the list of the properties used during the rendering of all render
 * elements. These are available as properties on the render element (handled
 * by magic setter/getter) and also the render array starting with a #
 * character. For example $element['#access'] or $elementObject->access.

For the record, yes I understand that these keys can be accessed as properties using the magic getter, but similarly you could access $obj->foo and $obj->bar, and they are not documented. Unfortunately I don't have a better suggestion for where to document them.