Skip to content

Add stub for \Drupal\Core\Field\FormatterInterface #822

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

Merged
merged 2 commits into from
Mar 27, 2025

Conversation

Niklan
Copy link
Contributor

@Niklan Niklan commented Jan 24, 2025

With the PHPStan level 7 every field formatter starts to reports an error:

  XX     Method Drupal\foo\Plugin\Field\FieldFormatter\FooFormatter::viewElements() has parameter $items with generic interface Drupal\Core\Field\FieldItemListInterface but does not  
         specify its types: T                                                                                                                                                                                                   
         🪪  missingType.generics  

This PR adds a stub for the method that defines each element of $items as an instance of \Drupal\Core\Field\FieldItemInterface.

I'm not sure why the FieldItemListInterface doesn't handle this case. I'm not sure if my solution is the correct one in this case. Maybe it is possible to improve FieldItemListInterface.stub

@mglaman
Copy link
Owner

mglaman commented Mar 27, 2025

We have to do the same in FieldItemListInterface for get

@mglaman mglaman merged commit e27ce97 into mglaman:main Mar 27, 2025
13 of 14 checks passed
mglaman added a commit that referenced this pull request Mar 27, 2025
Add FormatterInterface.stub

Co-authored-by: Matt Glaman <[email protected]>
(cherry picked from commit e27ce97)
@ceesgeene
Copy link
Contributor

I don't think this PR is correct. Phpstan wants you to specify the generic type. Repeating the interface as a phpdoc param just hides/suppresses the phpstan finding/error.

So e.g. for a link filed formatter in your codebase, you could add:

  /**
    * {@inheritDoc}
    * 
    * @param \Drupal\Core\Field\FieldItemListInterface<\Drupal\link\Field\LinkItem> $items
    */
   public function viewElements(...

This way phpstan knows for example that $item->uri exists.

I think this PR should be reverted. We have serveral Drupal projects (with phpstan level 9) that use this but since this commit their phpstan starts reporting the following error (twice per encounter, not sure why):

Parameter #1 $items (Drupal\Core\Field\FieldItemListInterface<Drupal\***\Plugin\Field\FieldType\***Item>)  
of method Drupal\custom_***\Plugin\Field\FieldFormatter\***Formatter::viewElements()
should be compatible with parameter $items (Drupal\Core\Field\FieldItemListInterface<Drupal\Core\Field\FieldItemInterface>)
of method Drupal\Core\Field\FormatterInterface::viewElements()

This error is incorrect because they are compatible. I think this error is reported because the stub file incorrectly repeats the interface of the generic type.

@Niklan
Copy link
Contributor Author

Niklan commented Mar 29, 2025

I've played more with PHPStan generics recently. I think @ceesgeene is right — it better be reverted. Sorry, I've just started to dig into PHPStan extending and might be doing something wrong. :(

But I still think we need that stub as a generic, so it forces other field formatters to specify their field item types. What do you think?

<?php

namespace Drupal\Core\Field;

/**
 * @template TFieldItemList of \Drupal\Core\Field\FieldItemListInterface
 */
interface FormatterInterface {

  /**
   * @param TFieldItemList $items
   * @param string $langcode
   *
   * @return array<int, array<int|string, mixed>>
   */
  public function viewElements(FieldItemListInterface $items, $langcode): array;

  /**
   * @param TFieldItemList $items
   * @param string $langcode
   *
   * @return array<int|string, mixed>
   */
  public function view(FieldItemListInterface $items, $langcode = NULL);

}

This way, field formatters must explicitly set types for field item lists:

/**
 * @implements FormatterInterface<StringItemList>
 */
class FooFormatter implements FormatterInterface {

But if the field is using another generic FieldItemList, they must be able to specify the type for an item:

/**
 * @implements FormatterInterface<FieldItemList<MyCustomItem>>
 */
class FooFormatter implements FormatterInterface {

If you folks think this should be done, I can prepare another issue/PR to discuss it.

@Niklan Niklan deleted the formatter-interface-stub branch March 29, 2025 05:18
Niklan added a commit to Niklan/phpstan-drupal that referenced this pull request Mar 29, 2025
Niklan added a commit to Niklan/phpstan-drupal that referenced this pull request Mar 29, 2025
@ceesgeene
Copy link
Contributor

Yes, making the FormatterInterface generic would even be better.

@Niklan
Copy link
Contributor Author

Niklan commented Mar 29, 2025

@mglaman and @ceesgeene

I've made PRs for both suggestions, which basically conflict with each other and depend on the decision of which way to go.

  1. Revert FormatterInterface stub:
  2. Convert FormatterInterface into a proper generic:

Personally, I think we should make it a proper generic. The stricter the code, the better, in my opinion. This generic will force formatters to declare their list item types and item types in some cases.

@ceesgeene
Copy link
Contributor

I would revert first and then work on the generic part. The latest version of phpstan-drupal is reporting incorrectly now.

mglaman pushed a commit that referenced this pull request Apr 3, 2025
)

Revert "Add stub for \Drupal\Core\Field\FormatterInterface (#822)"

This reverts commit e27ce97.
mglaman pushed a commit that referenced this pull request Apr 3, 2025
)

Revert "Add stub for \Drupal\Core\Field\FormatterInterface (#822)"

This reverts commit 40ddc27.
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.

3 participants