๐Ÿ‡ฎ๐Ÿ‡น
Account created on 7 May 2011, almost 13 years ago
#

Merge Requests

More

Recent comments

๐Ÿ‡ฎ๐Ÿ‡นItaly mondrake ๐Ÿ‡ฎ๐Ÿ‡น

Time to reopen this.

๐Ÿ‡ฎ๐Ÿ‡นItaly mondrake ๐Ÿ‡ฎ๐Ÿ‡น

Come on, botโ€ฆ

๐Ÿ‡ฎ๐Ÿ‡นItaly mondrake ๐Ÿ‡ฎ๐Ÿ‡น

Yes, this is overcome by phpstan-drupal incorporating depercation rules. Letโ€™s close.

๐Ÿ‡ฎ๐Ÿ‡นItaly mondrake ๐Ÿ‡ฎ๐Ÿ‡น

Great work... I'd RTBC this but think the honours better be done by someone who can test in the specific context of using xdebug.

๐Ÿ‡ฎ๐Ÿ‡นItaly mondrake ๐Ÿ‡ฎ๐Ÿ‡น

+1, I like this... besides being more self-contained, it will also be simpler to adjust if in the future we will change default commit from implicit to explicit. Left a couple of questions on the MR, not sure how feasible/relevant.

๐Ÿ‡ฎ๐Ÿ‡นItaly mondrake ๐Ÿ‡ฎ๐Ÿ‡น

#37 is the explanation, and #39 is the solution. Is that not clear enough?

๐Ÿ‡ฎ๐Ÿ‡นItaly mondrake ๐Ÿ‡ฎ๐Ÿ‡น

#23 I think it's better split this issue in two then, one for the CommentLinkBuilderTest fix and one for MailHandlerTest.

๐Ÿ‡ฎ๐Ÿ‡นItaly mondrake ๐Ÿ‡ฎ๐Ÿ‡น

mondrake โ†’ created an issue.

๐Ÿ‡ฎ๐Ÿ‡นItaly mondrake ๐Ÿ‡ฎ๐Ÿ‡น

Corrected one method's docbloc that was indicating a parameter as an array where it is actually a string.

๐Ÿ‡ฎ๐Ÿ‡นItaly mondrake ๐Ÿ‡ฎ๐Ÿ‡น
๐Ÿ‡ฎ๐Ÿ‡นItaly mondrake ๐Ÿ‡ฎ๐Ÿ‡น

mondrake โ†’ created an issue.

๐Ÿ‡ฎ๐Ÿ‡นItaly mondrake ๐Ÿ‡ฎ๐Ÿ‡น
๐Ÿ‡ฎ๐Ÿ‡นItaly mondrake ๐Ÿ‡ฎ๐Ÿ‡น
๐Ÿ‡ฎ๐Ÿ‡นItaly mondrake ๐Ÿ‡ฎ๐Ÿ‡น

mondrake โ†’ created an issue.

๐Ÿ‡ฎ๐Ÿ‡นItaly mondrake ๐Ÿ‡ฎ๐Ÿ‡น

Updated the IS with the completed tasks.

๐Ÿ‡ฎ๐Ÿ‡นItaly mondrake ๐Ÿ‡ฎ๐Ÿ‡น
๐Ÿ‡ฎ๐Ÿ‡นItaly mondrake ๐Ÿ‡ฎ๐Ÿ‡น
๐Ÿ‡ฎ๐Ÿ‡นItaly mondrake ๐Ÿ‡ฎ๐Ÿ‡น

This https://git.drupalcode.org/project/drupal/-/blob/6ba264f34cc45511333ef47... is how the trait will change to support PHPUnit 10.

As long as the PHPUnit 10 metadata method handles both attributes and annotations then I don't see an issue, and we should be able to upgrade to PHPUnit 10 first and then migrate test annotations to attributes separately if we need to.

PHPUnit 10 will support both. PHPUnit 11 will deprecate annotations. PHPUnit 12 will remove them. We potentially need to convert annotations to attributes before Drupal 11 if we want to target PHPUnit 11 for it AND we do not want deprecation notices.

๐Ÿ‡ฎ๐Ÿ‡นItaly mondrake ๐Ÿ‡ฎ๐Ÿ‡น

mondrake โ†’ changed the visibility of the branch 3417650-replace-calls-to to hidden.

๐Ÿ‡ฎ๐Ÿ‡นItaly mondrake ๐Ÿ‡ฎ๐Ÿ‡น

Adjusted IS with latest direction discussed in Slack.

๐Ÿ‡ฎ๐Ÿ‡นItaly mondrake ๐Ÿ‡ฎ๐Ÿ‡น
๐Ÿ‡ฎ๐Ÿ‡นItaly mondrake ๐Ÿ‡ฎ๐Ÿ‡น

Tests were failing due to missing @group annotation on the newly introduced test.

๐Ÿ‡ฎ๐Ÿ‡นItaly mondrake ๐Ÿ‡ฎ๐Ÿ‡น

My 0.02:

Determine if we should apply to all methods in tests (MR !6604) or just methods with the test prefix (MR !6606).

Only methods with test prefix. The other methods may be used by contrib/custom and their (non)value used and therefore fail PHPStan. Also forcing a void seems a too strong assumption.

Determine if this needs to be split in smaller chunks creating significant amounts of busy work or if we can apply it in one hit

One big megapatch, in an agreed date during the beta window. Probably needs release manager review to assign a date.

๐Ÿ‡ฎ๐Ÿ‡นItaly mondrake ๐Ÿ‡ฎ๐Ÿ‡น

Tricky stuff. Needs pipeline execution for the other dbs, I can not run those.

While this also fixed some things along, IMHO the future should be along the lines of #84. Hopefully we'll get there, all these dances at destruction time seem a bit fragile.

๐Ÿ‡ฎ๐Ÿ‡นItaly mondrake ๐Ÿ‡ฎ๐Ÿ‡น

mondrake โ†’ changed the visibility of the branch 10.1.x to hidden.

๐Ÿ‡ฎ๐Ÿ‡นItaly mondrake ๐Ÿ‡ฎ๐Ÿ‡น

mondrake โ†’ changed the visibility of the branch 3405976-mondrake-approach to hidden.

๐Ÿ‡ฎ๐Ÿ‡นItaly mondrake ๐Ÿ‡ฎ๐Ÿ‡น

mondrake โ†’ changed the visibility of the branch 3405976-mondrake-approach-10.2.x to hidden.

๐Ÿ‡ฎ๐Ÿ‡นItaly mondrake ๐Ÿ‡ฎ๐Ÿ‡น

Needs rebase and understanding why the tests failed

๐Ÿ‡ฎ๐Ÿ‡นItaly mondrake ๐Ÿ‡ฎ๐Ÿ‡น

#12 mmmโ€ฆ there are comments in the code above the change explaining why exception should be avoided here

๐Ÿ‡ฎ๐Ÿ‡นItaly mondrake ๐Ÿ‡ฎ๐Ÿ‡น

c/p from Slack

my intention would be to backfill expectWarning in the issue where we drop the bridge. Itโ€™s there already. We could easily backfill expectError, too. But I thought that where for error we should comply to PHPUnit direction, for warning instead we could keep it to reduce the refactoring needed, that alas in this case covers runtime code too, not just test one.

๐Ÿ‡ฎ๐Ÿ‡นItaly mondrake ๐Ÿ‡ฎ๐Ÿ‡น

The other, alternative approach here would be to avoid trigger_error() completely and log a message through the logger, instead. An example in that sense is in ๐Ÿ“Œ Find an alternative to trigger_error in Drupal\Core\EventSubscriber\RedirectResponseSubscriber::checkRedirectUrl Active . We need to take a direction, here.

๐Ÿ‡ฎ๐Ÿ‡นItaly mondrake ๐Ÿ‡ฎ๐Ÿ‡น

I'll ask for 2nd eyes on Slack.

๐Ÿ‡ฎ๐Ÿ‡นItaly mondrake ๐Ÿ‡ฎ๐Ÿ‡น
๐Ÿ‡ฎ๐Ÿ‡นItaly mondrake ๐Ÿ‡ฎ๐Ÿ‡น

mondrake โ†’ created an issue.

๐Ÿ‡ฎ๐Ÿ‡นItaly mondrake ๐Ÿ‡ฎ๐Ÿ‡น

mondrake โ†’ created an issue.

๐Ÿ‡ฎ๐Ÿ‡นItaly mondrake ๐Ÿ‡ฎ๐Ÿ‡น

Slightly edited the CR, hopefully for good :)

๐Ÿ‡ฎ๐Ÿ‡นItaly mondrake ๐Ÿ‡ฎ๐Ÿ‡น

I think itโ€™s out of scope, in the sense that here weโ€™re focusing in the thing needed for PHPUnit 10.

Adding return types is absolutely something to do, but then what about adding void to the test methods themselves, and using PHPStan array shapes docs to describe the array returned by the data providers, etc etc. A different story.

๐Ÿ‡ฎ๐Ÿ‡นItaly mondrake ๐Ÿ‡ฎ๐Ÿ‡น

Rector-phpunit already has some rules for this, https://github.com/rectorphp/rector-phpunit/blob/main/docs/rector_rules_... and https://github.com/rectorphp/rector-phpunit/blob/main/docs/rector_rules_...

I just found them once I had already made the script attached, and since it worked I saw no reason to change anything.

But that could be useful for contrib/custom conversions.

๐Ÿ‡ฎ๐Ÿ‡นItaly mondrake ๐Ÿ‡ฎ๐Ÿ‡น

#32 see #7.

There is a PHPStan rule checking for that, but only if PHPUnit 10 is already installed which is a bit pointless IMHO, https://github.com/phpstan/phpstan-phpunit/issues/178.

Using listeners I'm not very keen on since we're going to gut them.

Develop a custom PHPStan rule seems a bit outworldly since one exists already.

So IMHO the best thing to do now is to get these 1k in here, then watch and rebase ๐Ÿ“Œ Drop Symfony PHPUnit-bridge dependency Active where the deprecation of non-static dataprovider is reported/baselined. That will tell us if we readd some through the cracks before we bump PHPUnit to 10. Later, it will be PHPUnit 10 itself to tell us.

๐Ÿ‡ฎ๐Ÿ‡นItaly mondrake ๐Ÿ‡ฎ๐Ÿ‡น

$process->setTimeout(NULL); should do, for it does not set any time limit to the process execution.

https://github.com/symfony/symfony/blob/7.0/src/Symfony/Component/Proces...

๐Ÿ‡ฎ๐Ÿ‡นItaly mondrake ๐Ÿ‡ฎ๐Ÿ‡น

Serms OK for now. A possible follow up is to make the timeout dependent on an env variable.

๐Ÿ‡ฎ๐Ÿ‡นItaly mondrake ๐Ÿ‡ฎ๐Ÿ‡น

Note to self: when this is unblocked, do #3386263-10: Ignore, testing issue โ†’ first thing.

๐Ÿ‡ฎ๐Ÿ‡นItaly mondrake ๐Ÿ‡ฎ๐Ÿ‡น

This could do, I think.

We remove @trigger_error on classes, that fires on class autoloading, leaving the @deprecate annotations. That's enough to have PHPStan detect usage of deprecated classes, in case - that's demonstrated by the fact that we need to add to PHPStan baseline the extension of TestSuiteBase by its child classes.

๐Ÿ‡ฎ๐Ÿ‡นItaly mondrake ๐Ÿ‡ฎ๐Ÿ‡น

I think thatโ€™s because those tests are run directly via Phpunit CLI โ€”group and not via run-tests.sh.

That triggers PHPUnitโ€™s own test discovery that loads all the classes and therefore triggers the deprecation errors. run-tests uses its own test discovery and then runs test on a file-by-file basis, so that doesnโ€™t occur.

Not sure what is the best way forward. Maybe just avoid the @trigger_error of the deprecation of the classes so we donโ€™t get this issue with autoloading.

๐Ÿ‡ฎ๐Ÿ‡นItaly mondrake ๐Ÿ‡ฎ๐Ÿ‡น

Replied to the inline comment. Please post something on the d.o. issue too when commenting the MR so that it becomes visible on the d.o. dashboard, otherwise it's just casual to get to know about the MR comment. Thanks!

๐Ÿ‡ฎ๐Ÿ‡นItaly mondrake ๐Ÿ‡ฎ๐Ÿ‡น

The script converts all the dataproviders that do not use calls to instance properties, that will have to be converted separately. The only exception is ConfigEntityValidationTestBase::providerInvalidMachineNameCharacters() that needs to be manually reverted to non-static, because if the method base class does not make calls to $this, the extending classes do.

๐Ÿ‡ฎ๐Ÿ‡นItaly mondrake ๐Ÿ‡ฎ๐Ÿ‡น

Wow this is tough. Quite close but not there yet.

๐Ÿ‡ฎ๐Ÿ‡นItaly mondrake ๐Ÿ‡ฎ๐Ÿ‡น

If the objective of the trigger_error is to add a log entry for site admins to review, however, then this is not fully right as the errors/warnings are only added if the appropriate setting allows - which is discouraged in prod sites. Better logging directly to a logger service.

๐Ÿ‡ฎ๐Ÿ‡นItaly mondrake ๐Ÿ‡ฎ๐Ÿ‡น

In this case, the user is anyway informed about the issue via the 400 response with the message. The point is whether we want to track the same message for site admin. If we use trigger_error(), the error will only be logged if the setting allows, which is not recommended in prod sites. So it'd be better log it directly via a service, if the container is available?

Production build https://api.contrib.social 0.61.6-2-g546bc20