Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make FormatterInterface stub generic (2.x) #846

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

Niklan
Copy link
Contributor

@Niklan Niklan commented Mar 29, 2025

From #822: This PR changes FormatterStub to be a proper generic instead of narrowing types for others.

Conflict with #844.

@Niklan Niklan changed the title Make FormatterInterface stub generic Make FormatterInterface stub generic (2.x) Mar 29, 2025
@Niklan
Copy link
Contributor Author

Niklan commented Mar 29, 2025

@ceesgeene, if possible, please take a look and give feedback on the generic proposal.

@Niklan Niklan marked this pull request as draft March 29, 2025 09:09
@Niklan
Copy link
Contributor Author

Niklan commented Mar 29, 2025

The only problem I faced is that I can't define these generic subtypes using a docblock above the class.

/**
 * @implements FormatterInterface<CommentFieldItemList>
 */

This leads to an error: Class {class} has @implements tag, but does not implement any interface, because it actually extends FormatterBase, which implements the FormatterInterface. This appears to make it impossible to declare generics TFieldItemList for both methods at the same time:

Method {formatter}::viewElements() has parameter $items with generic interface Drupal\Core\Field\FieldItemListInterface but does not specify its types: T

This forces us to add types directly at the method level:

/**
 * @param CommentFieldItemList $items
 */
public function viewElements(FieldItemListInterface $items, $langcode): array {

Then it works. But in that case, it's unclear what the generic is actually doing. You can simply remove it and get the same result.

So it appears that we also should provide stub for FormatterBase:

<?php

namespace Drupal\Core\Field;

use Drupal\Core\Plugin\ContainerFactoryPluginInterface;
use Drupal\Core\Field\FormatterInterface;
use Drupal\Core\Field\PluginSettingsBase;

/**
 * @template TFieldItemList of \Drupal\Core\Field\FieldItemListInterface
 * @implements FormatterInterface<TFieldItemList>
 */
abstract class FormatterBase extends PluginSettingsBase implements FormatterInterface, ContainerFactoryPluginInterface {

}

In that case it is possible to tell PHPStan what types we are using:

/**
 * @extends FormatterBase<CommentFieldItemList>
 */
final class FooFormatter extends FormatterBase {

It also looks like it's working well with generics as a type:

/**
 * @extends FormatterBase<FieldItemList<StringItem>>
 */
final class FooFormatter extends FormatterBase {

@Niklan
Copy link
Contributor Author

Niklan commented Mar 29, 2025

@mglaman how to handle these test failures? It means we also should add stubs for ContainerFactoryPluginInterface and PluginSettingsBase or add them into baseline?

@mglaman
Copy link
Owner

mglaman commented Mar 29, 2025

@Niklan it means we need the missing stubs added. Making a stub requires providing stubs for any extends or implementation, even if they're empty. See https://github.com/mglaman/phpstan-drupal/blob/main/stubs/Drupal/Component/Plugin/PluginInspectionInterface.stub as an example

@Niklan
Copy link
Contributor Author

Niklan commented Mar 30, 2025

@mglaman this one and #847 (1.x backport) are ready for review. I think there is nothing I can do about test failures in HEAD baseline check action.

If you are planning to merge these two, then these should be closed:

@Niklan Niklan marked this pull request as ready for review March 31, 2025 05:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants