Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bugfix issue 17523 #17524

Closed

Conversation

maximbaibakov
Copy link

Description

Increasing number of cookies to 180 as supported by popular browsers.

Fixed Issues (if relevant)

  1. Unable to send the cookie. Maximum number of cookies would be exceeded #17523

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
Copy link
Contributor

Hi @maximbaibakov. 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 {$VERSION} instance - deploy vanilla Magento instance

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

@VladimirZaets
Copy link
Contributor

Hi @maximbaibakov. Thanks for the collaboration. As I know, IE currently supports only 50 cookies. Currently, according to [https://devdocs.magento.com/guides/v2.2/install-gde/system-requirements_browsers.html](browser support) document, Magento supported IE browsers.

@orlangur
Copy link
Contributor

This does not solve any issue in fact as 180 cookies limit may be reached just like 50 until #17195 is fixed properly.

@orlangur orlangur closed this Aug 11, 2018
@maximbaibakov
Copy link
Author

Hi @orlangur and @VladimirZaets

Thanks for taking time to review this PR.

I do agree on some of your statements and disagree on the others.

In regards to "IE currently supports only 50 cookies"
It does not mean that Magento should limit functionality for other browsers. Sometimes certain extension or tracking systems aka Facebook Pixel could set cookies differently depends on a browser, country, OS and etc.

Also, the rfc6265 is setting limits to a browser, it is setting minimum requirements to a browser.
Magento should not fail with an exception if the cookie size exceeded, it should still keep working.

This does not solve any issue in fact
@orlangur tend to disagree, it does solve issue for users who are using modern browsers like Edge, Chrome, Firefox and etc and the website still operating well in case of cookies limit increased

Why more then 50 cookies could be presented on the website?

  • The website could have number of tracking systems aka Facebook, GA, GTM, Hotjar and etc.
  • While each of the tracking systems could have 2-5 cookies as a group they could go above 50, especially, if your website has number of subdomains and cookies could be defined on top domain
  • For example, Magento website together with Magento Marketplace website sometimes could have almost 50 cookies.

I think, increasing cookies should not harm to Magento as a platform and / or cookies limit should not get website down.

What do you think?

Regards,
Maxim

@maximbaibakov maximbaibakov reopened this Aug 12, 2018
@VladimirZaets
Copy link
Contributor

Hi @maximbaibakov.
In regards to "IE currently supports only 50 cookies"
I agree that Magento shouldn't limit functionality for other browsers, but we can't simply just raise cookies to the limit of the new browsers, we should have some fallback to provide the correct system functionality

@orlangur
Copy link
Contributor

it does solve issue for users who are using modern browsers like Edge, Chrome, Firefox and etc and the website still operating well in case of cookies limit increased

No, it does not. Please check #17195 and just repeat steps more times then to reach 50 limit.

The website could have number of tracking systems aka Facebook, GA, GTM, Hotjar and etc.

You shouldn't have all of them enabled for one site. Having such amount of cookies is not good from any perspective, what is amount of cookies with vanilla Magento 2 installation?

Some browsers supporting 150+ cookies should not be a reason to increase limit ignoring RFC: http://browsercookielimits.squawky.net/
It's better to fix buggy implementation / improve by removing unnecessary cookies.

@sidolov
Copy link
Contributor

sidolov commented Aug 27, 2018

Hi @maximbaibakov , I am closing this PR now due to inactivity.
Please reopen and update if you wish to continue.
Thank you for the collaboration!

@sidolov sidolov closed this Aug 27, 2018
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants