Problem/Motivation

PHPStan baseline is currently skipping multiple Call to an undefined method errors.

Proposed resolution

Fix DataReferenceBase errors, clean up the baseline.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mondrake created an issue. See original summary.

mondrake’s picture

mondrake’s picture

Status: Active » Needs review
longwave’s picture

Can we just change getString() now, given this is not actually implemented in core?

mondrake’s picture

An external class calling getString() right now in HEAD would lead to fatal, no? So what should getString() return, really? It is public API but quite evidently not called by anything at the moment. Maybe we can just deprecate it?

longwave’s picture

We could just delete the getString() method from DataReferenceBase, and fall back to the parent implementation?

mondrake’s picture

FileSize
1.18 KB

Good idea!

longwave’s picture

Status: Needs review » Reviewed & tested by the community

Works for me, given that the existing method would cause a fatal error.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Something could extend DataReferenceBase in contrib and then implement getType(). This is not that farfetched because DataReferenceBase is abstract.

I think we should do something like:

  /**
   * {@inheritdoc}
   */
  public function getString() {
    if (!method_exists($this, 'getType')) {
      throw new \BadMethodCallException(__CLASS__ . '::getType() not implemented');
    }
    return (string) $this->getType() . ':' . $this->getTargetIdentifier();
  }

We can add a test for this in \Drupal\KernelTests\Core\TypedData\TypedDataDefinitionTest::testDataReferences as we have \Drupal\Core\TypedData\Plugin\DataType\LanguageReference in core that extends but does not implement this method.

mondrake’s picture

Status: Needs work » Needs review

I am 97% that #9 won’t work for PHPStan because it will still report the missing method. IMHO the best option is to go back to #2 here, then.

longwave’s picture

@mondrake I am not so sure: https://github.com/phpstan/phpstan/issues/323 implies that method_exists() checks inside the same method will work.

mondrake’s picture

Assigned: Unassigned » mondrake
Status: Needs review » Needs work

Let’s try then! Thx

mondrake’s picture

Assigned: mondrake » Unassigned
Status: Needs work » Needs review
Issue tags: +Needs tests
FileSize
1.29 KB
mondrake’s picture

Issue tags: -Needs tests
FileSize
2.75 KB
1.46 KB

Adding a test as suggested in #9.

longwave’s picture

Status: Needs review » Reviewed & tested by the community

Looks great!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 4695b21 and pushed to 10.1.x. Thanks!
Committed 5615ede and pushed to 10.0.x. Thanks!

  • alexpott committed e00c523 on 10.1.x
    Issue #3317651 by mondrake, longwave, alexpott: Fix DataReferenceBase...

  • alexpott committed 5615ede on 10.0.x
    Issue #3317651 by mondrake, longwave, alexpott: Fix DataReferenceBase...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.