Correctly implement SessionHandlerInterface

znerol → credited jungle → .

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

The only question - should we keep ReturnTypeWillChange attribute?

The only question - should we keep ReturnTypeWillChange attribute?

Good question. Let's try without.

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

Hmm, not convinced the scoping of this issue is correct - this is fixing three 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.

Bulk conversion to constructor property promotion is being discussed in 📌 Use PHP 8 constructor property promotion for existing code Needs work

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

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.

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

MR seemed to have CC failure.

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

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

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

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

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

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

Left a comment on MR.

Believe this is ready.

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!

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.

Actually set to NW

Followup in !4751.

Updated the CR.

Opened https://www.drupal.org/project/drupal/issues/3386766 📌 Add return types to SessionHandler Active

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

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

znerol → closed merge request !4751

Publish the change record

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

znerol → closed merge request !4483