Problem/Motivation

Amongst the current issues found in the PHPStan level 1 baseline is: Access to an undefined property Foo\Bar::$baz.

This issue exists to fix all of those, except for the ones listed below.

Steps to reproduce

- Run PHPStan on level 1 and see the above issue amongst all others.

Proposed resolution

- Solve all of the reported issues for the above mentioned.
- Run PHPStan on level 1 and don't see the above issue any more, except for the following instances that are either a bit more complicated and are split off into separate issues, or are already handled elsewhere.

   message: "#^Access to an undefined property Drupal\\\\Core\\\\TypedData\\\\TypedData\\:\\:\\$value\\.$#"
   count: 2
   path: lib/Drupal/Core/TypedData/TypedData.php

   message: "#^Access to an undefined property Drupal\\\\Core\\\\Entity\\\\EntityDisplayBase\\:\\:\\$_serializedKeys\\.$#"
   count: 2
   path: lib/Drupal/Core/Entity/EntityDisplayBase.php

- Handled in #3419114: Convert FileTransferTest into a Kernel test

   message: "#^Access to an undefined property Drupal\\\\Tests\\\\system\\\\Functional\\\\FileTransfer\\\\TestFileTransfer\\:\\:\\$connection\\.$#"
   count: 5
   path: modules/system/tests/src/Functional/FileTransfer/TestFileTransfer.php

Also not fixed are all nine suppressions for this error in lib/Drupal/Component/Diff/Engine/DiffEngine.php.
That class is already deprecated and will be removed in D11, after which the entries in the PHPStan baseline can also be removed.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3347502

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

Spokje created an issue. See original summary.

Spokje opened merge request !3638

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Should we consider adding DiffEngine to excludePaths?

spokje’s picture

spokje
Primary language Dutch
commented

Should we consider adding DiffEngine to excludePaths?

Pondered about that myself.
Decided against it, because it's not really "visual" for the average user that the file is excluded.
If only PHPStan had something like @phpstan-ignore-file...

Anyway, that's just my personal opinion, happy to be convinced to go the excludePaths route :)

mondrake’s picture

mondrake
Location 🇮🇹
commented

Should we consider adding DiffEngine to excludePaths?

I would suggest no. In D11 the file will be gone, anyway. In the meantime, any refactoring introducing more errors will be at least pointed out. That applies to any deprecated file, imho.

spokje’s picture

spokje
Primary language Dutch
commented
Assigned: Unassigned » spokje
Status: Active » Needs work
spokje’s picture

spokje
Primary language Dutch
commented
Title: Fix PHPStan L1 errors "Access to an undefined property Foo\Bar::$baz" » Fix PHPStan L1 errors "Access to an undefined property Foo\Bar::$baz" (Low Hanging Fruit Edition)
spokje’s picture

spokje
Primary language Dutch
commented
Issue summary: View changes
spokje’s picture

spokje
Primary language Dutch
commented
Issue summary: View changes
spokje’s picture

spokje
Primary language Dutch
commented
Issue summary: View changes

Spokje opened merge request !3775

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

spokje’s picture

spokje
Primary language Dutch
commented
Assigned: spokje » Unassigned
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Lots of places we can drop @var following #3238192: Allow omitting @var for strictly typed class properties

ankithashetty’s picture

ankithashetty
Location Karnataka, India
commented

Hello @longwave, noticed in #3238192: Allow omitting @var for strictly typed class properties that we can omit @var only for strictly typed class properties.
If we look at the Proposed resolution section, it says "This type can not be represented in the PHP typesystem yet." to variables like @var int|string|NULL 

class Foo {
  /**
    * Where one can order a soda.
    */
  protected Bar $baz;

  /**
    * Some complex type that probably needs cleaning up in a future.
    * 
    * This type can not be represented in the PHP typesystem yet.
    *
    * @var int|string|NULL
    */
   public $someVar;
}

So can we really remove @var here:

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Anywhere that we do declare the types on the following lines, we can remove @var, for example:

  /**
   * The connection chroot.
   *
   * @var string|bool
   */
  private string|bool $chroot;

Not so sure about

protected Connection|false $connection;

as I think |false is only valid in PHP 8.2 and we need to support PHP 8.1.

ankithashetty’s picture

ankithashetty
Location Karnataka, India
commented

Can we create a new MR for 11.x instead of changing the target branch in the existing MR?

spokje’s picture

spokje
Primary language Dutch
commented

Can we create a new MR for 11.x instead of changing the target branch in the existing MR?

Ah, wasn't aware you were working on this at the moment. Put MR back to 10.1.x.
Feel free to change whatever you need, I'll be leaving this one alone.

ankithashetty’s picture

ankithashetty
Location Karnataka, India
commented

Thanks! 🙂 Was just thinking about working on #14 suggestions.

ankithashetty’s picture

ankithashetty
Location Karnataka, India
commented

In MR !3775 (against 10.1.x), if the change is not as expected, we can just revert the latest commit.

Spokje changed the visibility of the branch 3347502-fix-phpstan-l1 to hidden.

Spokje changed the visibility of the branch 10.1.x to hidden.

Spokje changed the visibility of the branch 11.x to hidden.

Spokje opened merge request !6436

spokje’s picture

spokje
Primary language Dutch
commented
Assigned: Unassigned » spokje

Spokje closed merge request !3775

Spokje changed the visibility of the branch 3347502-low-hanging-fruit to hidden.

spokje’s picture

spokje
Primary language Dutch
commented
Issue summary: View changes
Related issues: +#3419114: Convert FileTransferTest into a Kernel test
spokje’s picture

spokje
Primary language Dutch
commented
Issue summary: View changes

quietone made their first commit to this issue’s fork.

quietone’s picture

quietone commented
Status: Needs work » Needs review
spokje’s picture

spokje
Primary language Dutch
commented
Assigned: spokje » Unassigned
smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Small comments on the MR.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs work » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Believe all feedback for this one has been addressed

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs work

Saving credit. Needs a rebase.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs work » Reviewed & tested by the community

Rebased.

  • catch committed ee1c3991 on 11.x 
    Issue #3347502 by spokje, longwave, ankithashetty, quietone, smustgrave...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 11.x, thanks!

catch closed merge request !6436

Status: Fixed » Closed (fixed)

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