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:

  1. In src/EventSubscriber/OpenIDConnectAutoLogin.php, replace $route_name = $request->get(RouteObjectInterface::ROUTE_NAME); with $route_name = RouteMatch::createFromRequest($request)->getRouteName(); (and update the use statement).
  2. In openid_connect.module, replace user_load_by_mail() and user_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:

  1. src/Form/UserLogoutConfirmation.phpreadonly property is incompatible with the parent class's DependencySerializationTrait on PHP < 8.4.
  2. src/Service/LogoutService.phpaddCalled with a string instead of the <code>GeneratedUrl.
  3. tests/src/Unit/OpenIDConnectEntityConverterTest.php — description placed after @covers, so it is parsed as part of the annotation.
  4. drupal.entityStoragePropertyAssignment in src/OpenIDConnect.php and two unit tests. Suppress inline rather than refactor: removing the protected $userStorage property 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

  1. Confirm the phpstan and phpstan (next minor) jobs pass in CI.
  2. Open a follow-up for phpstan (next major) /

User interface changes

None.

API changes

None.

Data model changes

None.

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:

Comments

benjifisher created an issue. See original summary.

benjifisher’s picture

Issue summary: View changes

It is a little tricky to fix this deprecation. The message I get (from GitLab CI or from running phpstan locally) is

Line src/EventSubscriber/OpenIDConnectAutoLogin.php
------ -----------------------------------------------------------------------------------------
211 Call to deprecated method get() of class Symfony\Component\HttpFoundation\Request:
since Symfony 7.4, use properties `->attributes`, `query` or `request` directly instead
🪪 method.deprecated

So the first step is to figure out whether to use attributes or query or request. The line in question is

    $route_name = $request->get(RouteObjectInterface::ROUTE_NAME);

and RouteObjectInterface is in the Drupal\Core\Routing namespace. So check that namespace for usage hints:

$ grep -r ROUTE_NAME core/lib/Drupal/Core/Routing
core/lib/Drupal/Core/Routing/RouteObjectInterface.php:  const ROUTE_NAME = '_route';
core/lib/Drupal/Core/Routing/UrlMatcher.php:    $attributes[RouteObjectInterface::ROUTE_NAME] = $name;
core/lib/Drupal/Core/Routing/RouteMatch.php:        $request->attributes->get(RouteObjectInterface::ROUTE_NAME),

It sure looks like an attribute!

My first thought was to replace the offending line with

    $route_name = $request->attributes->get(RouteObjectInterface::ROUTE_NAME);

That works: PHPStan is happy (except for the next-minor and next-major CI jobs).

Then I had another idea: use the RouteMatch class instead of the low-level Symfony objects. This works, and it makes the line a little shorter:

    $route_name = RouteMatch::createFromRequest($request)->getRouteName();

(Also replace the use statement.)

benjifisher’s picture

Title: Fix deprecated method call so that PHPStan passes » Fix deprecations so that PHPStan CI jobs pass
Assigned: benjifisher » Unassigned
Issue summary: View changes
Status: Active » Needs review
Related issues: +#3561671: [meta] Refactor tests to use stubs instead of mocks where mocks do not configure expectations

The first change gets the phpstan CI job to pass.

It is not too hard to get the phpstan (next minor) CI job to pass. The messages are

Line openid_connect.module
------ ----------------------------------------------------------------------------
165 Call to deprecated function user_load_by_mail():
in drupal:11.4.0 and is removed from drupal:13.0.0.
Use \Drupal::entityTypeManager()->getStorage('user')->loadByProperties()
instead.
🪪 function.deprecated
168 Call to deprecated function user_load_by_name():
in drupal:11.4.0 and is removed from drupal:13.0.0.
Use \Drupal::entityTypeManager()->getStorage('user')->loadByProperties()
instead.
🪪 function.deprecated

This 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):

Line tests/src/Unit/OpenIDConnectTest.php
------ ----------------------------------------------------------------------
192 Call to deprecated method any() of class PHPUnit\Framework\TestCase:
https://github.com/sebastianbergmann/phpunit/issues/6461
🪪 method.deprecated

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.

benjifisher’s picture

Issue summary: View changes

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

pfrilling’s picture

Status: Needs review » Reviewed & tested by the community

The changes looked good. I decided to go a step further:

  1. Added test coverage for _openid_connect_user_pass_form_validate(), which had none: both lookups, the reset([])FALSE fallback, the openid connect set own password bypass, and the no-match path.
  2. Added a unit test for the NullRouteMatch branch of isLoginRequested() — a functional test can't reach it, since RouterListener throws at REQUEST priority 32 and this subscriber is at 0.
  3. Added user.register, user.pass, and a negative case on openid_connect.login to AutoLoginTest.
  4. Confirmed the new tests also pass against the reverted code, so they're equivalence guards rather than failing-before-fix tests.

Also cleared the remaining PHPStan failures. All six were pre-existing on 3.x, not from the deprecation changes:

  1. UserLogoutConfirmation.php — dropped readonly; the parent's DependencySerializationTrait::__wakeup() can't reinitialize a child's readonly property on PHP < 8.4.
  2. LogoutService.php — a real bug: getGeneratedUrl() returns a string, so addCacheableDependency() was discarding the cacheability toString(TRUE) had just collected.
  3. OpenIDConnectEntityConverterTest.php — moved the description above @covers, which PHPStan was parsing as part of the annotation.
  4. drupal.entityStoragePropertyAssignment (3 sites) — inline @phpstan-ignore instead of refactoring. Removing OpenIDConnect::$userStorage would be a BC break for subclasses of a non-final service; the two test hits hold mocks that every test method uses.
pfrilling’s picture

Issue summary: View changes
Status: Reviewed & tested by the community » Fixed

Merged. I also updated the issue summary with the changes that were made.

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.