Is SYMFONY_DEPRECATIONS_HELPER: weak correct for contrib deprecation testing needs?

if we did not set SYMFONY_DEPRECATIONS_HELPER at all in the template. That will tell core's PHPUnit bootstrap to use the core's ignore file by default

Is that the case for both _PHPUNIT_CONCURRENT set to 0 (uses phpunit directly) and set to 1 (uses run-tests.sh script from core)?

#4 AFAICS, yes. If the variable is NOT set in the environment at all, when PHPUnit bootstraps (which is happening bot when CLI is invoked directly, and when it's spawned by run-tests.sh), then boostrap.php has this code:

// Ensure ignored deprecation patterns listed in .deprecation-ignore.txt are
// considered in testing.
if (getenv('SYMFONY_DEPRECATIONS_HELPER') === FALSE) {
  $deprecation_ignore_filename = realpath(__DIR__ . "/../.deprecation-ignore.txt");
  putenv("SYMFONY_DEPRECATIONS_HELPER=ignoreFile=$deprecation_ignore_filename");
}

which will load core's .deprecation-ignore.txt.

I'm setting that value as default. I was going to remove the variable, but this is one that we pass explicitly when calling the "run-tests.sh", so I decided to keep it and change the value to the default core's one.

In order to test, you can replace these two lines in your ".gitlab-ci.yml" file:

  - project: $_GITLAB_TEMPLATES_REPO
    ref: $_GITLAB_TEMPLATES_REF

for

  - project: issue/gitlab_templates-3400979
    ref: 3400979-deprecations-helper-value

Thank you!

Testing in 📌 [CI] testing issue Active . I am expecting that testing Image Effects module will be full green with D10.1, but with D10.2, will result in a bunch of deprecations.

1. Just remove my own override of SYMFONY_DEPRECATIONS_HELPER, with current HEAD --> all tests pass, NOT OK

https://git.drupalcode.org/project/image_effects/-/pipelines/48754

2. Replace the included project with this branch one -->D10.1 tests pass, D10.2 tests fail with deprecation info, OK

https://git.drupalcode.org/project/image_effects/-/pipelines/48758

3. Readd override of SYMFONY_DEPRECATIONS_HELPER, to 'disabled' --> all tests pass, OK

https://git.drupalcode.org/project/image_effects/-/pipelines/48770

To me this is exactly how it should be. Not RTBCing yet, I think this deserves more +1s from others.

Is this failing the test when deprecations happen, or just reporting them? IMO deprecations are advisory only and should not result in a phpunit job failure by default. Also see https://drupal.slack.com/archives/C51GNJG91/p1698355817936279 for a discussion of better presentation of deprecations in Gitlab UI (move to Code Quality tab).

#9 this is failing the test when the deprecation happens. Like in DrupalCI when choosing suppress-deprecations: false. Overriding with SYMFONY_DEPRECATIONS_HELPER: 'disabled' in the GitlabCI file will result in same as suppress-deprecations: true in DrupalCI. AFAICT.

SYMFONY_DEPRECATIONS_HELPER: 'weak' as currently is in HEAD is neither, nor.

Ran into this over at 📌 Test Webform against upcoming 10.2.x on GitLab CI Needs review .

Note that I had to go from

  SYMFONY_DEPRECATIONS_HELPER: disabled

to

  # Commented out because Webform has opted in to the concurrent test runner aka run-tests.sh, which needs the
  # `--suppress-deprecations` parameter instead to achieve the same.
  # @see https://www.drupal.org/project/gitlab_templates/issues/3370952
  # SYMFONY_DEPRECATIONS_HELPER: disabled
  _PHPUNIT_CONCURRENT: "1"
  _PHPUNIT_EXTRA: --suppress-deprecations

to be able to use concurrent test runs while disabling deprecations.

See https://git.drupalcode.org/issue/webform-3402134/-/commit/b105edca2f81f6... and its test results for phpunit (next minor) vs the preceding commit.

I just saw #3400218: [GitlabCI] run-tests.sh does not pass on SYMFONY_DEPRECATIONS_HELPER environment variable value to spawned processes → . As you can see in the test results linked in #11, that did not actually work? 🤔

This issue was about passing the parameter, and as you can see in the (very useful) output in this line https://git.drupalcode.org/issue/webform-3402134/-/jobs/352378#L50, we are passing the value, but it's "weak".

Also, this MR needed a rebase, to have that other issue in it. So it should work now without the _PHPUNIT_EXTRA: --suppress-deprecations (which is a very valid option btw and could stay).

(which is a very valid option btw and could stay).

I disagree it should stay, because --suppress-deprecations is not actually a PHPUnit parameter. That makes this extremely confusing 🙈

I meant that it was a valid workaround and that is totally valid to pass options to "run-tests.sh" like that.

It's confusing due to the variable name because of this CONCURRENCY parameter, where 0 is "phpunit" (and _PHPUNIT_EXTRA has options for it), and 1 is "run-tests.sh" (and _PHPUNIT_EXTRA has options for it).

In any case, the rebase should have helped, didn't it?

+1 on #14

There already were too many places where that variable can be manipulated (env itself, phpunit.xml, bootstrap.php, run-tests.sh), now GitlabCI itself adds one; I'd rather drop --suppress-deprecations from run-test.sh, at one point we will have to trim features out of it and just keep for concurrency, or even drop it completely on behalf of some parallel runner. But that's not for here.

#16++

Back to this issue. Testing with #7 should show the right value for the variable.

#8 reported the expected results.
Note that the issue mentioned in #11 is not using this fix yet.

Are any further changes needed here? Happy for more people to test it.

Linking related issue.

I haven't had time to analyze this issue closely. Main question is, does this still prevent indirect deprecation causing failures ? (those that occur in a 3rd party library)

Was attempting to validate #7 against a previous run where I had Indirect deprecation reported and hit the following:

phpunit                    > PHPUnit 9.6.13 by Sebastian Bergmann and contributors.
phpunit                    > 
phpunit                    > PHP Fatal error:  During inheritance of Countable: Uncaught InvalidArgumentException: Unknown configuration option "ignoreFile". in /gcl-builds/vendor/symfony/phpunit-bridge/DeprecationErrorHandler/Configuration.php:302

Seems this is only safe for newer Symfony installs (D10+) and may break those sites using the GitLab template to test D9 installs (which would not be uncommon as D9 just went EOL a couple weeks ago and some of the well maintained modules still have support for D8)

#19 the "ignoreFile" configuration option was introduced in Symfony 6.1, https://symfony.com/doc/current/components/phpunit_bridge.html#ignoring-...

Given D8 and D9 are both EOL, is it a valid expectation to have GitlabCI supporting them?

Given D8 and D9 are both EOL, is it a valid expectation to have GitlabCI supporting them?

I would suggest it is in everyone’s best interest to do so as much as possible.

If this were merged today and somone adopted a D9 module tommorow it will make it harder for the adopter to ensure everything t is working properly and will encourage the dev to make it a hard cut of D10 only instead of D9+D10 which makes upgrades for other site owners unnecessarily harder.

I’m not against modules having hard breaks without dual major support, but equally I’m also very much in support of not breaking dual major support unnecessarily

Additionally while we have been focused on adding support for testing bleeding edge, I assume eventually we’re going to see min/max jobs pop up in projects to be absolutely sure they stay fully compatible, those jobs are much more likely to need a wide ranging CI platform.

Ignoring if we should or shouldn’t support older core releases we need to look at all GitLab Templates changes with an eye for BC and opening up new branches for breaking changes.

As a maintainer:
While I expect some pain in my choices to maintain long term support, the more often I get hit by breaking changes the less likely I am to stick with the standard template, that means if I split off I’m less likely to pickup fixes like #3403212: Customize resource requests per job → .

One project isn’t much in that regard, however it adds up, one project here one project there and the next think you know the CI budget is 10% larger.

Agree that we should at least try a bit more to see if there is an option where we can keep compatibility with D8/D9 versions.

Maybe we can see if the file is in the filesystem and change the value of the variable dynamically? Open to suggestions.

I just mentioned this in a thread on slack. I have to go now but hope to come back to this: https://drupal.slack.com/archives/CGKLP028K/p1705005746798639?thread_ts=...

The important difference from the MR is I had to add a small phpunit config file:

 <?xml version="1.0" encoding="UTF-8"?>
 <phpunit xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance">
   <listeners>
     <listener class="\Drupal\Tests\Listeners\DrupalListener">
     </listener>
   </listeners>
 </phpunit>

Also noticed it failing on d9. But it can be surpressed by adding "weak" to previous major (for now)?

Coming back to this one after a while. Maybe we can leave "weak" if the ".deprecation-ignore.txt" does not exist (old version of Drupal), but change the value to use SYMFONY_DEPRECATIONS_HELPER: 'ignoreFile=/builds/project/image_effects/web/core/.deprecation-ignore.txt' if the file exists.

Is this something that might still be wanted?

The related issue 🐛 Gitlab CI testing of core version of the module does not fail on deprecations Closed: duplicate did just that https://git.drupalcode.org/project/automatic_updates/-/merge_requests/10... and it works as expected.

We need to change the approach for BC but it's good to know that the suggested fix works.

fjgarlin → closed merge request !86

fjgarlin → changed the visibility of the branch 1.0.x to hidden.

fjgarlin → changed the visibility of the branch main to hidden.

fjgarlin → changed the visibility of the branch 3400979-use-core-file-if-present to hidden.

MR ready for review: https://git.drupalcode.org/project/gitlab_templates/-/merge_requests/162

I changed the default to the core file and added a fallback if the file is not present.

Running on D9 (where that file is not present): https://git.drupalcode.org/project/keycdn/-/jobs/1112342
Running on D10: https://git.drupalcode.org/project/keycdn/-/jobs/1112340

See the output line where it shows the value used (which is going to be removed in the next commit as it was only for debug purposes).

However, doing this would disallow any module maintainer to override the variable, which at the moment is possible. So I added another wrapping "if" to see that the value is the default.

D9: https://git.drupalcode.org/project/keycdn/-/jobs/1112400
D10: https://git.drupalcode.org/project/keycdn/-/jobs/1112398

Open question from #19:

How does this new method work with indirect deprecations? Do they cause tests to fail if they occur (and are not in the core ignore file) or are they ignored?

New questions:
Should this consider checking in the module folder for a deprecation ignore file and use that (without need to override settings)?

Is this change a BC break on testing? While it would be arguably a bugfix for tests to have ignored a direct deprecation that has been (mostly) handled by PHPStan.

How does this new method work with indirect deprecations? Do they cause tests to fail if they occur (and are not in the core ignore file) or are they ignored?

I did not know that you could pass this option to phpunit up until this issue, so I am not sure about it. If we want to go ahead with this approach we'd need to test multiple cases.

Should this consider checking in the module folder for a deprecation ignore file and use that (without need to override settings)?

We could, but it's also very easy. We could place the file, name it as we want, and then do:

phpunit:
  variables:
    SYMFONY_DEPRECATIONS_HELPER: "ignoreFile=./.local-deprecation-ignore.txt"

Is this change a BC break on testing? While it would be arguably a bugfix for tests to have ignored a direct deprecation that has been (mostly) handled by PHPStan.

While I am not sure (see first reply...), I think this will definitely change the reported errors or warnings, so it might be that modules that were "green" for phpunit are suddenly not.

--

An alternative that I thought is just to document all this in the https://project.pages.drupalcode.org/gitlab_templates/jobs/phpunit/ page. We can leave the default weak untouched, and document that to change the behaviour, we'd need to do something like this:

phpunit:
  variables:
    SYMFONY_DEPRECATIONS_HELPER: "ignoreFile=$CI_PROJECT_DIR/$_WEB_ROOT/core/.deprecation-ignore.txt"

This way:
* Modules wanting to do the same as core, could set those lines
* Modules wanting to do their own file, could do like the second example above in this comment
* Or they could use another value or even do nothing and leave things as they are now.

--

I actually went ahead and documented this. If we want to keep full BC we can just commit the docs change. If we want to push forward with the change of value, maybe we can do it via a toggle variable, but I see this as complex as just setting the new (and documented) variable.

Given that we can't ensure BC and that green jobs might end up failing if we do this, I've reverted the actual change but documented it instead. If we want to mirror what core does, it's a one-line change.

So, the only changes in the MR now are just documentation changes.

Marking RTBC and committing shortly the documentation change.

Merged the documentation change. It can be seen here: https://project.pages.drupalcode.org/gitlab_templates/jobs/phpunit/

If anyone can think of a way to not break BC, feel free to reopen the issue. I also guess that once D11 is release and we no longer need to worry about D9, we might be able to revisit the topic.

Just wanted to echo my comment from #24:

You would also have to have a minimal phpunit.xml.dist file in the project to get this to trigger. This is illustrated by looking at the drupalci test runs (where 10.x shows the deprecations) for this issue: https://www.drupal.org/project/linkchecker/issues/3438441 📌 use gitlab CI Active . And then comparing the test run before I added the file (https://git.drupalcode.org/project/linkchecker/-/jobs/1241546) to one after i add the file (https://git.drupalcode.org/project/linkchecker/-/jobs/1242345)

This is not reflected in the docs change that was merged. Should I open a MR for this addition somehow?

If there is no file we use a default: https://git.drupalcode.org/project/gitlab_templates/-/blob/main/includes...

I can look into this tomorrow with more detail.

Indeed. Even better. Then that one can just add the Drupal test listener and it will simply work

We have #3427357: Move default phpcs configuration file and clean up unneeded lines in it → where we are changing slightly that default file. Perhaps you want to make a suggestion there to see what needs to be added to the default file (currently this one).

I've also added the suggestion in #24 to the docs: https://git.drupalcode.org/project/gitlab_templates/-/merge_requests/178

I mixed up phpcs and phpunit in the above comments, so I modified the comments to clarify and avoid confusion.
The suggestion was added to the right place in the documentation page: https://project.pages.drupalcode.org/gitlab_templates/jobs/phpunit/#depr...

Something is not quite right with the ```
See annotated screengrab.

The extra space in front of three ``` did not cause any problem when viewing the file in the repo here gitlab_templates-3400979/-/blob/3400979-update-docs/docs/jobs/phpunit.md so it would have been hard to detect unless you have mkdocs locally.

At one point, when you were first introducing the 'Check Code' job, I recall seeing something about markdown checking? But that is not being run in the current version. I will look back and see what it was, and open a new issue if there is anything to do.

It was a markdown-specific spelling checker, which we discarded in favor of using cspell for all.

It would have been really hard for a tool to detect that as it was technically correct I guess. I guess we humans are still useful 🙂

GitLab Formatted Markdown (commonmark base) vs Python Markdown most likely cause. Some discussion in #2952616: Adopt CommonMark spec for Markdown files → .

Not sure if we perhaps want to consider a followup task to render, but not publishing, mkdocs on all MR’s that make changes to allow easier review.

Thanks for the issue, following it now.

We tried to create a doc-site per MR (https://docs.gitlab.com/ee/user/project/pages/#use-multiple-deployments-... and related issue #3426311: Allow testing documentation pages via MRs → ), but our instance and project settings won't allow for that.

In any case, most documentation changes are now really minimal and easy to review (either checking the MD files in the browser or locally like this
https://project.pages.drupalcode.org/gitlab_templates/help/documentation/). We can still miss some things like the space that was fixed in #50 but I think we don't need this extra rendering, at least for now.

Automatically closed - issue fixed for 2 weeks with no activity.

Re-opening this issue as D11.0.0 will be released within 2 months, and the default could be set to the same as core.

Everything is documented in the docs page, but maybe we can change the default and provide a fallback for D9. See this commit for referenced about the previous work: https://git.drupalcode.org/project/gitlab_templates/-/merge_requests/162...

Please review the following:
- MR: https://git.drupalcode.org/project/gitlab_templates/-/merge_requests/207...
- Running on Drupal 9: https://git.drupalcode.org/project/keycdn/-/jobs/1614271 (line 325: "SYMFONY_DEPRECATIONS_HELPER=weak")
- Running on Drupal 10: https://git.drupalcode.org/project/keycdn/-/jobs/1614269 (line 323: "SYMFONY_DEPRECATIONS_HELPER=ignoreFile=/builds/project/keycdn/web/core/.deprecation-ignore.txt")

Changing to just "disabled" for all versions as default value.
- MR: https://git.drupalcode.org/project/gitlab_templates/-/merge_requests/207...
- D10: https://git.drupalcode.org/project/keycdn/-/jobs/1614510
- D9: https://git.drupalcode.org/project/keycdn/-/jobs/1614512

A comment added inline.

Feedback around documentation addressed.

Looks good to me, thanks - I'll leave RTBC to someone that sees this from the outside, I'm too much an insider here :)

Whoa. This is harder than I thought. Looks like phpunit.xml MUST exist, both in D10 and in D11, to get deprecations reported when running PHPUnit straight. In D10 it must include the listener, in D11 the ignoreSuppressionOfDeprecations attribute in the source section. That means version dependent phpunit.xml. Will try on a module and report back here.

Using _PHPUNIT_CONCURRENCY it’s much simpler… you turn it on and run-tests.sh takes care of all the sherpa work.

Adding _PHPUNIT_EXTRA: '-c $CI_PROJECT_DIR/$_WEB_ROOT/core' works. This enforces PHPUnit to read core's configuration phpunit.dist.xml, which itself differs per version so it's accurate to the context. See PoC in 📌 [CI] PHPUnit CLI testing Active .

This would mean:

- for D10 (=PHPUnit 9)

• specifiying the --printer in the cmd line arguments is redundant, the entry is in the xml already
• the listener is already in the xml

- for D11 (=PHPUnit 10+)

• ignoreSuppressionOfDeprecations is already in the xml
• --fail-on-deprecation is needed, since it's not in the xml (but if you do, alas, PHPUnit 9 fails...)
• reporting in testdox format is not colliding anymore with the HTML output logger

Obviously one can have its own phpunit.xml in the contrib root and map the -c cmd line argument to it, and do whatever needed according to PHPUnit docs https://docs.phpunit.de/en/11.0/configuration.html. But then it's needed at least two xmls, one for PHPUnit 9 and one for PHPUnit 10, so the value in _PHPUNIT_EXTRA needs to be variable to $PHPUNIT_VERSION (is that possible?)

Setting to NW because I feel we need some more thinking here.

so the value in _PHPUNIT_EXTRA needs to be variable to $PHPUNIT_VERSION (is that possible?)

Yes, we do that in the templates. For example https://git.drupalcode.org/project/gitlab_templates/-/blob/main/includes...

Also, easy to achieve like this:

variables:
  _PHPUNIT_EXTRA: "default value..."

phpunit (next major):
  variables:
    _PHPUNIT_EXTRA: "value for next major only"

So it kind of feels like all is needed is extra documentation to explain the cases presented in #66, which can be also accompanied with the above code.

Echoing myself and #65.

The current setup still requires a maintainer to add phpunit.xml.dist to the repo (or I guess use the extra option mentioned)

I think it would be nice to either provide this by default or more clearly document this

Happy for people to make suggestion for the documentation in the MR.

Note that if you use _PHPUNIT_CONCURRENT: 1 most of the things we're talking about here just go away. There is an issue to make this the default, that still needs work: #3447087: Switch to using run-tests.sh by default →

I'll try to add the above findings/snippets etc to the documentation but also note that I'm struggling with time on this, so any help would be greatly appreciated, otherwise patience is the next thing that would be greatly appreciated 🙂.

♥️

Just wanted to highlight my comment was not meant as snark or impatience. I see it comes across as a bit short and unfriendly. I was typing on my phone and simply wanted to make sure we did not forget that detail. I will try to find some time for at least doc suggestions this week hopefully

No worries at all @eiriksm, I didn't perceive it that way - it's also Monday morning and I'm going through many notifications, creating MRs for other issues, etc. I appreciate all the comments, help and support from you all.

FYI currently two projects I maintain, Imagemagick and File Metadata Manager, are running tests with this setup

variables:
  _PHPUNIT_CONCURRENT: '0'
  _PHPUNIT_EXTRA: '-c $CI_PROJECT_DIR/$_WEB_ROOT/core --testdox --colors=always'
  OPT_IN_TEST_PREVIOUS_MINOR: '0'
  OPT_IN_TEST_NEXT_MINOR: '1'
  OPT_IN_TEST_NEXT_MAJOR: '1'

phpunit:
  extends: .phpunit-base
  variables:
    SYMFONY_DEPRECATIONS_HELPER: "ignoreFile=$CI_PROJECT_DIR/$_WEB_ROOT/core/.deprecation-ignore.txt"

and results are really nice, for example

PHPUnit 9.6.19 by Sebastian Bergmann and contributors.

Testing /builds/project/file_mdm/web/modules/custom/file_mdm

File Metadata Exif (Drupal\Tests\file_mdm_exif\Kernel\FileMetadataExif)
 ✔ Exif plugin
 ✔ Jpeg exif save to file
 ✔ Tiff exif save to file

File Metadata Font (Drupal\Tests\file_mdm_font\Kernel\FileMetadataFont)
 ✔ Font plugin
 ✔ Supported keys

Settings Form (Drupal\Tests\file_mdm\Functional\SettingsForm)
 ✔ Missing file logging

File Metadata Manager (Drupal\Tests\file_mdm\Kernel\FileMetadataManager)
 ✔ File metadata
 ✔ File metadata caching
 ✔ Remote file set local path
 ✔ Remote file copy
 ✔ Sanitized uri

Time: 00:09.371, Memory: 6.00 MB

OK (11 tests, 436 assertions)

I went through the above comments and I think that everything is captured, one way or another. See the file here: https://git.drupalcode.org/issue/gitlab_templates-3400979/-/blob/3400979... or the MR.

I made a couple of MR suggestions to the help text.

They make perfect sense. Applied. Thanks.

Looks great to me now, thanks.

I've merged this. Thanks all.

Just a quick question. The final change to the docs added
"If you are testing with Drupal 10 (which uses PHPUnit 9), your project needs to have a PHPUnit configuration file to be able to get deprecation warnings."
I think this is only true if you are running phpunit direct via _phpunit_concurent=0. Scheduler does not have a phpunit.xml but I can get deprecation warnings just by setting SYMFONY_DEPRECATIONS_HELPER: "ignoreFile=$CI_PROJECT_DIR/$_WEB_ROOT/core/.deprecation-ignore.txt" provided I also use _PHPUNIT_CONCURRENT: 1

So somehow, in the docs page, this needs to be expained.

I see. I never set that variable so I guess it was a comment regarding using the out of the box setup. As a consequence, I assume since run tests makes these appear maybe we could pass the same arg to the PHP unit command? The listener class, that is

. If you use _PHPUNIT_CONCURRENT: 1, then run-tests.sh takes care of these cases for you. However, if you use _PHPUNIT_CONCURRENT: 0, you might need to adapt the options per variant as shown in the previous section.

https://project.pages.drupalcode.org/gitlab_templates/jobs/phpunit/

Maybe it needs to be clearer but i thought that would cover it.

Happy to this to be made even clearer (maybe on the issue we have to improve docs?).

Automatically closed - issue fixed for 2 weeks with no activity.