Problem/Motivation

Follow up from #3377256-27: Correctly implement SessionHandlerInterface from @catch

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...).

As per @quietone in #3377256-28: Correctly implement SessionHandlerInterface

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

Steps to reproduce

Proposed resolution

Remove #[\ReturnTypeWillChange] attribute and add return types to session handler methods.

Remaining tasks

User interface changes

API changes

The SessionHandler class uses the correct PHP \SessionHandlerInterface method signatures for return types.

Note: This will break subclasses of SessionHandler if they do not specify the correct return types. However, since non-base classes are not considered public API, this isn't considered a BC break. Also a code search revealed no subclass of SessionHandler in contrib.

Data model changes

Release notes snippet

Issue fork drupal-3386766

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

smustgrave created an issue. See original summary.

catch’s picture

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

znerol’s picture

Status: Active » Needs review
znerol’s picture

Also adapted the CR from the original issue.

smustgrave’s picture

Status: Needs review » Needs work

Seemed to cause nightwatch failures.

smustgrave’s picture

Status: Needs work » Needs review

Actually seeing this on other tickets. Wonder if head broke.

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

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

Yup was broken HEAD.

Thanks @andypost!

xjm’s picture

Hmm, this seems too disruptive for a minor to me, at least without additional process or tooling. We'd want to provide best-effort BC for typehint changes, or at least a tool to automatically fix all typehint changes, and I think if nothing else it should target 11.0.x with a single rector rule for all added typehints or something (if we can't come up with a BC way to document the typehint additions in a D10 minor, which I think we also could do somehow-or-other). We've never broken BC even for an internal API like this before.

It'd also need at least a CR if not a release note -- possibly one single CR or something that documents all the typehint additions in a given major-or-minor, and a single simple release note about typehint additions linking that CR. Or something.

Also, there are whole sprawling policy issues about how to go about adding typehints to existing code; this issue should have one of them as a parent, or at least related issues.

Tagging for RM review while I check with @catch about this.

znerol’s picture

Issue summary: View changes

Updated the issue summary with information from the original issue #3377256: Correctly implement SessionHandlerInterface and also linked the CR from over there.

znerol’s picture

Issue summary: View changes
znerol’s picture

Issue summary: View changes
znerol’s picture

Issue tags: -Needs change record
xjm’s picture

Issue tags: +Needs change record

Thanks @znerol. The stuff we need a CR for is not the original API, but the typehint specifically. It is not related to the API itself; I am talking about a generic CR for added return typehints across core. So retagging.

znerol’s picture

Adding this to the meta issue #3050720

catch’s picture

We did #3377356: Make DebugClassLoader ignored deprecations more accurate recently, this means that during tests, Symfony's debug classloader will issue deprecations for any parent method that has a documented type hint that it might add a return type hint in the future. So contrib will get notified when it's extending core classes that have documented type hints but haven't added them yet. I would prefer it if this was configurable via an attribute on methods or similar, but it's not, they just issue the deprecation for any random method on the basis that child classes can add type hints and it won't do any harm.

However, with this issue specifically, the parent interface, which is PHP's own interface, already has the return typehints, and you already get deprecation notices from PHP itself, if you implement those methods without a [#ReturnTypeWillChange] attribute (which is what's being removed here). So PHP is already doing the deprecation for us in this case.

https://www.php.net/manual/en/class.sessionhandlerinterface.php

The issue if we don't do this in a minor, is if PHP releases PHP 9, and they remove the [#ReturnTypeWillChange] support to skip those deprecation notices, we'd never be able to make Drupal 10 fully compatible with it. That makes this specific issue different to other core classes where in general I agree we'd want to add the return type hints to existing code in a major release.

xjm’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs change record updates

Thanks @catch; that's compelling. I think maybe we can do the following:

  1. Create a second CR about typehint changes in 10.2, and include this and any other issues.
  2. Add a release note about typehint changes generally linking that first CR.
  3. Update the CR to make it a little more clear about the typehint change and the consequences/mitigations.

Then we can use a similar pattern for the rest that we implement in D11.

znerol’s picture

Status: Needs work » Needs review
Issue tags: -Needs change record, -Needs change record updates

Issue now targets 10.3, added a dedicated change record now.

andypost’s picture

Status: Needs review » Reviewed & tested by the community

Thank you, back to RTC

  • catch committed e0e5634f on 11.x
    Issue #3386766 by znerol, andypost, smustgrave, xjm, catch: Add return...
catch’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: -Needs release manager review

Committed/pushed to 11.x (which will become 10.3.x too), thanks!

andypost’s picture

Please close the MR

Status: Fixed » Closed (fixed)

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