localgovdrupal / localgov_project

Project template for Drupal 10 sites built with the LocalGov Drupal distribution.
https://localgovdrupal.org
GNU General Public License v2.0
10 stars 8 forks source link

Set environment variable SYMFONY_DEPRECATIONS_HELPER for phpunit #157

Open finnlewis opened 4 months ago

finnlewis commented 4 months ago

Set environment variable SYMFONY_DEPRECATIONS_HELPER for phpunit to disabled to suppress deprecation notices in phpunit output.

millnut commented 4 months ago

Hi @finnlewis I think we shouldn't change this as we'd miss deprecations that might not be picked up by automated tooling and can only be flagged at runtime, so it is good to have these reported so they can be looked at, logged and fixed. Looking at Drupal core they also don't have this set up.

ekes commented 4 months ago

Question from Merge Tuesday. Is this possible to split out into another check run by github actions that raises a warning, but separate from the rest of the phpunit tests?

finnlewis commented 4 months ago

My main motivation here is to be able to see the results of the phpunit tests without the long list of deprecations.

Currently we've not been doing anything about the deprecations, so it gets in the way of seeing the test output, both locally and on Github.

I had wrongly assumed that these deprecations would be picked up in the phpstan job.

So for local test config, maybe we can enable this variable in the ddev or lando environments.

And/or we could look at running a separate job for the phpunit tests, suppressing deprecation notices, and one specifically for the deprecation notices. I've not found a way to do that just yet, any ideas?

millnut commented 4 months ago

Hi @finnlewis I'll take a look at this for local vs pipeline and create some tickets for the deprecations which mainly look D11 related, so makes sense we fix now rather than tests breaking when Drupal 11 is released.

millnut commented 3 months ago

Hi @finnlewis @ekes I've run the full tests locally with phpunit and it has come back with a lot of deprecations ~6250 which can't be detected by phpstan or upgrade status which makes the display of these important and should be fixed. I'm not sure if paratest is hiding these or something else on the workflows.

The majority of these seem to be a change in the install yaml file for each module which is due to be removed in Drupal 11. I'll give a brief overview below which we can discuss and then break into separate tickets to investigate further.

  1. Using a translatable string as a category for field type is deprecated in drupal:10.2.0 and is removed from drupal:11.0.0. See https://www.drupal.org/node/3375748
  2. The definition for the 'core.entity_form_display.paragraph.localgov_contact.default.third_party_settings.field_group' sequence declares the type of its items in a way that is deprecated in drupal:8.0.0 and is removed from drupal:11.0.0. See https://www.drupal.org/node/2442603. There is a lot of module config affected by this one, but the Drupal change record doesn't state what happens if the config is left as is and run on Drupal 11. Some examples of other modules fixing this https://www.drupal.org/project/pathauto/issues/3411043 and https://www.drupal.org/project/field_group/issues/3409719
  3. The core/js-cookie asset library is deprecated in Drupal 10.1.0 and will be removed in Drupal 11.0.0. There is no replacement. See https://www.drupal.org/node/3322720 I've created a PR for this on alert banner
  4. The deprecated alter hook hook_search_api_index_items_alter() is implemented in these functions: localgov_directories_search_api_index_items_alter. This hook is deprecated in search_api:8.x-1.14 and is removed from search_api:2.0.0. Please use the "search_api.indexing_items" event instead. See https://www.drupal.org/node/3059866
  5. Defining Drupal\localgov_openreferral\Normalizer\ContentEntityNormalizer::supportedInterfaceOrClass property is deprecated in drupal:10.2.0 and is removed from drupal:11.0.0. Use getSupportedTypes() instead. See https://www.drupal.org/node/
  6. Passing null to the $typedConfigManager parameter of ConfigFormBase::__construct() is deprecated in drupal:10.2.0 and must be an instance of \Drupal\Core\Config\TypedConfigManagerInterface in drupal:11.0.0. See https://www.drupal.org/node/3404140. Seems to come from localgov_review_date
  7. Installing the table sequences with the method KernelTestBase::installSchema() is deprecated in drupal:10.2.0 and is removed from drupal:12.0.0. See https://www.drupal.org/node/3349345. Seems to come from localgov_menu_link_group
finnlewis commented 3 months ago

Crikey, thanks @millnut !

So we do want to keep an eye on these phpunit deprecations. Is there a way to run the phpunit tests twice, once for the actual tests and once for the deprecations? That might make it quicker to scan the results for what we're working on.

millnut commented 3 months ago

Hi @finnlewis based on this it looks like paratest hides the deprecation messages.

Maybe creating a separate workflow that runs the tests using phpunit weekly might be a good first step to surface these, what do you think?