From fd63ef1fef268bda19e99eb87c605beba543c422 Mon Sep 17 00:00:00 2001 From: Alex Horkun Date: Thu, 18 Oct 2018 19:02:36 +0300 Subject: [PATCH 1/3] Sessions to be used only in controllers and blocks It is unsafe to use sessions anywhere but controllers and blocks due to multiple web APIs in Magento --- design-documents/sessions-only-for-html.md | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) create mode 100644 design-documents/sessions-only-for-html.md diff --git a/design-documents/sessions-only-for-html.md b/design-documents/sessions-only-for-html.md new file mode 100644 index 000000000..e5a5923d6 --- /dev/null +++ b/design-documents/sessions-only-for-html.md @@ -0,0 +1,20 @@ +### Magento\Framework\Session\SessionManagerInterface should be used only in controllers and blocks +#### Why? +Web APIs (REST (by definition), SOAP, GraphQL) do not employ sessions, +relying on sessions in classes that are not directly linked to preparing HTML +may cause sessions being used during Web API requests unintentionally causing +unexpected behaviour during and negative performance impact. + +#### Current situation +Sessions are being used in model classes. + +#### What has to be done +Add PHPMD rule which will be triggered when a class has a constructor parameter of type +SessionManagerInterface (and any of the implementations) and is not an instance of +ActionInterface or AbstractBlock. +The rule's message should explain that UserContextInterface should be used to get current +user instead of sessions. + +Removing all existing session usages from non-controller and non-block classes would +prove a difficult process so we just have to warn developers not to use them +in the future with the PHPMD rule. From bf84e7f1eef275bcc4e0d1f816a46610c7c752ba Mon Sep 17 00:00:00 2001 From: Alex Horkun Date: Fri, 19 Oct 2018 10:45:09 +0300 Subject: [PATCH 2/3] Updated the doc --- design-documents/sessions-only-for-html.md | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/design-documents/sessions-only-for-html.md b/design-documents/sessions-only-for-html.md index e5a5923d6..8050c3125 100644 --- a/design-documents/sessions-only-for-html.md +++ b/design-documents/sessions-only-for-html.md @@ -1,20 +1,20 @@ -### Magento\Framework\Session\SessionManagerInterface should be used only in controllers and blocks +### Magento\Framework\Session\SessionManagerInterface and Magento\Framework\Stdlib\Cookie\CookieReaderInterface should be used only in HTML related presentation level #### Why? -Web APIs (REST (by definition), SOAP, GraphQL) do not employ sessions, -relying on sessions in classes that are not directly linked to preparing HTML +Web APIs (REST (by definition), SOAP, GraphQL) do not employ sessions and cookies, +relying on sessions and cookies in classes that are not directly linked to preparing HTML may cause sessions being used during Web API requests unintentionally causing unexpected behaviour during and negative performance impact. #### Current situation -Sessions are being used in model classes. +Sessions and cookies are being used in model classes. #### What has to be done Add PHPMD rule which will be triggered when a class has a constructor parameter of type -SessionManagerInterface (and any of the implementations) and is not an instance of -ActionInterface or AbstractBlock. +SessionManagerInterface/CookieReaderInterface (and any of the implementations) and are not a part of HTML presentation level +(instances of ActionInterface, AbstractBlock, DataProviderInterface, Document). The rule's message should explain that UserContextInterface should be used to get current -user instead of sessions. +user instead of sessions and cookie usage should be avoided. -Removing all existing session usages from non-controller and non-block classes would -prove a difficult process so we just have to warn developers not to use them +Removing all existing session and cookie usages from classes that are not a part of HTML presentation level would +prove to be a difficult process so we just have to warn developers not to use them in the future with the PHPMD rule. From fa88d70f94600be062a6869032a573c988ad8523 Mon Sep 17 00:00:00 2001 From: Alex Horkun Date: Wed, 24 Oct 2018 20:48:08 +0300 Subject: [PATCH 3/3] Allowing controller and block plugins --- design-documents/sessions-only-for-html.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/design-documents/sessions-only-for-html.md b/design-documents/sessions-only-for-html.md index 8050c3125..b7f47b492 100644 --- a/design-documents/sessions-only-for-html.md +++ b/design-documents/sessions-only-for-html.md @@ -11,7 +11,7 @@ Sessions and cookies are being used in model classes. #### What has to be done Add PHPMD rule which will be triggered when a class has a constructor parameter of type SessionManagerInterface/CookieReaderInterface (and any of the implementations) and are not a part of HTML presentation level -(instances of ActionInterface, AbstractBlock, DataProviderInterface, Document). +(instances of ActionInterface, AbstractBlock, DataProviderInterface, Document, controller and block plugins). The rule's message should explain that UserContextInterface should be used to get current user instead of sessions and cookie usage should be avoided.