Problem/Motivation
Spinoff of #3274474: Fix 'Access to an undefined property' PHPStan L0 errors, for test code only.
Steps to reproduce
Proposed resolution
Remaining tasks
User interface changes
API changes
Data model changes
Release notes snippet
| Comment | File | Size | Author |
|---|---|---|---|
| #29 | 3281535-29-94x.patch | 58.05 KB | spokje |
| #29 | raw_diff_27-29.txt | 2.05 KB | spokje |
| #27 | 3281535-27-95x.patch | 58.07 KB | mondrake |
Issue fork drupal-3281535
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:
- 3281535-fix-access-to
changes, plain diff MR !2295
Comments
Comment #2
mondrakeOn this
Comment #4
mondrakeHas the execution time of
phpstan analyze --generate-baselinemore than tripled for me only? Since latest updates of phpstan and phpstan-drupal.Not sure what to do about typehinting e.g. User vs UserInterface - in runtime code we'd certainly be using the interfaces, but in tests?
Comment #5
mondrakeComment #6
mondrakeComment #7
mondrakeReady for review.
Comment #8
mondrakeComment #9
mallezieDid an initial review.
Changes look correct, and in scope of only test classes. (Did not check if we have all of them here, but we're removing 150+ instances, so I would not block of missed some in test classes here).
Only question from my side (but could be wrong here) is that we're a bit inconsistent in using classes vs interfaces here. (In some tests we add User vs UserInterface, same for Node vs NodeInterface). Would not block on that, just wondering if there is a 'standard' we should use.
Needs rebase however.
Comment #10
mondrakeRe #4, #9, asked for guidance in Slack
Comment #11
andypostComment #12
mondrakeConsistently use concrete classes for entities
Comment #13
daffie commentedJust 1 nitpick. After that it is RTBC for me.
Comment #14
mondrakeComment #15
daffie commentedAll code changes look good to me.
All change are for tests only.
For me it is RTBC.
Comment #16
mondrakerebased
Comment #17
alexpottWe should backport the test changes here. For PHP 8.2 compatibility and to make backports easier.
Committed 6dd0db5 and pushed to 10.0.x. Thanks!
Comment #19
mondrakeon that
Comment #21
mondrakeHere's a 9.5.x and 9.4.x patch.
Comment #22
mondrakeComment #24
mondrakeComment #25
daffie commentedThe changes are the same ones as the ones for D10, only without the added parameter typehints.
For me it is RTBC.
Comment #26
alexpottWe need to account for #3291018: Move \Drupal\Tests\ckeditor5\Functional\CKEditor5QuickEditLibraryTest to the quickedit namespace/directory - error: core/modules/ckeditor5/tests/src/Functional/CKEditor5QuickEditLibraryTest.php: does not exist in index - so the test has moved...
Comment #27
mondrakeRerolled and adjusted for #26
Comment #28
mondrakeA 9.4.x port will require another patch again.
Comment #29
spokjeAdded patch for
9.4.x.Comment #30
alexpottCommitted 76ca2c3 and pushed to 9.5.x. Thanks!
Committed e71bebf and pushed to 9.4.x. Thanks!
Keeping tests aligned.