[phpunit-bridge] default to not failing on deprecations #442
Conversation
ghost
deployed
to
|
But seeing deprecations reported in methods of your code is scary, and prompts urgent WTF'ing. I'd advocate the average developer doesn't care for deprecations in code that isn't theirs and for that reason, this should be disabled by default, with a note in the testing docs detailing how to opt it. |
|
@nicolas-grekas I see 1 WTF effect for this: it might make it impossible to configure this in the phpunit.xml (as this |
|
Configuring the bridge via |
|
I'd prefer if this new default is handled in https://github.com/symfony/symfony/blob/0eea07752211f464818a54efc7907e8ca4c617a6/src/Symfony/Bridge/PhpUnit/bootstrap.php#L39 so that it also behaves like this when not using the simple-phpunit wrapper. |
|
But that would be a BC break. I also think having a stricter default is a better practice. And requiring people to opt-in for less strict config (via their own wrapper or a real env var) encourages ownership IMHO. |
|
ping @symfony/deciders |
|
As the recommended default, wouldn't it be cleaner and more self-explaining to have an explicit config value for this like |
|
That'd be a new feature, thus not actionable until 4.2. And personally, I prefer the number, because it hints it exists and can be changed: lowering this number up to zero is a best practice IMHO. |
|
Where is this best practice declared so people can read up on it? |
|
@ChangePlaces I looked at the doc, it's not explicit so we could improve (it's implicit because that's the default, but I suppose that's not enough.) Would you mind sending a PR to improve that §? |
|
I don't think I'm the best person to submit a PR as I fail to see it as a best practice myself |
|
@nicolas-grekas what (or who) is needed to get this merged? |
|
One more vote from any @symfony/deciders |
|
I would prefer a new config option too. This one looks like a hack, and should’nt be in the official recipe if we can provide something nicer. |
|
A new option will not be applicable until 4.2. I can wait personally, and I also prefer strict mode so I'm also fine closing. But that's not what others would prefer... Let's move on ? |
|
Oh and about |
|
About the future, see symfony/symfony#28048 |
|
Also, I think a logical follow-up would be to make a BC-breaking PR to use |
|
Nope, there's no logical BC break. |
|
Sorry, what do you mean with "logical BC-break"? I think a user having suddenly the bridge no longer make their test suite fail where it used to fail is entitled to call that a BC-break, aren't they? Would you do that PR on the stable branch? Or did I misunderstand your answer? |
|
I mean we cannot change that line 40 you linked: that'd be a BC break and we just don't allow us to do that. |
|
Thanks for the clarification, I'm trying to address some concerns voiced on doctrine/doctrine-website#200 |
Until now, I've been advocating that peoples' CI should fail whenever a deprecation is reported.
But when a vendor throws a deprecation, sometime, it's not your fault and you can't do anything about it (except contributing back to the project, which is good for OSS!)
Talking on Slack, @greg0ire convinced me we can do this change: by setting
SYMFONY_DEPRECATIONS_HELPERto999999by default, we make ppls' CI green (when they don't raise more than 1M deprecations.)And it still makes the deprecation report visible on the tests' output so that ppl still have some incentive to fix them.
Fixes symfony/symfony#27936