Removed direct use of SessionManager class, used SessionManagerInterface instead

[jaimin-ktpl]

Description (*)

Removed direct use of SessionManager class, used SessionManagerInterface instead.

Fixed Issues (if relevant)

1. Why is SessionManager used instead of its Interface? #19274: Why is SessionManager used instead of its Interface?

Manual testing scenarios (*)

1. Replaced SessionManager with SessionManagerInterface class

Contribution checklist (*)

• Pull request has a meaningful description of its purpose
• All commits are accompanied by meaningful commit messages
• All new or changed code is covered with unit/integration tests (if applicable)
• All automated tests passed successfully (all builds on Travis CI are green)

[magento-engcom-team]

Hi @jaimin-ktpl. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

• @magento-engcom-team give me test instance - deploy test instance based on PR changes
• @magento-engcom-team give me 2.3-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Assistant documentation

[magento-engcom-team]

Hi @sivaschenko, thank you for the review.
ENGCOM-3543 has been created to process this Pull Request

[orlangur]

@sivaschenko just wondering, is such change backward-compatible in case we have custom code extending core classes?

[sivaschenko]

@orlangur yes, it is. Extended classes will simply have more strict control over constructor parameters requiring SessionManager instead of SessionManagerInterface

[sivaschenko]

@orlangur Actually I checked the case and looks like you are correct the change will result in PHP Warning if these classes were extended. I will clarify our approach in this case and will get back to this PR

[sivaschenko]

Hi @jaimin-ktpl to keep backward compatibility it's required to keep the existing SessionManager dependency. It's still possible to introduce a new optional constructor parameter of SessionManagerInterface type.

[jaimin-ktpl]

Hi @jaimin-ktpl to keep backward compatibility it's required to keep the existing SessionManager dependency. It's still possible to introduce a new optional constructor parameter of SessionManagerInterface type.

Hello, @sivaschenko ,
Thank you for your inputs on the issue. Are you stating that I should inject a new class and keep the existing one? Shall I also update the code of session manager to use interface object or keep the same session manager object?

[sivaschenko]

Hi @jaimin-ktpl ,

• these changes are not required for app/code/Magento/Customer/CustomerData/Plugin/SessionChecker.php as it is a plugin
• it's ok to assing iterace object to \Magento\Paypal\Controller\Transparent\RequestSecureToken::$sessionManager as this property is private
• it's ok to assign an interface object to \Magento\Framework\View\Context::$session as it has the interface type annotation

[jaimin-ktpl]

Hi @jaimin-ktpl ,

• these changes are not required for app/code/Magento/Customer/CustomerData/Plugin/SessionChecker.php as it is a plugin
• it's ok to assing iterace object to \Magento\Paypal\Controller\Transparent\RequestSecureToken::$sessionManager as this property is private
• it's ok to assign an interface object to \Magento\Framework\View\Context::$session as it has the interface type annotation

Thanks @sivaschenko , for your suggestions. I will update the code as per and will commit the changes.

[sivaschenko]

Hi @jaimin-ktpl would you like to continue progress on this contribution?

[jaimin-ktpl]

Hi @jaimin-ktpl would you like to continue progress on this contribution?

Hello @sivaschenko , Apologies for the delay. I have updated the code as per your suggestions. Thanks.

[sivaschenko]

Hi @jaimin-ktpl , thanks for the update. Please pay attention that according to comment #19359 (comment) the constructur signature cannot be changed for lib/internal/Magento/Framework/View/Context.php (that is the only changed file now in the pull request)

[jaimin-ktpl]

Hello @sivaschenko ,
I think I have misunderstood your comments and got a little confused.
You stated that

these changes are not required for app/code/Magento/Customer/CustomerData/Plugin/SessionChecker.php as it is a plugin

So I removed my code for that plugin.
For second comment,

it's ok to assing iterace object to \Magento\Paypal\Controller\Transparent\RequestSecureToken::$sessionManager as this property is private

I misunderstood it and removed the changes from that file where it should have applied.

For third comment,

it's ok to assign an interface object to \Magento\Framework\View\Context::$session as it has the interface type annotation

You stated that its okay to assign interface object, so I kept my changes for that file but with your latest comment you stated that

the constructur signature cannot be changed for lib/internal/Magento/Framework/View/Context.php

So I am little confused if I should keep the Interface object or not for that file. I am really sorry for the inconvenience but can you please confirm if I should keep Interface object for Context.php or remove it.
Many thanks.

[sivaschenko]

Sorry for confusion @jaimin-ktpl . By these changes are not required I meant the changes that I requested. Backward compatibility policy does not apply to plugins so you can change the method signatures there.

As for other classes, you are required to add an additional optional dependency to the constructor, but you can assign this new dependency to $session property (replacing the previous assignment)

[jaimin-ktpl]

Sorry for confusion @jaimin-ktpl . By these changes are not required I meant the changes that I requested. Backward compatibility policy does not apply to plugins so you can change the method signatures there.

As for other classes, you are required to add an additional optional dependency to the constructor, but you can assign this new dependency to $session property (replacing the previous assignment)

Hello @sivaschenko, I have added an optional argument with SessionManagerInterface $sessionManagerInterface = null and updated assignment like $this->sessionManager = $sessionManagerInterface ?: $sessionManager;, Please let me know if its the correct approach so I can commit the code. Thanks.

[sivaschenko]

Hi @jaimin-ktpl the approach looks correct to me, it will be easier to review the committed code.

[magento-engcom-team]

Hi @sivaschenko, thank you for the review.
ENGCOM-3543 has been created to process this Pull Request

[ghost]

Hi @jaimin-ktpl, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.