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
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:
- 3386766-add-return-types
changes, plain diff MR !4785
Comments
Comment #2
catchhttps://git.drupalcode.org/project/drupal/-/merge_requests/4751 should be transferred over here.
Comment #5
znerol commentedComment #6
znerol commentedAlso adapted the CR from the original issue.
Comment #7
smustgrave commentedSeemed to cause nightwatch failures.
Comment #8
smustgrave commentedActually seeing this on other tickets. Wonder if head broke.
Comment #10
smustgrave commentedYup was broken HEAD.
Thanks @andypost!
Comment #11
xjmHmm, 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.
Comment #12
znerol commentedUpdated the issue summary with information from the original issue #3377256: Correctly implement SessionHandlerInterface and also linked the CR from over there.
Comment #13
znerol commentedComment #14
znerol commentedComment #15
znerol commentedComment #16
xjmThanks @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.
Comment #17
znerol commentedAdding this to the meta issue #3050720
Comment #18
catchWe 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.
Comment #19
xjmThanks @catch; that's compelling. I think maybe we can do the following:
Then we can use a similar pattern for the rest that we implement in D11.
Comment #20
znerol commentedIssue now targets 10.3, added a dedicated change record now.
Comment #21
andypostThank you, back to RTC
Comment #23
catchCommitted/pushed to 11.x (which will become 10.3.x too), thanks!
Comment #24
andypostPlease close the MR