🇮🇹Italy @mondrake

Nits added inline to the MR.

mondrake → created an issue.

mondrake → created an issue.

rebased

rebased

didn’t mean to drop support to 5 - not here anyway

will revert

#21 yes it’s another’s issue discussion that one. I’ll open one in due time.

mondrake → created an issue.

#19 that needs a separate issue, will open one now.

@andypost did you force push your local branch to the MR branch?

Locked versions, since there's a bug somewhere in earlier versions that causes failures in 📌 Use specific PDO drivers instead of PDOConnection Postponed .

Temp merged the MR with the MR in 📌 Update PHPStan to 2.1.19 Active .

One notch up.

https://github.com/sebastianbergmann/phpunit/releases/tag/11.5.39

This is blocking PHP 8.5 support, specifically 📌 Use specific PDO drivers instead of PDOConnection Postponed .

The PHPStan job failure is likely due a to a PHPStan bug - we are on 2.1.17, and locally I can get it pass on 2.1.25. So let's postpone here on 📌 Update PHPStan to 2.1.19 Active .

mondrake → made their first commit to this issue’s fork.

rebased

If 📌 [PP-1] Convert all transactions to explicit COMMIT Postponed fixes the issue, I would not pursue this. Under normal circumstances, there should not be the need to check that the client connection is active.

mondrake → created an issue.

I think we should seriously consider removing PHPStan and the extensions we use from composer.json, and let it install freely the latest releases during CI jobs.

Second thinking. Isn't there a risk of endless looping by allowing a post-transaction callback to start another transaction of its own? That second transaction may register post-transaction callbacks again, and so forth. Don't think this was considered in the original intent for introducing post-transaction callbacks.

I think this needs some thinking/agreement on what should be the behavior we have to support.

critical while head testing is broken

mondrake → created an issue.

Actually, that is on purpose, as it's the current behavior with commit-on-destruct. Even if extremely edge, in the current scenario you may have db operations between ::commitOrRelease() and ::destruct(), and the callback rely on those.

I was planning to do that as a follow-up of 📌 [PP-1] Convert all transactions to explicit COMMIT Postponed , see plan in 🌱 [meta] Drop implicit autocommit on Transaction destruction Active , point 4 in the IS; but of course, it can be put forward if we decide so.

A bit counterintuitive that a ‘post-transaction callback’ starts a transaction of its own but yeah, life is difficult.

Can we have the same test also doubled so to use commitOrRelease instead of unset? So we can make the ones using unset to be deprecation tests when converting all transaction to explicit commit.

Changed https://www.drupal.org/node/3512006 → which indeed was published pointing to 11.3.0 whereas it should refer to 11.2.0

@alexpott this issue did, but 📌 [PP-1] Convert all transactions to explicit COMMIT Postponed is still wip. So the edit of https://www.drupal.org/node/3524461 → is incorrect, actually. Not reverting it since it’s still a draft.

Note that the deprecation for removal in Drupal 11.4 is on purpose.

The point here is that in order to bump PHPUnit to 12, this issue is a must (unless we get another solution from upstream, which on PHPUnit 11 that we're currently using it less and less likely).

So if we were to remove only in D12, this would mean that D12 would ship with PHPUnit 12 i.e. a version whose first release would be 16 to 22 months behind D12 release date. D12 will release when PHPUnit will be on version 13, with version 14 coming in Feb 2027.

I'm not saying we necessarily have to aim being current with PHPUnit release, just we need to be aware. Tagging for release manager review.

One notch up.

mondrake → changed the visibility of the branch 3445240-metadata-check to hidden.

Filed 📌 Check that #[RunTestsInSeparateProcesses] attribute is added to all Kernel and Functional tests Needs review that imho should happen first.

mondrake → created an issue.

mondrake → changed the visibility of the branch 3445240-rule to hidden.

Bumping to critical while HEAD testing is broken.

Also, this has broken HEAD testing because

    There was 1 PHPUnit test runner deprecation:
    
    1) Metadata found in doc-comment for class Drupal\Tests\layout_builder\Functional\LayoutBuilderNewFieldsTest. Metadata in doc-comments is deprecated and will no longer be supported in PHPUnit 12. Update your test code to use attributes instead.

nice that we can get rid of @legacy-covers again pretty easily

on class level, yes; on method level, it's a challenge.

rebased

rebased

rebased

.

See comment inline

#6: 📌 Deprecate expectDeprecation(), use PHPUnit's expectUserDeprecationMessage() instead Postponed will cover that in the sense that after it, #[Group('legacy')] will just be a group like any other and any deprecation thrown during the test will make the test fail. So I wouldn't do anything additional right now; let's play whackamole when they pop up.

Filed 🐛 Use #[IgnoreDeprecations] instead of #[Group('legacy')] - again Active for follow up

mondrake → created an issue.

#11 I couldn’t find any phpcs/bcf rule dealing with PHPDoc indentation, surprisingly. That also explains why the PHPCS ci job does not fail. But yeah, it looks like this is a good job for that tool.

FWIW, I could not find a coding standard on the same topic, either.

Looks like we need follow ups here, tagging.

Adding related issue.

+1 to keep this open

#7 done, thanks!

mondrake → created an issue.

Bah, bot.

mondrake → created an issue.

rebased

Not sure about #2. If we readd here, we may need to remove again in a few months while adjusting for PHPUnit 12.

mondrake → created an issue.

Fixed - was broken by 📌 Deprecate node_type_get_names Active adding a @dataProvider annotation back into test code.

needs rebase

Well I guess since the introduction of the .deprecation-ignore.txt file this is solved in the sense that contrib can define their own file, and use it during testing with a proper setup of the SYMFONY_DEPRECATIONS_HELPER environment variable, no?

https://git.drupalcode.org/project/drupal/-/blob/11.x/core/phpunit.xml.d...

See comment inline

Filed 📌 Inject the 'settings' service to ImagemagickExecManager Active for follow up.

mondrake → created an issue.

mondrake → created an issue.

mondrake → created an issue.

mondrake → created an issue.

mondrake → created an issue.

mondrake → created an issue.

mondrake → created an issue.

Committed after some minor changes. Thanks!