🇮🇹
Account created on 7 May 2011, over 14 years ago
#

Merge Requests

More

Recent comments

🇮🇹Italy mondrake 🇮🇹

mondrake created an issue.

🇮🇹Italy mondrake 🇮🇹

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

🇮🇹Italy mondrake 🇮🇹

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

🇮🇹Italy mondrake 🇮🇹

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

🇮🇹Italy mondrake 🇮🇹

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

🇮🇹Italy mondrake 🇮🇹

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

🇮🇹Italy mondrake 🇮🇹

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 .

🇮🇹Italy mondrake 🇮🇹

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

🇮🇹Italy mondrake 🇮🇹

rebased

🇮🇹Italy mondrake 🇮🇹

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.

🇮🇹Italy mondrake 🇮🇹
🇮🇹Italy mondrake 🇮🇹

mondrake created an issue.

🇮🇹Italy mondrake 🇮🇹

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.

🇮🇹Italy mondrake 🇮🇹
🇮🇹Italy mondrake 🇮🇹

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.

🇮🇹Italy mondrake 🇮🇹

critical while head testing is broken

🇮🇹Italy mondrake 🇮🇹
🇮🇹Italy mondrake 🇮🇹

mondrake created an issue.

🇮🇹Italy mondrake 🇮🇹

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.

🇮🇹Italy mondrake 🇮🇹

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.

🇮🇹Italy mondrake 🇮🇹

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

🇮🇹Italy mondrake 🇮🇹

@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.

🇮🇹Italy mondrake 🇮🇹

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.

🇮🇹Italy mondrake 🇮🇹

One notch up.

🇮🇹Italy mondrake 🇮🇹

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

🇮🇹Italy mondrake 🇮🇹
🇮🇹Italy mondrake 🇮🇹

mondrake created an issue.

🇮🇹Italy mondrake 🇮🇹

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

🇮🇹Italy mondrake 🇮🇹

Bumping to critical while HEAD testing is broken.

🇮🇹Italy mondrake 🇮🇹

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.

🇮🇹Italy mondrake 🇮🇹

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

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

🇮🇹Italy mondrake 🇮🇹

#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.

🇮🇹Italy mondrake 🇮🇹

#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.

🇮🇹Italy mondrake 🇮🇹

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

🇮🇹Italy mondrake 🇮🇹

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

🇮🇹Italy mondrake 🇮🇹

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...

🇮🇹Italy mondrake 🇮🇹

Committed after some minor changes. Thanks!

🇮🇹Italy mondrake 🇮🇹

I think inline ignores are ok in this case. RTBC

🇮🇹Italy mondrake 🇮🇹

PDO driver subclasses were introduced by PHP 8.4 so I think we can use them also in 8.4. and not just in 8.5.

Production build 0.71.5 2024