Fixed incompatible interface deprecations in DB Session Adapter

[elidrissidev]

Description (*)

Small fix to make the methods in DB session adapter compatible with SessionHandlerInterface.

Fixed Issues (if relevant)

1. Fixes Incompatible interface deprecations in Mage_Core_Model_Resource_Session #3496

Manual testing scenarios (*)

1. Switch <session_save> in local.xml to db.
2. Clean the cache and check system.log.

Contribution checklist (*)

• Pull request has a meaningful description of its purpose
• All commits are accompanied by meaningful commit messages
• All automated tests passed successfully (all builds are green)
• Add yourself to contributors list

[elidrissidev]

I tried adding the type hints following SessionHandlerInterface signature and only adding ReturnTypeWillChange to the methods with union types which aren't supported pre-PHP 8, but PHPStan keeps complaining 🤷‍♂️

[colinmollenhour]

I didn't see your PR and made an attempt to ignore these errors here: #3498 but it seems yours actually works. 😄

What do you think about changing:

class Mage_Core_Model_Resource_Session implements Zend_Session_SaveHandler_Interface

to

class Mage_Core_Model_Resource_Session implements SessionHandlerInterface

?

[elidrissidev]

@colinmollenhour I've switched it. Zend_Session_SaveHandler_Interface wasn't referenced anywhere anyways.

[fballiano]

if we change the interface of a public class, shouldn't we have this PR in the next branch?

[elidrissidev]

The interface is unused in the core and the only reference I found was commented out 15 years ago.

[fballiano]

😂 ok then, it seems enough

some of our code is really old

[fballiano]

on my local machine phpstan runs without errors also without the ReturnTypeWillChange now that we're using the SessionInterface, should we still keep the ReturnTypeWillChange?

[elidrissidev]

The issue this PR was meant to be fixing is PHP deprecation errors related to incompatible types with the interface. PHPStan never complained about them in main, but I think it should fix it in next.

[fballiano]

sure, that's why I'm running phpstan on my local machine with php8, which is the difference that triggers the error in next ;-)

(actually i've to admin I ran it with php8.2 instead of 8.1)

[elidrissidev]

In next proper types could be added to this class. Here it's either you remove ReturnTypeWillChange which causes PHP deprecation messages, or you try to introduce types but PHPStan's expectations do not conform with what's in PHP documentation, it seems to expect the methods to have mixed type which is not available in PHP 7.x.

[fballiano]

true, this version works correctly but it would have to be committed only on the next branch

[colinmollenhour]

Does this patch also fix the next branch? My only goal was to get the PHPStan action passing again - if this does that then I am for it. :)
What is the difference between main and next that causes next to fail in the first place?

[fballiano]

It should only be that next workflows run on php8.1

[elidrissidev]

Does this patch also fix the next branch?

It should fix it.

[colinmollenhour]

Looks good to me. The change of which interface class is implemented should be benign.

[fballiano]

I would have preferred the proper fix on the next branch :-\

[elidrissidev]

@fballiano Feel free to open one for next to introducing proper types :)

[fballiano]

done, but I think this one should be merged too, right?

[elidrissidev]

Yeah. The PR for next should wait until this one is merged and forward-ported so you can rebase it and remove the ReturnTypeWillChange since it won't be needed there.

[fballiano]

then I guess it's time ;-)

[fballiano]

merged, forward ported and updated #3499 ;-)