Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PHPUnit deprecation warnings #6

Closed
wants to merge 1 commit into from

Conversation

AaronGilMartinez
Copy link
Collaborator

No description provided.

@donquixote donquixote mentioned this pull request Nov 7, 2024
@@ -8,6 +8,7 @@
<php>
<ini name="error_reporting" value="32767"/>
<ini name="memory_limit" value="-1"/>
<env name="SYMFONY_DEPRECATIONS_HELPER" value="disabled"/>
Copy link
Owner

@donquixote donquixote Nov 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Context

I think the goal of this is to suppress a deprecation warning caused by a contrib module (group).
Details:

  • The warning is emitted in FieldTypePluginManager.php:113, after analyzing a field type definition.
  • The field type is defined in 'group' module, class RelationshipEntityReferenceItem. The problem is the category = @Translation("Reference"), in the annotation.
  • The message is "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." (it does not mention the field type id).
  • The issue exists here and already has a patch: https://www.drupal.org/project/group/issues/3458530

Problem

I am skeptical about this change, because it also suppresses deprecation warnings caused by our own code. It can be valuable to see if we are doing something deprecated.

Actually phpstan or phpcs might find some of these deprecations. Perhaps even this one. But SYMFONY_DEPRECATIONS_HELPER might hide some other deprecations that are based on dynamic conditions, and not visible to static analysis.

A simple way to test is to create a unit test case like below, and run only that test, with and without the <env name="SYMFONY_DEPRECATIONS_HELPER" value="disabled"/> in phpunit.xml.

<?php

declare(strict_types=1);

namespace Drupal\Tests\collabora_online\Unit;

use Drupal\Tests\UnitTestCase;

class ExampleTest extends UnitTestCase {

    public function testDeprecation() {
        // Optionally add or remove the `@` error suppression.
        @trigger_error('Something is deprecated in ' . __METHOD__, E_USER_DEPRECATED);
        $this->addToAssertionCount(1);
    }

}

Alternative

Apply the patch from https://www.drupal.org/project/group/issues/3458530, or help the fix to get committed.
TBD: I think we are mostly interested in the patch for local development, but we don't want it auto-applied in websites that use the module.

Temporary solution

I think it could be ok to keep this commit in the epic branch, and then fix it before we merge the epic branch.
I don't think it should be merged into the 1.x branch as standalone change.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of the current solution, we will apply the patch to disable the deprecation error while testing.
I would not find a way to disable specific paths for deprecations.

@brummbar brummbar closed this Nov 7, 2024
@AaronGilMartinez AaronGilMartinez deleted the PHPUnit-deprecation-warnings branch November 7, 2024 17:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants