[PhpunitBridge] Exit code 1 instead of 0 using "quiet" option in 6.1.0

[jmsche]

Symfony version(s) affected

6.1.0

Description

Hi,

I'm using this configuration for the Phpunit Bridge: SYMFONY_DEPRECATIONS_HELPER="quiet[]=indirect&quiet[]=other"

Using Symfony 6.0.x I get a 0 exit code, however with Symfony 6.1.0 I get a 1 exit code.

How to reproduce

• Have some tests trigger a deprecation
• SYMFONY_DEPRECATIONS_HELPER="quiet[]=indirect&quiet[]=other"
• Run tests
• echo $? should return 0 but returns 1

[Edit] Added a simple reproducer here: https://github.com/jmsche/symfony-deprecation-reproducer

To reproduce using the reproducer:

git clone https://github.com/jmsche/symfony-deprecation-reproducer.git
cd symfony-deprecation-reproducer/
composer i
bin/phpunit 
echo $? # outputs 1
git fetch 
git checkout 6.0
composer i
bin/phpunit 
echo $? # outputs 0

Possible Solution

No response

Additional Context

The only way I managed to get a 0 exit code is to set SYMFONY_DEPRECATIONS_HELPER="disabled=1", using the new logFile option makes PHPUnit return 1 as well.

[xabbuh]

What output do you get when omitting the quiet part on Symfony 6.0 and 6.1 respectively?

[jmsche]

What output do you get when omitting the quiet part on Symfony 6.0 and 6.1 respectively?

When I remove the SYMFONY_DEPRECATIONS_HELPER env var from .env.test I get 1 for both Symfony versions.

I just added a simple reproducer to the issue description if you can have a look :)

[PythooonUser]

Same is happening for our team. After upgrading to 6.1 neither quiet nor verbose take any effect.

[tjveldhuizen]

Related: while I still have SYMFONY_DEPRECATIONS_HELPER="quiet[]=indirect" in my phpunit.xml.dist, running bin/phpunit is reporting indirect deprecations.

[PythooonUser]

Would someone be able to create a PR with a failing test?

[PythooonUser]

@xabbuh Hey Christian, would the failing test above help out a bit more?

However, I'm surprised that the partially_quiet.phpt test still works.

[HypeMC]

The problem was introduced in #45923 , before that PR total would default to 999999, now it defaults to 0. Because of that $isFailing is now true so the process exits with 1.

[derrabus]

The problem was introduced in #45923

cc @mondrake

[PythooonUser]

@HypeMC I think the failing behavior then is fine actually. When max[total] = 0 is set implicitly, this is to be expected.

The actual problem (for me) is, that also the behavior of the quiet option changed. The docs state that

It's also possible to change verbosity per deprecation type. [...] will hide details for deprecations of types "indirect" and "other".

It does not (#46651). Is it because when the max[total] constraint fails, the verbosity settings are ignored and full issue details are always shown? If this is the case, then the docs could be enhanced because that behavior is not obvious, at least not for me.

[HypeMC]

@PythooonUser It's related, the $isFailing variable also affects whether the deprecations should be displayed.

[tjveldhuizen]

@nicolas-grekas Since you have reviewed #45923, do you think the fix in that PR breaks BC, since SYMFONY_DEPRECATIONS_HELPER="quiet[]=indirect" after that change does report indirect deprecations?

[flohw]

Hello,

Long time without response so I will mention you @nicolas-grekas and @greg0ire as you reviewed the mentionned PR, just in case.

I was about to open an issue about this behavior too and found this one and came to the same conclusion: the bug was introduced by #45923. We just updated to Symfony 5.4.18.

Setting SYMFONY_DEPRECATIONS_HELPER=weak or SYMFONY_DEPRECATIONS_HELPER=max[total]=99999 (and variations) solves the issue but is not ideal as verbose=0 should not trigger the error too (or we have to update to documentation)

Thank you.

[greg0ire]

Sorry, which mentioned PR did I review?

[flohw]

Woops... I may have confused different tabs 😬
Sorry for that ping.

[flohw]

@greg0ire Just in case the notification already reached you but the edit wouldn't:

I found the original one I wanted to point : #48787

And didn't confused my tabs 😇

[carsonbot]

Hey, thanks for your report!
There has not been a lot of activity here for a while. Is this bug still relevant? Have you managed to find a workaround?

[carsonbot]

Friendly reminder that this issue exists. If I don't hear anything I'll close this.

[alvarocebrian]

Hi everybody, just a reminder that this is still happening

I've just updated my code to 6.3 and my jenkins has started to fail because of deprecations in the code while it didn't with symfony 5.4 and PHPUnit 10.3.

I've read some posts about this problem (such as #49069) but not a clear solution found.
Also I'd say PHPUnit bridge's documentation should be updated in order to warn about this and to propose a temporal solution if it exists

[greg0ire]

If the PR was caused by #45923, why is nobody mentioning @mondrake ?

[flohw]

Done a bit higher, 21st june last year 😬, so we are trying other ways as none of us seems to understand properly how it should behave, how to fix or what's going wrong.

#46497 (comment)

If the fix is to update the documentation, I can take care of that.

[flohw]

Also the issue may have been introduced by #48787 and the right person to mention is @ogizanagi ?

I hope to not make a mistake by involving another person.

Thank you

Sorry for double posting.

[greg0ire]

how it should behave

I think the verbosity level should govern the output and only that. That is what I would expect without reading any docs.
When I read the docs, I cannot find anything that contradicts that expectation.

[flohw]

I agree. But the exit code still has changed. Should it have been considered a BC break?
We may update the doc to mention the change even if it's too late?
Or this issue could be enough and should now be closed?

[greg0ire]

I think a documentation update might be cool, since this is not obvious. As for whether it is a breaking change, it's harder to argue when there are no docs. Maybe it initially behaved like I just described, who knows?

[flohw]

I will try to make a pr to the doc by the end of the week. The harder for me will be to know how to explain this and how to write it... In english. 😅

Thanks, Gregoire. 🙂

[greg0ire]

No need to overthink it 🙂 a note with the following should suffice:

quiet hides details for the specified deprecation types, but will not change the outcome in terms of exit code. That's what max is for, and both settings are orthogonal.

In terms of UX, I'm starting to think that a quiet_if_successful setting might be handy. Still in terms of UX, there is no explanation of what exactly is responsible for a failure. It would be nice to have something like "exited with code X because there were 5 direct deprecations, and the configured maximum is 4". Don't ask me to implement that though, I have a lot on my plate and I remember the phpunit bridge was quite hard to contribute to the few times I tried. 😛 Just… giving ideas in hope somebody here has too much time on their hands.

[flohw]

The only thing I can do about this nice change (to me too at least) would be to open a dedicated issue. 😇

[greg0ire]

Well that would be something already 👍

[ogizanagi]

I might not understand well the issue here yet, but the docs states :

By default, any non-legacy-tagged or any non-silenced (@-silencing operator) deprecation notices will make tests fail.

#48787 didn't mean to change anything regarding this ; any deprecation by default should make the tests fail and exit 1.

So, when using:

SYMFONY_DEPRECATIONS_HELPER="quiet[]=indirect&quiet[]=other"

the behavior, to me, should be an exit code of 1.
➜ If you want it to be 0, you should explicitly configure a threshold for the total (max[total]=9999) or each type individually (max[indirect]=9999).

SYMFONY_DEPRECATIONS_HELPER="max[total]=9999&quiet[]=indirect&quiet[]=other"

---

#48787 aimed to show verbose output only for the types of deprecation reaching a threshold, instead of showing the verbose output for all of the types.

So when using:

SYMFONY_DEPRECATIONS_HELPER="max[direct]=0&max[self]=0&max[indirect]=10&max[total]=999999&quiet[]=indirect

I'd expect indirect deprecations not to be shown as verbose output, unless reaching the threshold of 10 configured in max[indirect]=10.

self and direct ones would still use the verbose output, since not quiet.

---

➜ So, I think @PythooonUser got it right regarding this:

@HypeMC I think the failing behavior then is fine actually. When max[total] = 0 is set implicitly, this is to be expected.

The actual problem (for me) is, that also the behavior of the quiet option changed. The docs state that

It's also possible to change verbosity per deprecation type. [...] will hide details for deprecations of types "indirect" and "other".

It does not (#46651). Is it because when the max[total] constraint fails, the verbosity settings are ignored and full issue details are always shown? If this is the case, then the docs could be enhanced because that behavior is not obvious, at least not for me.

But the behavior of the quiet option didn't actually changed ; it was buggy in the first place, showing verbose output for every deprecation types as soon as a threshold is reached. Now it does only for the deprecation type that reached the threshold.

Though, considering this, there is no way to totally hide verbose output for specific deprecation types as soon as the threshold is reached. But would it make sense anyway ? If your threshold is reached, you probably want to get the maximum information about it and how to solve it (or reconsider the threshold if no solution is applicable to your codebase)

[greg0ire]

Ah I didn't know that quiet already behaves as quiet_if_successful, so that's great 👍

Though, considering this, there is no way to totally hide verbose output for specific deprecation types as soon as the threshold is reached. But would it make sense anyway ? If your threshold is reached, you probably want to get the maximum information about it and how to solve it (or reconsider the threshold if no solution is applicable to your codebase)

I can't imagine a situation where you would want to hide that valuable piece of information. I think this can be closed or transferred to symfony-docs.

[flohw]

it was buggy in the first place, showing verbose output for every deprecation types as soon as a threshold is reached

And the fix may have introduced the break with the existing code. (only supposition, it will be hard to know where, when and how the behavior has changed and I would be the least qualified to point the line)

I think this issue can be closed too. I will try to make a pr by the end of the week on the doc to make the difference between max and quiet clearer.

Thanks both of you for the investigations and explanations. :-)

[mondrake]

Sorry I missed #46497 (comment) and I'm late on the last mention. If this was introduced in #45923 or, in general, as part of the ignoreFile implementation, well that was certainly not intentional. I'll try to catch up with the comments here and look into the code if I can figure out anything.