beStrictAboutOutputDuringTests=true or failOnRisky=true causing test failures due to PHP8.4 Implicit Nullable deprecation

FWIW, I think this should be triggered, because it isn't suppressed like core deprecations. This will be displayed on your site as you develop and logged to watchdog. It should be fixed and it's perfectly backwards-compatible to at least 8.1. If anything, IMHO it's the opposite. That these messages are sometimes *not* causing tests to fail (e.g. inside ajax callbacks), but that's a different issue.

Regardless of if we feel they are worth having or not it is a template BC break.

Some of us setup our pipelines to not fail on deprecation, gitlab_template is not permitted to make changes that would cause test that would previously pass to fail without a new major branch (which has been repeatedly objected to doing so we have to keep the previous base for the foreseeable future).

Side note:
Some of us maintain compatibility to PHP 7.0 still (though I found a couple projects have accidentally breached this) not because we recommend using it, instead because it’s not our place to go arbitrarily changing the minimum system requirements inside a major branch and have zero justification to add a new major when BC has to date been extremely easy to maintain.

Thanks @cmlara for raising the issue and thank you @berdir for giving the oppsite view. It's always good to see both sides of an issue like this.

$CORE_PHP_MAX was updated from 8.3 to 8.4 last week on 🐛 Update PHP_MAX to 8.4 Active . I see that the tfa project → has a few customisations in the .gitlab-ci.yml and in particular is setting the global php version to $CORE_PHP_MAX. That is why at first glance the problem appeared to affect the "current core" variant. But for projects which do not have this custom override the "current core" variants still run PHP 8.3 and so will not be affected.

As you both know obviously, the pipeline can be made to not fail by using allow_failure: true, which would still show those warnings but return amber not red, and not fail the pipeline. But I understand that you do not want to add that to your principal testing variant. So maybe another solution would be not to override the php version to $CORE_PHP_MAX for your current variant, but instead use OPT_IN_TEST_MAX_PHP: 1 so that you test at 8.3 and 8.4. The 'phpunit (max php version)' variant could then be allowed to fail and not affect the overall pipeline result.

I have also noticed that the gitlab templates definition for 'phpunit (max php version)' does not have allow_failure: true and I think this is an ommision/oversight, so I have created a MR to add this. Probably this kind of php version increase which causes new phpunit failures has not happened since we started the gitlab templates for contrib projects.

a few customisations in the .gitlab-ci.yml and in particular is setting the global php version to $CORE_PHP_MAX.

Fair point. I did not realize we had yet another policy violation for gitlab_templates (PHP target is suppose to be the latest version of PHP supported by the latest version of core), I did not even think to go look at that in my .gitlab-ci.yml.

I can pull that out, although that only delays the inevitable here as eventually we will jump the version in templates.

Probably this kind of php version increase which causes new phpunit failures has not happened since we started the gitlab templates for contrib projects

This would not have happened (at least on the straight PHPUnit runs) before we committed 🐛 browser_output is empty when testing with Drupal 11 Active as phpunit defaults these values to false. It is the Core phpunit.xml that sets them to true. Additionally a significant amount of PHP 8.3 around no longer allowing coercion were detected by phpstan before the deprecation were even voted into PHP8.3 making them less likely to exist in code.

I can override this by placing a custom phpunit.xml in my project to avoid cores phpunit from impacting my configuration (far more ideal than allow failures) however my point is more that I should not have to, it should be those who want to opt into this that do so (as a baseline change from templates version 1.0.0 we are suppose to give deference to avoiding breaks in existing pipelines and make everyone else opt in to “breaking” changes).

I can override this by placing a custom phpunit.xml in my project ... my point is more that I should not have to

Yes that's a fair point too. I think we should look at either removing those settings from the core file once downloaded, and/or provide options to set them, but default to false.

Wouldn't setting the default value of _PHPUNIT_EXTRA to --no-configuration be equivalent to what we had before the change?

Wouldn't setting the default value of _PHPUNIT_EXTRA to --no-configuration be equivalent to what we had before the change?

I’m not sure how —no-configuration and -c work togehter so that would have to need to be tested.

However even if it does word that would depend upon no jobs having a customized _PHPUNIT_EXTRA (I do for code coverage) or again back to requiring maintainers to make job changes.

Yeah I'm just trying to understand/identify the possibilities, which might be reflected via documentation.

In any case, we've been adding features and improvements as well as trying to mirror many of the checks that Drupal core does, not just for PHPUnit but for other jobs too, so I don't think that using (a slightly modified version of) core's PHPUnit file is a regression, as it was something that could have been done since day 0 but just wasn't (same as allowing PHPUnit vs run-tests.sh). I know that some people can see as a new feature, but some other could even argue that it was a bug that needed to be fixed.

The gitlab_templates have been since day 0 a "starting point" for GitLab CI for contrib modules, not a static template that will make things work now and forever. It'll make it easier, but it can't solve all the problems as the software itself move forwards. At any point, you can or could have taken a snapshot and then modify as needed.

Yes I agree with #9 but I still think that we need to make some consideration for allowing these config arguments to be altered via variables, and not force the maintainer to modify their phpunit.xml. The main problem is that it affects projects where their 3rd-party testing requirements are not fixed yet, and it is those which are causing the failures. If we had variables for those config settings they could be off by default in the current variant and on by default in 'max php' and 'next major'. If we make the maintainer change their phpunit.xml then this would affect testing in all variants, which would likely not satify anyone.

Happy to continue improving this job (and its configuration). Thanks for the title change, I think it makes sense to see what needs improving rather than focussing on just a few variables.

I think we should evaluate options, like having variables that can be set per variant and that might override settings in a file, or maybe it's time we provide a contrib-specific PHPUnit file(s) per variant adapted to the context.

I updated the issue description but kept the original one.

Sorry if this is completely off-topic for this issue, but I’m unclear why setting a custom ignore file for deprecations via SYMFONY_DEPRECATIONS_HELPER: "ignoreFile=…" doesn’t work to prevent those failures in PHP 8.4 anymore? E.g., in this MR I added such a file for ignoring deprecation warnings triggered by a different Drupal module (since it makes no sense to fail on those), but the “phpunit (max PHP version)” job still fails because of output caused by the deprecation warning.

If there is really currently no way to avoid failing on deprecated code in other contrib modules (except to disable larger swaths of checks such as tests printing output) then I’m very much in favor of adding some mechanism for that. Though I’m not sure whether the proposed changes would actually allow for that or just help you disable failing on output.