search

localgovdrupal / localgov_shared_workflows

GNU General Public License v2.0
0 stars 0 forks source link

Could Drupal 12 deprecations be flagged as warnings instead of errors (fails) #2

Closed andybroomfield closed 2 weeks ago

andybroomfield commented 1 month ago

Noting that this test run flags up a deprecation error (actually fixed in https://github.com/localgovdrupal/localgov_alert_banner/pull/333).

Error: Call to deprecated method renderPlain() of class Drupal\Core\Render\Renderer:
[13](https://github.com/localgovdrupal/localgov_alert_banner/actions/runs/9810768401/job/27091673422?pr=346#step:4:14)
in drupal:10.3.0 and is removed from drupal:12.0.0. Use
[14](https://github.com/localgovdrupal/localgov_alert_banner/actions/runs/9810768401/job/27091673422?pr=346#step:4:15)
  \Drupal\Core\Render\RendererInterface::renderInIsolation() instead.
[15](https://github.com/localgovdrupal/localgov_alert_banner/actions/runs/9810768401/job/27091673422?pr=346#step:4:16)
 ------ ----------------------------------------------------------------------- 
[16](https://github.com/localgovdrupal/localgov_alert_banner/actions/runs/9810768401/job/27091673422?pr=346#step:4:17)
  Line   src/Controller/AlertBannerEntityController.php                         
[17](https://github.com/localgovdrupal/localgov_alert_banner/actions/runs/9810768401/job/27091673422?pr=346#step:4:18)
 ------ ----------------------------------------------------------------------- 
[18](https://github.com/localgovdrupal/localgov_alert_banner/actions/runs/9810768401/job/27091673422?pr=346#step:4:19)
  142    Call to deprecated method renderPlain() of class                       
[19](https://github.com/localgovdrupal/localgov_alert_banner/actions/runs/9810768401/job/27091673422?pr=346#step:4:20)
         Drupal\Core\Render\Renderer:                                           
[20](https://github.com/localgovdrupal/localgov_alert_banner/actions/runs/9810768401/job/27091673422?pr=346#step:4:21)
         in drupal:10.3.0 and is removed from drupal:12.0.0. Use                
[21](https://github.com/localgovdrupal/localgov_alert_banner/actions/runs/9810768401/job/27091673422?pr=346#step:4:22)
           \Drupal\Core\Render\RendererInterface::renderInIsolation() instead.  
[22](https://github.com/localgovdrupal/localgov_alert_banner/actions/runs/9810768401/job/27091673422?pr=346#step:4:23)
 ------ -----------------------------------------------------------------------

It would be good to be able to have these flagged as warnings, as the impact won't be till Drupal 12 in 2026.

millnut commented 1 month ago

There are a couple of things at play here, firstly PHPStan doesn't have the concept of notice or warning types everything is an error type, secondly, GitHub does a very poor UX job if you make a step warning/continue-on-error (it will still mark it as a success even if it errors) unlike GitLab which will visually show the difference with a yellow warning.

Given it's deprecated in 10.3 we can use the DeprecationHelper in Drupal which we did similar in localgov_base https://github.com/localgovdrupal/localgov_base/pull/567/files#diff-f9772c1a7f4e40b1fa6f9272c087adbfef38038d1d182a377d91d6993ac01255 for the same error or we could use a phpstan ignore line similar to this with a FIXME note to fix in future as we have done in the past e.g. https://github.com/localgovdrupal/localgov_guides/blob/b63fa1db95c02d3dec6e677a7ea03bee991ee3c1/tests/src/Kernel/OverviewPageIntegrity.php#L21.

As a personal preference now that the functionality is available we should use the DeprecationHelper.

millnut commented 2 weeks ago

Closing this as the PR https://github.com/localgovdrupal/localgov_project/pull/153 adds ignores for Drupal 12 deprecations