Problem/Motivation

We're testing deprecation messages using Symfony's PHPUnit bridge. We're doing so in ways that were not envisaged by the original design and we need the ability to dynamically set expected deprecation messages. I've opened a PR to do this upstream but we can achieve this in Drupal by using a trait.

This came up in #2858482: Simplify REST routing: disallow requesting POST/PATCH in any format, make consistent that is adding tonnes of messages to \Drupal\Tests\Listeners\DeprecationListenerTrait::getSkippedDeprecations() which is not really want we want. We need to be removing messages from there and not adding them.

Here's the related symfony PR: https://github.com/symfony/symfony/pull/25757

Proposed resolution

Add a trait so people can do $this->setExpectedDeprecation() in tests.

Remaining tasks

User interface changes

None

API changes

Addition to tests and probably only the tests that use the trait. We could use the trait in the base classes but that might not be worth it.

Data model changes

None

CommentFileSizeAuthor
#10 2935755-10.patch4.81 KBalexpott
#10 7-10-interdiff.txt2.89 KBalexpott
#7 2935755-7.patch4.89 KBalexpott
#7 4-7-interdiff.txt618 bytesalexpott
#4 2935755-3.patch4.65 KBalexpott
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Thunder commented
Issue summary: View changes
Related issues: +#2858482: Simplify REST routing: disallow requesting POST/PATCH in any format, make consistent
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Thunder commented
Issue summary: View changes
Status: Active » Needs review

Here's a trait that can give us the functionality without upstream changes (https://github.com/symfony/symfony/pull/25757).

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Thunder commented
FileSize
2935755-3.patch4.65 KB
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +@deprecated

We can land this now and remove it if https://github.com/symfony/symfony/pull/25757 lands and when we update to a Symfony version that includes it? :)

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented 
+++ b/core/tests/Drupal/Tests/Traits/SetExpectedDeprecationTrait.php
@@ -0,0 +1,85 @@
+trait SetExpectedDeprecationTrait {

If we do commit this, we should:

  1. mark this @internal
  2. add a @todo here to remove this once https://github.com/symfony/symfony/pull/25757 lands and we update to a Symfony release that includes it
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Thunder commented
FileSize
4-7-interdiff.txt618 bytes
2935755-7.patch4.89 KB

Here's #6 done.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

👌

neclimdul’s picture

neclimdul
He/Him
CreditAttribution: neclimdul commented

I've been thinking about this in a similar issue for assert() expectations, should we use the phpunit 6 style expectDeprecation() style or the older setExpectedDeprecation() style or provide both?

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Thunder commented
Status: Reviewed & tested by the community » Needs review
FileSize
7-10-interdiff.txt2.89 KB
2935755-10.patch4.81 KB

@neclimdul I think it makes sense to only support the latest naming. Eventually we'll move off PHPUnit 4 so aiming for PHPUnit 6 will be more consistent. Happy to add a setExpectedDeprecation() if people feel having both styles is important.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Thunder commented
neclimdul’s picture

neclimdul
He/Him
CreditAttribution: neclimdul commented
Status: Needs review » Reviewed & tested by the community

not the prettiest patch with all that reflection but you work with the interface you have. 👍

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Thunder commented

@neclimdul or don't have. Symfony have quite reasonably moved my related PR to Symfony 4.1 and new features are not being added to Symfony 3 so we might need to try out this solution for the time being. That said it might be possible to use the 4.1 version of PHPUnit bridge because it has no dependencies per se.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

We'll need to ensure that https://github.com/symfony/symfony/pull/25757 also includes expectDeprecation() then, otherwise the @todo is wrong.

I'd update https://github.com/symfony/symfony/pull/25757, but … GitHub is down…

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Thunder commented

@Wim Leers I already did that this morning :)

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

OF COURSE you did!

❤️ Alex Pott 👌

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented
Status: Reviewed & tested by the community » Fixed

The reflection here is a shame, but its the best we can do with what we currently have.

And the phpunit version dance is the same deal, and consistent with other code in core.

There is no impact here, other than tests but it does allow us to keep our exception list lean.

Committed 75f9e43 and pushed to 8.6.x

Cherry-picked as 86b363e and pushed to 8.5.x

#2858482: Simplify REST routing: disallow requesting POST/PATCH in any format, make consistent is in too, so can we get a follow-up to remove that large list and use this new API.

Thanks!

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Related issues: +#2936704: Remove REST route deprecations from DeprecationListenerTrait::getSkippedDeprecations(), use ExpectDeprecationTrait::expectDeprecation() instead

Created #2936704: Remove REST route deprecations from DeprecationListenerTrait::getSkippedDeprecations(), use ExpectDeprecationTrait::expectDeprecation() instead for #17.

  • larowlan committed 75f9e43 on 8.6.x 
    Issue #2935755 by alexpott, Wim Leers: Add a trait to allow dynamic...

  • larowlan committed 86b363e on 8.5.x 
    Issue #2935755 by alexpott, Wim Leers: Add a trait to allow dynamic...

Status: Fixed » Closed (fixed)

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