Problem/Motivation

Adding the GitLab CI template identified some PHPStan issues:

 ------ ----------------------------------------------------------------------- 
  Line   src/Form/RRSSBSettingsForm.php                                         
 ------ ----------------------------------------------------------------------- 
  244    Relying on entity queries to check access by default is deprecated in  
         drupal:9.2.0 and an error will be thrown from drupal:10.0.0. Call      
         \Drupal\Core\Entity\Query\QueryInterface::accessCheck() with TRUE or   
         FALSE to specify whether access should be checked.                     
         💡 See https://www.drupal.org/node/3201242                             
 ------ ----------------------------------------------------------------------- 
 ------ ---------------------------------------------------------------------- 
  Line   src/Plugin/Block/RRSSBBlock.php                                       
 ------ ---------------------------------------------------------------------- 
  27     \Drupal calls should be avoided in classes, use dependency injection  
         instead                                                               
 ------ ---------------------------------------------------------------------- 
 ------ ---------------------------------------------------------------------- 
  Line   src/Plugin/Block/RRSSBDemoBlock.php                                   
 ------ ---------------------------------------------------------------------- 
  26     \Drupal calls should be avoided in classes, use dependency injection  
         instead                                                               
  29     \Drupal calls should be avoided in classes, use dependency injection  
         instead                                                               
 ------ ----------------------------------------------------------------------

Let's fix these.

Issue fork rrssb-3446295

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

ptmkenny created an issue. See original summary.

ptmkenny opened merge request !14

ptmkenny’s picture

ptmkenny commented

The DI adds a lot of lines, but once D9 support is dropped and PHP 8 is required, we can make use of php 8 constructor property promotion.

ptmkenny’s picture

ptmkenny commented
Status: Active » Needs review
adamps’s picture

adamps commented
Status: Needs review » Needs work

Relying on entity queries to check access by default is deprecated in drupal:9.2.0 and an error will be thrown from drupal:10.0.0.

So this first one is presumably a bug.

The other 3 are all insisting on DI. Personally I don't really agree when it adds loads of code and anyway does anyone care😃? It seems like we are responding to the automatic warning rather than changing it because someone actually wants it. Also the constructor change would break any code that extends the class.

The DI adds a lot of lines, but once D9 support is dropped and PHP 8 is required,

D9 is already EOL and no longer supported and it could safely be removed from the info file.

ptmkenny’s picture

ptmkenny commented

If D9 support is dropped we can do constructor promotion to wipe out most of the boilerplate code, but that will still change the constructor, breaking the class for anyone extending it.

Since this needs discussion, I'll open a separate issue to fix the query access check.

ptmkenny’s picture

ptmkenny commented
Related issues: +#3453055: entityQuery on settings form is missing an access check
adamps’s picture

adamps commented

In theory we ought to create a major release for a non-BC change, however it seems a bit excessive in this case.

adamps’s picture

adamps commented

OK here's as idea. I feel that it would be worth creating a new major release if we did a more widespread update of the entire module to use PHP8. We could adopt constructor promotion, add types to class variable and function returns, and anything else relevant. It's not a big module so it probably wouldn't take very long.

Otherwise we could postpone this issue for now until there is another reason to create a major release.

ptmkenny’s picture

ptmkenny commented
Status: Needs work » Postponed

@AdamPS: Agreed, going full PHP 8 seems worthy of a new major release. Having done this with a few other modules recently, it should be quite easy. I assume that should be a new issue ("require php 8")?

adamps’s picture

adamps commented

A new issue sounds good to me thanks

ptmkenny’s picture

ptmkenny commented
Version: 8.x-2.x-dev » 3.x-dev
Status: Postponed » Needs work

Let's handle this in 3.x. There are now some Drupal 12/13 deprecations to address as well.

ptmkenny closed merge request !14

ptmkenny opened merge request !22

ptmkenny’s picture

ptmkenny commented
Title: Fix phpstan issues » Fix phpstan test and address Drupal 12/13 deprecations
Status: Needs work » Needs review
ptmkenny’s picture

ptmkenny commented
Priority: Minor » Normal

  • ptmkenny committed aae364dd on 3.x 
    task: #3446295 Fix phpstan test and address Drupal 12/13 deprecations...
ptmkenny’s picture

ptmkenny commented
Status: Needs review » Fixed

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

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