Replace calls to ::expectWarning*() from Drupal\Tests\Core\EventSubscriber\SpecialAttributesRouteSubscriberTest

Created on 11 March 2024, about 1 month ago
Updated 9 April 2024, 3 days ago

Problem/Motivation

PHPUnit 9 deprecated ::expectError*() and ::expectWarning*()methods. They're removed from PHPUnit 10.

Proposed resolution

Replace trigger_error() with an exception. Invalid route variables should not be allowed.

🐛 Bug report
Status

Fixed

Version

11.0 🔥

Component
PHPUnit 

Last updated about 6 hours ago

Created by

🇮🇹Italy mondrake 🇮🇹

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @mondrake
  • Merge request !7296Closes #3427176 → (Open) created by mondrake
  • Pipeline finished with Failed
    10 days ago
    Total: 178s
    #135973
  • Assigned to mondrake
  • Status changed to Needs work 10 days ago
  • 🇮🇹Italy mondrake 🇮🇹

    Working on this.

    @dineshkumarbollu thanks for your patch. Unfortunately that wion't work on PHPUnit 10 whose error handler no longer converts warnings/errors to exceptions. Please also not that now the preferred way to contribute is by opening Merge Requests; the patch workflow is being abandoned.

  • Pipeline finished with Failed
    10 days ago
    Total: 185s
    #135980
  • Pipeline finished with Failed
    10 days ago
    #135995
  • 🇮🇳India dineshkumarbollu

    @mondrake, Thanks for your reply, I will make MR's instead of patches from next time.

  • Pipeline finished with Failed
    10 days ago
    Total: 618s
    #136234
  • Pipeline finished with Success
    10 days ago
    Total: 1286s
    #136347
  • Issue was unassigned.
  • Status changed to Needs review 10 days ago
  • 🇮🇹Italy mondrake 🇮🇹
  • 🇮🇹Italy mondrake 🇮🇹
  • Status changed to Needs work 9 days ago
  • 🇬🇧United Kingdom longwave UK

    IMHO this should just be an exception, if the variable names aren't allowed we should prevent route building.

  • 🇮🇹Italy mondrake 🇮🇹
  • Status changed to Needs review 9 days ago
  • 🇬🇧United Kingdom longwave UK
  • 🇬🇧United Kingdom longwave UK
  • Pipeline finished with Success
    9 days ago
    Total: 1852s
    #137401
  • Status changed to RTBC 4 days ago
  • 🇺🇸United States smustgrave

    Seems straight forward with updating to use an exception vs trigger_error. All green so believe the update is good.

  • 🇬🇧United Kingdom catch

    Agreed with this but do we need a CR for 11.x to say the behaviour's changed? On the other hand the PHP warning should already be pretty loud.

  • 🇬🇧United Kingdom longwave UK

    Personally, I think no; if you were triggering this before you were doing something wrong, and now we're just a lot louder about it.

  • 🇬🇧United Kingdom longwave UK

    @catch you previously agreed an exception was OK here but Crell and sun went the other way in the end! #2051877-31: Log error when people use invalid route parameters

    • catch committed e4e981a8 on 11.x
      Issue #3427176 by mondrake, longwave: Replace calls to ::expectWarning...
  • Status changed to Fixed 3 days ago
  • 🇬🇧United Kingdom catch

    OK I think the PHP warning having been in for 11 years means people should have found any bugs like this by now. I do wonder a bit how we'll handle new exceptions from existing code in the future, but maybe @trigger_error() and assert() in a minor then throw exception in the next major would work for most things? We didn't have either deprecation support or assert() when we first started doing this pattern.

    Committed/pushed to 11.x, thanks!

Production build https://api.contrib.social 0.62.1