Problem/Motivation

The SessionHandlerInterface has type hints now.
Also note that the return value of gc() changed in PHP 7.1:

Returns the number of deleted sessions on success, or false on failure. Note this value is returned internally to PHP for processing.

Steps to reproduce

Proposed resolution

  • Type hint arguments of the SessionHandler.
  • Correct the return value of the gc() method.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3377256

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

znerol created an issue. See original summary.

znerol opened merge request !4483

znerol credited jungle.

znerol’s picture

znerol commented
Status: Active » Needs review

Transferring commit credits from #2536052-20: Remove try...catch from SessionHandler::write and 21.

andypost’s picture

andypost
he/him
Primary language Russian
commented

The only question - should we keep ReturnTypeWillChange attribute?

znerol’s picture

znerol commented

The only question - should we keep ReturnTypeWillChange attribute?

Good question. Let's try without.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative

Typehint didn't seem to cause any issue
New service parameter has a trigger_error and CR.

Could say this seems like a task but will leave as is.

LGTM

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Reviewed & tested by the community » Needs review

Hmm, not convinced the scoping of this issue is correct - this is fixing at least four different issues, but only for this class.

Not sure we can add the return types here? If someone has extended SessionHandler and swapped it in, and replaced one of those methods without adding a return type, there is a BC break, so we have do to this in a major version change I think.

The fix to the gc() return value looks valid.

Bulk conversion to constructor property promotion is being discussed in #3278431: [May 2026] Use PHP 8 constructor property promotion for existing code

Fixing REQUEST_TIME is done in children of #2902895: [meta][no patch] Replace uses of REQUEST_TIME and time() with time service

znerol’s picture

znerol commented

Not sure we can add the return types here?

You are right. We cannot. Is there a way to add deprecations for methods defined in a subclass lacking the return type?

Adding type hints on the arguments is allowed as far as I can tell.

znerol’s picture

znerol commented

Uh, ok. PHP tentative return types will likely cause serious trouble when they get enforced in PHP 9. Unless of course some PHP 8.x release will introduce more warnings for subclasses inheriting from types implementing built-in interfaces. -> https://3v4l.org/1fi6I

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

MR seemed to have CC failure.

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

znerol’s picture

znerol commented
Status: Needs work » Needs review

Looks like the last reroll did not go very well (it partially reverted #2536052: Remove try...catch from SessionHandler::write). Did the reroll again and fixed the CC failure pointed out in #11.

spokje’s picture

spokje
Primary language Dutch
commented
Status: Needs review » Needs work
znerol’s picture

znerol commented

@elber: DI injection is out of scope here, see #8.

znerol’s picture

znerol commented

@elber: Also we regrettably cannot add returns types. Again see #8.

elber’s picture

elber
Primary language Portuguese, Brazil
Location Brazil
commented

Hi @znerol if we removed the typehints methods they aren't compatible with SessionHandlerInterface

znerol’s picture

znerol commented
Status: Needs work » Needs review
znerol’s picture

znerol commented

Hi @znerol if we removed the typehints methods they aren't compatible with SessionHandlerInterface

This is incorrect. SessionHandlerInterface is subject to the PHP tentative return types mechanism.

Regrettably, as per #8 we cannot add return type hints in a minor release (at least not according to the current BC policy).

znerol’s picture

znerol commented
Issue summary: View changes
znerol’s picture

znerol commented

Updated the issue summary. Regrettably I do not have the powers necessary to remove the CR.

znerol’s picture

znerol commented
Issue summary: View changes
smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Left a comment on MR.

elber’s picture

elber
Primary language Portuguese, Brazil
Location Brazil
commented
Status: Needs work » Needs review
smustgrave’s picture

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

Believe this is ready.

  • catch committed 485e4942 on 11.x 
    Issue #3377256 by elber, znerol, smustgrave, longwave, jungle, andypost...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed
Issue tags: +Needs followup

I feel like we could probably check if contrib extends this class, and then go ahead and add return types in a minor release, based on the fact that non-base-classes are not considered @api (per https://www.drupal.org/about/core/policies/core-change-policies/bc-polic...).

However that can be a follow-up, since if someone actually is extending this, it would be better not to break them.

Committed/pushed to 11.x, thanks!

quietone’s picture

quietone commented

A search for SessionHanlder in contrib didn't find any case where it was extended.

Setting to NW for the followup asked for in #27.

quietone’s picture

quietone commented
Status: Fixed » Needs work

Actually set to NW

znerol opened merge request !4751

znerol’s picture

znerol commented
Status: Needs work » Needs review

Followup in MR !4751.

znerol’s picture

znerol commented

Updated the CR.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs followup

Opened https://www.drupal.org/project/drupal/issues/3386766

Moving to RTBC as I don't know if MR 4751 should be closed.

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Let's do the remaining work in the follow-up that's open, moving this back to fixed.

znerol closed merge request !4751

quietone’s picture

quietone commented

Publish the change record

Status: Fixed » Closed (fixed)

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

znerol closed merge request !4483