Problem/Motivation
The PHPStan tests fail in GitLab CI.
Steps to reproduce
Look at the most recent branch result on https://git.drupalcode.org/project/openid_connect/-/pipelines.
Proposed resolution
Get the phpstan and phpstan (next minor) CI jobs to pass.
Two deprecation fixes:
- In
src/EventSubscriber/OpenIDConnectAutoLogin.php, replace$route_name = $request->get(RouteObjectInterface::ROUTE_NAME);with$route_name = RouteMatch::createFromRequest($request)->getRouteName();(and update theusestatement). - In
openid_connect.module, replaceuser_load_by_mail()anduser_load_by_name()as described in the change record user_load_by_mail() and user_load_by_name() are deprecated.
Since this issue was filed, phpstan-drupal's ruleset has also started reporting six non-deprecation errors that must be cleared for the job to go green:
src/Form/UserLogoutConfirmation.php—readonlyproperty is incompatible with the parent class'sDependencySerializationTraiton PHP < 8.4.src/Service/LogoutService.php—addCalled with a string instead of the <code>GeneratedUrl.tests/src/Unit/OpenIDConnectEntityConverterTest.php— description placed after@covers, so it is parsed as part of the annotation.drupal.entityStoragePropertyAssignmentinsrc/OpenIDConnect.phpand two unit tests. Suppress inline rather than refactor: removing theprotected $userStorageproperty would be a BC break forrvice.
Note that phpstan.neon is not tracked in the repository, so CI uses the GitLab template default. These must be fixed in code or with inline ignores; a local config change will not affect the pipeline.
phpstan (next major) and phpunit (next major)< blocked on PHPUnit <code>any() deprecations intests/src/Unit/OpenIDConnectTest.php, which core only recently addressed in #3561671. Per #3, get 3.x to a stable release first, then fix PHPStan for Drupal 12 in a follow-up.
Remaining tasks
- Confirm the
phpstanandphpstan (next minor)jobs pass in CI. - Open a follow-up for
phpstan (next major)/
User interface changes
None.
API changes
None.
Data model changes
None.
Issue fork openid_connect-3585901
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:
Comments
Comment #3
benjifisherIt is a little tricky to fix this deprecation. The message I get (from GitLab CI or from running
phpstanlocally) isSo the first step is to figure out whether to use
attributesorqueryorrequest. The line in question isand
RouteObjectInterfaceis in theDrupal\Core\Routingnamespace. So check that namespace for usage hints:It sure looks like an attribute!
My first thought was to replace the offending line with
That works: PHPStan is happy (except for the next-minor and next-major CI jobs).
Then I had another idea: use the
RouteMatchclass instead of the low-level Symfony objects. This works, and it makes the line a little shorter:(Also replace the
usestatement.)Comment #4
benjifisherThe first change gets the
phpstanCI job to pass.It is not too hard to get the
phpstan (next minor)CI job to pass. The messages areThis time, there is a change record telling us what to do: user_load_by_mail() and user_load_by_name() are deprecated.
The
phpstan (next major)CI job is harder. There are a bunch of messages like this (all in the same file):These deprecations were only recently fixed in Drupal core: see #3561671: [meta] Refactor tests to use stubs instead of mocks where mocks do not configure expectations. I suggest we first get to a stable release of this module on the 3.x branch, and then worry about fixing PHPStan for Drupal 12.
Comment #5
benjifisherComment #7
pfrillingThe changes looked good. I decided to go a step further:
_openid_connect_user_pass_form_validate(), which had none: both lookups, thereset([])→FALSEfallback, theopenid connect set own passwordbypass, and the no-match path.NullRouteMatchbranch ofisLoginRequested()— a functional test can't reach it, sinceRouterListenerthrows at REQUEST priority 32 and this subscriber is at 0.user.register,user.pass, and a negative case onopenid_connect.logintoAutoLoginTest.Also cleared the remaining PHPStan failures. All six were pre-existing on 3.x, not from the deprecation changes:
UserLogoutConfirmation.php— droppedreadonly; the parent'sDependencySerializationTrait::__wakeup()can't reinitialize a child's readonly property on PHP < 8.4.LogoutService.php— a real bug:getGeneratedUrl()returns a string, soaddCacheableDependency()was discarding the cacheabilitytoString(TRUE)had just collected.OpenIDConnectEntityConverterTest.php— moved the description above@covers, which PHPStan was parsing as part of the annotation.drupal.entityStoragePropertyAssignment(3 sites) — inline@phpstan-ignoreinstead of refactoring. RemovingOpenIDConnect::$userStoragewould be a BC break for subclasses of a non-final service; the two test hits hold mocks that every test method uses.Comment #8
pfrillingMerged. I also updated the issue summary with the changes that were made.