MAX_NUM_COOKIES doesn't follow the open-closed principle

[brideo]

Preconditions

1. Magento 2.x

Steps to reproduce

1. open `Magento\Framework\Stdlib\Cookie\PhpCookieManager::checkAbilityToSendCookie
2. See PhpCookieManager::MAX_NUM_COOKIES

Expected result

1. Should bind using static::MAX_NUM_COOKIES

Actual result

2. I cannot change the outcome of this class without rewriting 3 methods

[orlangur]

Open-closed principle does not dictate static:: should only be used to access constants by the way.

[orlangur]

@brideo,

 /**#@+
     * Constants for Cookie manager.
     * RFC 2109 - Page 15
     * http://www.ietf.org/rfc/rfc6265.txt
     */
    const MAX_NUM_COOKIES = 50;
    const MAX_COOKIE_SIZE = 4096;
    const EXPIRE_NOW_TIME = 1;
    const EXPIRE_AT_END_OF_SESSION_TIME = 0;
    /**#@-*/

This value is not supposed to be changed as it is based on RFC.

Could you please describe what you try to achieve by changing this value? I remember it was a problem in 2.1.x as reaching limit produced a fatal error but now it is just logged

if ($numCookies > PhpCookieManager::MAX_NUM_COOKIES) {
            $this->logger->warning(
                new Phrase('Unable to send the cookie. Maximum number of cookies would be exceeded.'),
                array_merge($_COOKIE, ['user-agent' => $this->httpHeader->getHttpUserAgent()])
            );
        }

[brideo]

The open-close rule does not dictate anything. It’s a principle in which you don’t have to edit the original code to change the outcome. Binding to the constant using the static binding would achieve this.

The static binding I’m talking about was referencing a constant.

What I’m trying to achieve by changing the MAX_NUM_COOKIES constant is changing the max number of cookies.

Logging error is cool, but I’m trying to mitigate it all together.

Also, the RFC standards are 50, however, that's fairly conservative and most browsers tend to support a much higher limit.

http://browsercookielimits.squawky.net/

[orlangur]

Binding to the constant using the static binding would achieve this.

I meant that following this approach means to forbid self:: for class constants at all.

Thanks for a really illustrative link! Looks like as Magento supports IE11 50 is still a relevant value for core.

Logging error is cool, but I’m trying to mitigate it all together.

Great, looks like you really understand what you're doing 👍 If some Magento merchants don't care about IE and somehow their instance creates quite a bunch of cookies, value should be good to raise.

Class inheritance is not a recommended way and bla-bla-bla but a better way than copy-paste and I don't think this parameter should be injected via constructor, so, static:: replacement seems to be a good compromise.

Could you please create a pull request containing suggested change? For the future, when you desire a code change it can be posted directly as a PR, even if will be rejected or seriously reworked later it will be faster than discuss potential code change in an issue.

[brideo]

I see, I personally always try to use static:: rather than self::, but I can see why you'd want consistency.

Yeah, I did think about suggesting di or a config path, however, I don't know if you want it to be that easily configurable for reasons you've mentioned.

I have created a pull request:

#14128

I'm not sure whether I'm doing the correct thing in the test by referencing the constant from the object, I guess you could do a get_class or use class reflection but this seemed the simplest.

Let me know if you want to update anything.

[magento-engcom-team]

@brideo, thank you for your report.
We've acknowledged the issue and added to our backlog.

[magento-engcom-team]

Hi @brideo. Thank you for your report.
The issue has been fixed in #14128 by @brideo in 2.2-develop branch
Related commit(s):

• 9b34bf96772808a447ec6b84a0dae433c5f02ea7

The fix will be available with the upcoming 2.2.5 release.

[magento-engcom-team]

Hi @brideo. Thank you for your report.
The issue has been fixed in #14366 by @rostyslav-hymon in 2.3-develop branch
Related commit(s):

• 8cfc85cf297f0c4763d03c7e599e648eeaf3b54a

The fix will be available with the upcoming 2.3.0 release.

[siliconalchemy]

@brideo, could you possibly give an example of how to override MAX_NUM_COOKIES? I've tried overriding Magento\Framework\Stdlib\Cookie\PhpCookieManager::checkAbilityToSendCookie with Preference but no luck.

[brideo]

Hey @siliconalchemy,

I assume the class is still the same so you will need to add a preference to your class then override the max cookie constant.

<preference for="A" type="B" />

<?php

Class A
{
    const EXAMPLE = 'A';

    public function output()
    {
        echo static::EXAMPLE;
    }
}

Class B extends A {
    const EXAMPLE = 'B';
}

(new B())->output();

Output:

➜  example ✗ php example.php
B%

[siliconalchemy]

Hey @brideo, thanks for response. Is it not possible to override the core class to reset MAX_NUM_COOKIES?

<?php
namespace SiliconAlchemy\CustomCookies\Stdlib\Cookie;

class PhpCookieManager extends \Magento\Framework\Stdlib\Cookie\PhpCookieManager
{
    const MAX_NUM_COOKIES=2;
}

This has no effect as a preference (setting to 2 to test).