Problem/Motivation

#3555468: Recent commits to *.3.x branches break BC fixed some compatibility issues for 10.x under the 3.3.x branch. However, there are some outstanding PHPStan warnings that weren't blockers, but should be fixed.

Steps to reproduce

See pipeline for phpstan (previous major).

Proposed resolution

Some of the issues can be fixed with a before_script, to remove the attributes before phpstan (previous major) runs:

phpstan (previous major):
  before_script:
    - if [[ "$DRUPAL_CORE" =~ ^10 ]]; then
    -   sed -i '/#\[Legacy.*\]/d' $DRUPAL_PROJECT_FOLDER/*.module $DRUPAL_PROJECT_FOLDER/*.inc $DRUPAL_PROJECT_FOLDER/modules/*/*.module $DRUPAL_PROJECT_FOLDER/tests/modules/*/*.module
    -   sed -i '/^.*#\[\(Hook\)\(.*\)\].*$/d' $DRUPAL_PROJECT_FOLDER/src/Hook/*.php $DRUPAL_PROJECT_FOLDER/modules/*/src/Hook/*.php $DRUPAL_PROJECT_FOLDER/tests/modules/*/src/Hook/*.php
    - fi

There are some other problems that will need further investigation. These stem from changes to the (expected) warnings generated by phpstan v1 vs. v2, changed in #3533155: Fix CI after core updating to newer version of phpunit.. We need to conditionally define the expected warnings based on which phpstan version we're using.

Remaining tasks

  1. Add before script as described above.
  2. Investigate other warnings.

User interface changes

None.

API changes

None.

Data model changes

None.

Issue fork group-3555505

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

lostcarpark created an issue. See original summary.

dww made their first commit to this issue’s fork.

dww opened merge request !271

dww’s picture

dww
we/he/they
commented
Issue summary: View changes
Status: Active » Needs work

I pushed a commit to the issue fork from @lostcarpark in the parent issue's MR branch. All the LegacyHook stuff should hopefully now be resolved here. However, there are still other phpstan errors when testing against 10.5.x core, so calling this NW for now.

dww’s picture

dww
we/he/they
commented

Hrm, looking at phpstan.neon, it's clear that we're trying to silence deprecation warnings about all this GroupMembership and GroupMembershipLoader stuff:

    # Can only remove use of membership loader in 4.0.0
    ...

So I kinda think we're "barking up the wrong tree" here. The phpstan job is also failing (only in 10.5.x) due to:

     Ignored error pattern #^Access to constant on deprecated class            
     Drupal\\group\\GroupMembership\:# was not matched in reported errors.     
     Ignored error pattern #^Access to constant on deprecated interface        
     Drupal\\group\\GroupMembershipLoaderInterface\:# was not matched in       
     reported errors.                                                          
     Ignored error pattern #^Call to method [a-zA-Z]+\(\) of deprecated class  
     Drupal\\group\\GroupMembership\:# was not matched in reported errors.     
     Ignored error pattern #^Call to method [a-zA-Z]+\(\) of deprecated        
     interface Drupal\\group\\GroupMembershipLoaderInterface\:# was not        
     matched in reported errors.

This was most recently touched in #3533155: Fix CI after core updating to newer version of phpunit.:

diff --git a/phpstan.neon b/phpstan.neon
index e659a0d..8f661e5 100644
--- a/phpstan.neon
+++ b/phpstan.neon
@@ -11,8 +11,10 @@ parameters:
     # Ignore common errors for now.
     - "#Drupal calls should be avoided in classes, use dependency injection instead#"
     # Can only remove use of membership loader in 4.0.0
-    - "#^Fetching class constant class of deprecated class Drupal\\\\group\\\\GroupMembership\\:#"
-    - "#^Fetching class constant class of deprecated interface Drupal\\\\group\\\\GroupMembershipLoaderInterface\\:#"
+    - "#^Access to constant on deprecated class Drupal\\\\group\\\\GroupMembership\\:#"
+    - "#^Access to constant on deprecated interface Drupal\\\\group\\\\GroupMembershipLoaderInterface\\:#"
+    - "#^Call to method [a-zA-Z]+\\(\\) of deprecated class Drupal\\\\group\\\\GroupMembership\\:#"
+    - "#^Call to method [a-zA-Z]+\\(\\) of deprecated interface Drupal\\\\group\\\\GroupMembershipLoaderInterface\\:#"
     - "#has typehint with deprecated class Drupal\\\\group\\\\GroupMembership\\:#"
     - "#has typehint with deprecated interface Drupal\\\\group\\\\GroupMembershipLoaderInterface\\:#"
     - "#^Constructor of class Drupal\\\\group\\\\GroupMembershipLoader has an unused parameter#"

So instead of changing the test code at all, I think we need to consider something like https://git.drupalcode.org/project/project_browser/-/blob/2.0.x/phpstan-... and have a separate phpstan.neon include file for D10 vs. D11 so we're accurately ignoring the right warnings depending on the phpstan version we're running. More or less.

dww’s picture

dww
we/he/they
commented
Status: Needs work » Needs review
Related issues: +#3533155: Fix CI after core updating to newer version of phpunit.

Pushed some commits for that. Let's see what the bot thinks now...

dww’s picture

dww
we/he/they
commented
Issue summary: View changes

Huzzah, that's working. 🎉 The MR is now not touching any code, only the phpstan config and .gitlab-ci.yml. So I think this is safe and entirely non-disruptive. However, I'll definitely wait for someone else to RTBC before I commit this.

lostcarpark’s picture

lostcarpark commented
Status: Needs review » Reviewed & tested by the community

I have reviewed the changes in this MR.

The changes are only to PHPStan ignore rules, to cause specific errors to be ignored, specifically to skip known safe deprecation warnings, and for Drupal 10, to ignore warnings about hook attributes.

I haven't done any manual testing, but as there is no change to the module code, there should be no change to module behaviour, so manual testing should not be necessary to validate it.

I am happy that this change will correctly suppress hook messages for Drupal 10 only, and will suppress deprecation warnings appropriately for each version.

Moving to RTBC.

amateescu’s picture

amateescu commented

Reviewed the MR and it looks great to me! Posted a couple of comments, one for wrong indentation in the -v1 file and the other for a cosmetic change.

Leaving at RTBC because those points can be accepted (or not) before merging.

kristiaanvandeneynde made their first commit to this issue’s fork.

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
commented
Status: Reviewed & tested by the community » Fixed

Thanks all!

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.