🇮🇹Italy @mondrake

rebased

Reviewable again.

Stricter typing is highlighting a bug. On that.

Changes to return typehints were OK for CodeSniffer, good - but now this needs review again I suppose, sorry @daffie

I'm still trying to do some tuning with the typehints in PdoTrait - to make them as accurate and PHPStan-proof as possible. It may turn out that CodeSniffer will be unhappy, in which case I think we should silence its failures.

Thanks for review @daffie!

Juggled a bit with typehints.

Follow up: 📌 [PP-1] Introduce a StatementBase abstract class Postponed , to create a common base class for statement objects.

mondrake → created an issue.

Updated CR to reflect the new method is now a protected method of the Schema class.

Working on latest comment.

Merged, thanks!

Crediting

@chandansha thanks! The fix is correct, but there is a phpcs issue (extra white line) to be fixed.

The gain I see is to be able to test future versions of PHP through matrix (and potentially PHPUnit, too) on unit tests only, earlier than the rest of the test suite, and highlight deprecations in advance without failing overall testing job.

For example, PHP 8.4 is going to be delivered the day after tomorrow, and there is not yet a regular job testing with it AFAICS.

Not until upstream gets #9 done, I am afraid.

#7 it's replaced by PhpUnitTestDiscoveryTest , which is a kernel test that does the same thing (call PHPUnit CLI to get the test list), but needs services to build Drupal's list to compare.

#28 actually that's true: https://www.drupal.org/project/drupal/issues/3445106#comment-15587547 📌 Replace @dataProvider annotations with #[DataProvider()] attributes Postponed you can have attributes and annotations in the same test in PHPUnit 9, apparently. Need probably to decide what to use where though, to avoid duplication and possible mistakes.

Just noticed CR is there already, https://www.drupal.org/node/3447698; → please review

@catch

backport this to 10.x

can't do that alas - attributes are a PHPUnit 10+ thing, and D10 is testing with PHPUnit 9.

should we have a follow-up to try to use phpunit's discovery or did you end up deciding that's too far off

1. Must do prerequisite is 🐛 Ensure run-tests.sh and PHPUnit CLI run with the same list of tests to be executed Active to make sure we are testing the same things
2. It's not far ( 📌 Refactor PhpUnitTestRunner and TestRun internal classes Active latest revision is doing that already), but it's ... slow. The logic in that issue is to run PHPUnit's CLI to build the test list in a xml file, then parse that file to build the internal list in PHP. That takes time, also considering that PHPUnit invokes dataproviders to build the full testcases list. It's not dramatic in CI context but locally the difference is visible.

mondrake → created an issue.

mondrake → created an issue. See original summary → .

Changed the comment since my earlier suggestion duplicated the word 'because' and was not compliant to the 80 chars line limit.

I think I can leave that to RTBC.

mondrake → changed the visibility of the branch 3487488-extensionmimetypeguesserguessmimetype-must-support to hidden.

Shouldn't we then enable the checkInternalClassCaseSensitivity parameter in phpstan.neon so to prevent regressions?

Looks good. Test coverage added.

one more nitpicking comment

couple of comments inline

👏 great advancement!

My 2 cents.

I've just run analysis with level 0 and no baseline - we still have 230+ issues there, over 2 years since the introduction of PHPStan. We discussed to introduce level 2 during the beta window of D11.0, and missed that. No action happened since then, we are in beta window for D11.1 now and will therefore miss that chance here again.

Recently since the addition of the function/method return type check rules (I think they're level 5 or 6 rules, normally), the baseline grew so big that lots of MRs require continuous rebasing to catch up with its changes (can you imagine if we were bumping to level 9 as discussed somewhere else?).

IMHO before looking at adding more rules (phpstan-strict-rules has, btw, 'opinionated' rules, that will require discussion to use or not and possibly have coding standards impacts), we should still work on basics, cleanup existing baseline, bump one level at a time, reiterate. And there's very little focus on that to be honest.

in one of those 'after' images, we see a block test marked red. Do we need to file an issue to fix that test?

Aha 😀

Those screenshots actually captured a random test failure… those won’t go away with this MR 😀

Uh-oh. Thanks, indeed a leftover while checking something else. Strange test failure though. Let’s get this in and sort out if anything broken, then release again.

Merged.

Thanks @dhruv.mittal! Some of the changes are questionable (constructor description, capital sensitive ordering of use imports), but it's a problem with the coding standards - your work is correct.

Crediting.

@mile23 the tests were not ignored, actually. They were reported as if they passed, which is the original reason of this issue - and a cause of confusion.

PHPUnit tests can be marked as ‘skipped’ or ‘ignored’ in the test code, but both end up as ‘skipped’ in the JUnit log - that’s a JUnit limitation AFAICS.

I do not think there’s a need for generic followups to unskip tests - that’s very much dependent on the use case and should be decided case by case. For example, database driver specific kernel tests are prepared for all drivers all times, but then those of the drivers that are not of the currently running database get skipped. By design.

adding before/after screenshots

rebased

@longwave re #10 I really believe 🐛 Allow run-tests.sh to report skipped/risky/incomplete PHPUnit-based tests Needs work would be a step change for run-tests.sh output.

I will rebase it now and add before/after screenshot there as well.

This is kinda similar to 📌 [WIP] Decouple identifier management from database Connection, introduce an IdentifierHandler class Postponed .

This needs a mglaman/phpstan-drupal that works with phpstan/phpstan:^2.0

Postponed on that.

Reverted the change made on purpose in #3 to demo the outcome.

Rebased.

Added before and after screenshots - it's not the same test but they give pretty well the sense of the change IMHO.

Before

After

The status of the random test failures is, alas, rather unfortunate. Cannot remember not having to kick back test runs manually in the last month or so in any MR, especially if not MySql. Anyway, after playing some whacamole it's green on MySql and Sqlite now.

That one is the composer.json of the PHPStan rules testing, not core's. That is separate from the rest to allow independent testing from core. Being independent, it's also not bound to core's dependencies and it happens PHPUnit 11 can be used for this without any of the hurdles that core has: 📌 [PP-1] Make PHPStan rule testing use PHPUnit 11 Postponed . Here's is not strictly necessary, but I made that change to 'force' the PHPStan rule testing job to run as that job is based on changes in the PHPStan rules directory.

mondrake → created an issue.

Reverted changes to code that were meant to showcase the rule. The example pipeline were the PHPStan output is visible is https://git.drupalcode.org/issue/drupal-3486376/-/pipelines/333846

Added PHPUnit tests for the PHPSTan rule.

Needs tests for the rule and some cleanup, but I am opening it for review of the concept.

This shows how adding the @return-type-will-be-added to some methods of ImageInterface, PHPStan identifies that the implementing methods on the concrete Image class need to get the return type.

Opened 📌 Introduce a @return-type-will-be-added annotation to indicate return type addition in next major; enforce with a PHPStan rule Active to approach this with a PHPStan custom rule.

mondrake → created an issue.

mondrake → created an issue.

From https://linux.die.net/man/8/sudo, emphasis mine:

-H' The -H (HOME) option requests that the security policy set the HOME environment variable to the home directory of the target user (root by default) as specified by the password database. Depending on the policy, this may be the default behavior.

Tests pass (apart from the usual bunch of random fails in functional ones), so it seems to be working. Setting back to RTBC.

@murz that seems sensible; let’s try it and see if it works

rebased

@daffie couple comments inline

Fixed in 🐛 Fix ‘risky’ tests Fixed

mondrake → created an issue.

Removed #7.

rebased

Latest commit to the MR adds a job to run tests for Drupal components directly in PHPUnit, including code coverage report.

Probably needs to be spun off to a follow up, just wanted to showcase it.

plain PHPUnit: 1 min 54 secs
paratest: 24 secs
run-tests: 31 secs

Added parallel:matrix to be able running unit test on multiple PHP versions.

This is highligthing PHP 8.4 deprecations, for instance.

mondrake → created an issue.

is backport required for 10.5.x now, 10.4.x, or both?

I want to explore an alternative approach here, i.e. split the Test stage between unit tests and db-dependent tests. Let’s hold this for a bit longer.

mondrake → created an issue. See original summary → .

This was fixed in the related issue.

I'd be curious to know why this only failed on pgsql given the fix, and not on mysql and sqlite

@larowlan

Is there something we can enable in our phpstan config or ignore entries we can remove here to prevent this coming back.

that would be 📌 Bump PHPStan rule level to 2 Active

rerolled

working on this

To review this, you need to look at the raw output of a test of a core CI job.

For the sake of demonstrating the outcome, I broke a test so that its execution log can be dumped to the raw log.

See https://git.drupalcode.org/issue/drupal-3483931/-/jobs/3175261/viewer and search for 'PHPUnit 10' to see the result of the change.