[PhpUnitBridge] Use triggering class to generate baseline for deprecation messages from DebugClassLoader

[leongersen]

Q	A
Branch?	5.4
Bug fix?	yes
New feature?	no
Deprecations?	no
Tickets	Fix #45943
License	MIT
Doc PR	N/A

Fixes #45943 by using the triggering class to generate the baseline for messages from DebugClassLoader.

This would break currently generated baselines, but as #45943 describes, those are broken already when running tests in another order, or when running a sub-/superset of tests.

I'll finish this PR by adding/updating tests if this approach is acceptable.

[carsonbot]

Hey!

To help keep things organized, we don't allow "Draft" pull requests. Could you please click the "ready for review" button or close this PR and open a new one when you are done?

Note that a pull request does not have to be "perfect" or "ready for merge" when you first open it. We just want it to be ready for a first review.

Cheers!

Carsonbot

[carsonbot]

Hey!

I see that this is your first PR. That is great! Welcome!

Symfony has a contribution guide which I suggest you to read.

In short:

• Always add tests
• Keep backward compatibility (see https://symfony.com/bc).
• Bug fixes must be submitted against the lowest maintained branch where they apply (see https://symfony.com/releases)
• Features and deprecations must be submitted against the 6.2 branch.

Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change.

When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor!
If this PR is merged in a lower version branch, it will be merged up to all maintained branches within a few days.

I am going to sit back now and wait for the reviews.

Cheers!

Carsonbot

[carsonbot]

Hey!

I think @l-vo has recently worked with this code. Maybe they can help review this?

Cheers!

Carsonbot

[nicolas-grekas]

That'd work for me, please add tests :)

[ruudk]

@leongersen Thanks for working on fixing this problem. Would you be able to add the failing test? It would be super if this problem could be patched.

You can send me a Tikkie for a nice beer / coffee / whatever once this is merged 😉 🙏

[leongersen]

I'll try to finish this up next week :)

[leongersen]

Alright, I've added tests showing the new behavior and rebased against 5.4. I think I've tricked the pipeline into running the tests using the patched PhpUnit bridge for this PR, but I'm not sure if this is the correct way to do this. It took me a while to figure out how to get the tests to run using the changed bridge locally, too.

[leongersen]

@nicolas-grekas / @ruudk Sorry, I don't mean to nudge, but I'm not sure what the procedure is to report that this is ready for review. I don't think the remaining CI failure is caused by the changes in this PR.

[ruudk]

@nicolas-grekas / @ruudk Sorry, I don't mean to nudge, but I'm not sure what the procedure is to report that this is ready for review. I don't think the remaining CI failure is caused by the changes in this PR.

I don't know either, but maybe @xabbuh or @ogizanagi can help getting this reviewed? 🙏

[ruudk]

@leongersen could you rebase this PR so that we can get this merged?

[leongersen]

Yes, done.

[ruudk]

The failure in appveyor is related to RedisExtIntegrationTest and not caused by this PR.

[ruudk]

@leongersen Oke, the Psalm errors need to be fixed. Are you able to do it? Or do you rather let me fix it on your branch?

[stof]

Regarding the reported Psalm error, the one about LegacyDebugClassLoader not being defined is OK (as this is about supporting older Symfony version).
For DeprecationGroup not being defined, this look really weird.

[leongersen]

I don't know what causes the Psalm error on DeprecationGroup. It was already used in all classes before this PR.

[ruudk]

I tried to run Psalm locally, but that is extremely tedious to do. There is no psalm.phar nor an installer instruction. The baseline seems to be generated on the fly in the CI, before running the code from the branch. Why was this done like this? Wouldn't it be easier if the baseline was part of the repository, and that there was a psalm script that can be invoked locally?

[ruudk]

@nicolas-grekas Could you review this PR? Do you have an idea what could cause the Psalm error?

[stof]

@ruudk generating the baseline on the fly is because we want each PR to report only the new things. The codebase is too far from using Psalm for everything to bother about maintaining a committed baseline (we don't always fix all things reported by psalm, depending on what it reports). This CI job is here as an helper, not as a requirement.

[ruudk]

It would be really great if we could proceed with this PR. @leongersen sorry to ask, but could you update this PR one more time?

[leongersen]

@ruudk, I've rebased on 5.4. Based on the comments by @stof, I'm not taking action on the Psalm failures.

[nicolas-grekas]

Thank you @leongersen.

[ruudk]

@leongersen @nicolas-grekas just tested 6.3.2 and it works! I'm so happy to finally to be able to hide the indirect Monolog deprecation 🥳🥳🥳 thanks for your work on this