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

Merge Requests

More

Recent comments

🇮🇹Italy mondrake 🇮🇹

📌 Enforce removal of PHPUnit annotations from test classes' methods metadata Needs review will report #50 as a PHPStan error.

🇮🇹Italy mondrake 🇮🇹

Apparently, PHPUnit 11 is not throwing deprecations when @testWith is used...

🇮🇹Italy mondrake 🇮🇹

Any reason why Drupal\image\ImageEffectBase is not converted?

🇮🇹Italy mondrake 🇮🇹

Committed to 5.0.x and cherry-picked to 4.0.x

Not cherry-picked to 4.1.x because there is not likely going to be a release 4.1.0 ever, for the same reason of 💬 Stable release imagemagick 4.1.0 Active .

Thanks!

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

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

🇮🇹Italy mondrake 🇮🇹

mondrake created an issue.

🇮🇹Italy mondrake 🇮🇹

mondrake created an issue.

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

mondrake created an issue.

🇮🇹Italy mondrake 🇮🇹

No - database drivers in sub-namespaces of Drupal\Driver\Database are long gone, and in fact those test cases should have been removed when the logic was. Better late than never.

🇮🇹Italy mondrake 🇮🇹

Thanks

I wonder whether we should detect whether the rotate_ie plugin is available before swapping the operation.

Either way, we need a small test to prove this works and to prevent future regressions.

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

Committed to 5.0.x and cherry-picked to 4.0.x

Not cherry-picked to 4.1.x because there is not likely going to be a release 4.1.0 ever, for the same reason of 💬 Stable release imagemagick 4.1.0 Active .

Thanks!

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

I see the MR already targets 5.0.x, thanks

🇮🇹Italy mondrake 🇮🇹

There are deprecation failures.

🇮🇹Italy mondrake 🇮🇹

mondrake created an issue.

🇮🇹Italy mondrake 🇮🇹

PHPUnit 12 is out of the game for Drupal 11: #3546029-25: Ensure that #[RunTestsInSeparateProcesses] attribute is added to all Kernel tests

@catch:

Just discussed this with @alexpott, @bircher and others at DrupalCon, and we think that we should only update to phpunit 12 in Drupal 12 - just because forcing every contrib module to update their tests in a minor release would be a massive break.

It would be good to do 📌 Remove support for PHPUnit 10 Active though, to avoid rerolls.

🇮🇹Italy mondrake 🇮🇹

📌 Remove support for PHPUnit 10 Active must happen before committing this, but the MR can be reviewed now.

🇮🇹Italy mondrake 🇮🇹

Please target 5.0.x first.

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

The MR is 1280+ commits behind head, needs a rebase.

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

Let's add couple more test cases.

🇮🇹Italy mondrake 🇮🇹

Thanks! Applied suggestion.

🇮🇹Italy mondrake 🇮🇹

Added a test for the PHPStan rule.

🇮🇹Italy mondrake 🇮🇹

Needs tests, but I am opening up for review

🇮🇹Italy mondrake 🇮🇹

Let's extend to Kernel tests data providers, too - now we have a clean report of the failures in 📌 Introduce support for PHPUnit 12 Postponed .

🇮🇹Italy mondrake 🇮🇹

Just rebased to see if the new deprecation for Kernel tests missing the #[RunTestsInSeparateProcesses] attribute triggers as expected. 📌 Check that #[RunTestsInSeparateProcesses] attribute is added to all Kernel and Functional tests Needs review

Sorry for the noise.

🇮🇹Italy mondrake 🇮🇹

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

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

All children are in, I think we can mark this fixed too - leaving to RTBC for any input

🇮🇹Italy mondrake 🇮🇹

This is fixed, right?

🇮🇹Italy mondrake 🇮🇹

Thanks for addressing my input.

#38 well TestTools is a plural already so I do not see a problem with Mocks?

🇮🇹Italy mondrake 🇮🇹

rebased and updated baseline, back to RTBC

🇮🇹Italy mondrake 🇮🇹

Bot????? It looks like the bot is posting based on old info

🇮🇹Italy mondrake 🇮🇹

Well, bot...

🇮🇹Italy mondrake 🇮🇹

Hey, bot...

🇮🇹Italy mondrake 🇮🇹

Works nicely. Add an Autowire trait for plugins Needs review was just committed so let's wait a bit to see if it holds before committing this.

🇮🇹Italy mondrake 🇮🇹

mondrake created an issue.

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

I think #26 accounts for a release manager review. Fully agree, readjusting the deprecation message right now.

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

Totally missed the CR, sorry. Added a draft one.

📌 Ignore specifc sniffs when adding additional parameters to interface methods Active came up with a different format for the phpcs, @todo and @see dance. Attending the ball here.

🇮🇹Italy mondrake 🇮🇹

#22 well, that's exactly what was discussed above

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

OK then, lets do that.

🇮🇹Italy mondrake 🇮🇹

#17 actually, PHPUnit 12 will not throw anything... simply, it will not run the test in isolation without the attribute having been set.

That's why I am proposing to convert the deprecation triggered here in the setUp method into an exception - that would be Drupal's way of checking that the concrete test complies with the requirement. Alternatively, we could do that check in a static analysis rule, but that would not be guaranteed to be part of contrib checks: by adding it as an exception in the abstract base class there's no way around it.

So, I think we agree to start throwing the deprecation now. Question is when we start throwing the deprecation instead. Do I understand we can leave that open for now?

I.e. a deprecation message like 'Kernel tests must specify #[RunTestsInSeparateProcesses], not doing so is deprecated in drupal:11.3.0 and is removed when PHPUnit 12 is supported. See https://www.drupal.org/node/3548485' would be it?

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

I would suggest to move the Mocks sub-namespace under Drupal\TestTools, it eould be consistent with the rest and no need to register another namespace for testing

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

@dcam thanks for the review effort... really remarkable.

The reason why you see seemingly unrelated rearranging of use imports is due to the fact that the refactoring is done by rector, whereas the resorting is done by phpcbf on any file changed, independently from what the change was.

Will rebase now - tagged 'Avoid commit conflicts' exactly for the reason in #6.

🇮🇹Italy mondrake 🇮🇹

Let's call this issue 'round 1'. A 'round 2' issue would increase type coverage level to max (I checked and that will add a lot more goodness, including argument typing based on inference). 'Round 3' or more would be a manual check/refactor of the leftovers at that stage.

🇮🇹Italy mondrake 🇮🇹

mondrake created an issue.

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

Thank you!

The annotation code is under phpcs ignore so that's why, I assume, phpcbf does not make fixes there.

It made a bunch of lowercase `null` insertions. Not sure what was up with that. I ended up grepping the diff file to find them all. There should be suggestions for all of them.

I really wish we had Stop using uppercase TRUE, FALSE, NULL literals Active . IMHO, spending time on this is waste of resources.

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

Inspired by this (thank you!), I have opened 📌 Add return types to core tests code via Rector Active to try tackling test code.

🇮🇹Italy mondrake 🇮🇹

mondrake created an issue.

🇮🇹Italy mondrake 🇮🇹

not pushed

🇮🇹Italy mondrake 🇮🇹

Thank you. I've added some inline comments in the rule code, to explain a bit what's going on. Leaving at RTBC as no code was changed.

🇮🇹Italy mondrake 🇮🇹

rebased - any dependency update will cause merge errors here, unfortunately

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

Thanks!

Since we are at it, lets’s do #8.

If you are intersted, the MR in the parent is running tests with PHPUnit 12. It’s my reference to see reported results that lead to issues like this one.

🇮🇹Italy mondrake 🇮🇹

RTBC, but HEAD testing is broken now

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

Bump with increased constraint to prevent regression.

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

mondrake created an issue.

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

mondrake created an issue.

🇮🇹Italy mondrake 🇮🇹
  1. Removed from SettingTest some test cases that were still based on db connection logic that was removed in #3294695: Drupal 8 BC for database driver namespace fails for replicas .
  2. Made keys in array shapes have quotes so to improve readibility
  3. Addressed @longwave's feedback
🇮🇹Italy mondrake 🇮🇹
🇮🇹Italy mondrake 🇮🇹
🇮🇹Italy mondrake 🇮🇹
🇮🇹Italy mondrake 🇮🇹

Unblocked

🇮🇹Italy mondrake 🇮🇹

Unblocked

🇮🇹Italy mondrake 🇮🇹

Thanks!

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

Addressed feedback in #19 and edited the draft CR, hopefully for the good.

🇮🇹Italy mondrake 🇮🇹

Thanks a lot for the review. I'll be addressing the points in #19.

Anyway, let's postpone on 📌 Complete test annotations to attributes conversion for all remaining tests Postponed that is completing the transition from annotation to attributes, and is tagged for avoiding commit conflicts.

🇮🇹Italy mondrake 🇮🇹

Ah right... would it make sense to add a conflict key instead, then?

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

I think we also need to update the constraint in core/composer.json to ^1.23.0 so to prevent regression

Production build 0.71.5 2024