Postpone instantiation of session config by using a proxy

[fmarangi]

Description

The session config is initialized too early, on initialisation of the HTTP response object. The init of this config object triggers the load of store-specific configuration, which then caches the resolved config path. This happens before the pathinfo is processed, which defines the current store in multi-storeview environment. By using a proxy the instantiation is postponed.

This problem was introduced in c18e36b.

Fixed Issues (if relevant)

1. Upgraded to Magento 2.2.4 from Magento 2.1.9 - Locale and Store Configuration issues 'Store View' Locale not being used on frontend, 'Default Config' Locale being used instead  #15205
2. 2.2.4: Wrong home page loaded in multi store setup  #15245

Manual testing scenarios

1. Magento 2.2.4
2. Multiple storeviews, e.g. for different languages (store en for en_US and de for de_DE)
3. Config web/url/use_store set to 1
4. Navigate to /de

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-cicd2]

All committers have signed the CLA.

[hostep]

Nice one @fmarangi !

Unfortunately applying your fix appears to break loading certain assets.
For example the response of the request to https://example.com/static/version1528396674/frontend/Magento/blank/en_US/requirejs-config.js contains:

Fatal error: Uncaught ArgumentCountError: Too few arguments to function Magento\Framework\App\Response\Http::__construct(), 5 passed in vendor/magento/module-media-storage/Model/File/Storage/Response.php on line 47 and exactly 6 expected in vendor/magento/framework/App/Response/Http.php on line 68

Call Stack
#   Time    Memory  Function    Location
1   0.0001  419920  {main}( )   .../static.php:0
2   0.0553  3669672 Magento\Framework\App\Bootstrap->createApplication( )   .../static.php:12
3   0.0553  3669672 Magento\Framework\App\ObjectManager->create( )  .../Bootstrap.php:232
4   0.0553  3669672 Magento\Framework\ObjectManager\Factory\Dynamic\Developer->create( )    .../ObjectManager.php:56
5   0.0557  3681008 Magento\Framework\ObjectManager\Factory\Dynamic\Developer->_resolveArguments( ) .../Developer.php:59
6   0.0558  3682288 Magento\Framework\ObjectManager\Factory\Dynamic\Developer->resolveArgumentsInRuntime( ) .../Developer.php:34
7   0.0560  3684264 Magento\Framework\ObjectManager\Factory\Dynamic\Developer->resolveArgument( )   .../AbstractFactory.php:230
8   0.0560  3684264 Magento\Framework\App\ObjectManager->create( )  .../AbstractFactory.php:146
9   0.0560  3684264 Magento\Framework\ObjectManager\Factory\Dynamic\Developer->create( )    .../ObjectManager.php:56
10  0.0662  4106832 Magento\Framework\ObjectManager\Factory\Dynamic\Developer->createObject( )  .../Developer.php:66
11  0.0662  4107216 Magento\MediaStorage\Model\File\Storage\Response\Interceptor->__construct( )    .../AbstractFactory.php:111
12  0.0662  4107216 Magento\MediaStorage\Model\File\Storage\Response\Interceptor->__construct( )    .../Interceptor.php:14
13  0.0662  4107216 Magento\MediaStorage\Model\File\Storage\Response\Interceptor->__construct( )    .../Response.php:47

Which makes sense, since the call to the parent constructor of the class Magento\MediaStorage\Model\File\Storage\Response doesn't have a sixth argument which it passes along.

Can you take a further look at this? Thanks!

[hostep]

I tested a bit more, looks like if I only apply your changes to the app/etc/di.xml file and don't apply the changes to the lib/internal/Magento/Framework/App/Response/Http.php file. It fixes everything.

I tested with the 3 scenario's:

• Upgraded to Magento 2.2.4 from Magento 2.1.9 - Locale and Store Configuration issues 'Store View' Locale not being used on frontend, 'Default Config' Locale being used instead  #15205: different locales configured works again
• 2.2.4: Wrong home page loaded in multi store setup  #15245: different homepages configured works again
• 2.2.4 EE Unable to change themes of stores #15873: different themes configured works again (can you also add this one to the 'fixed issues' list?)

Feel free to update your PR, unless you have a particular reason why you want to change that other class?

[fmarangi]

@hostep hi, thanks a lot for testing my changes! I updated my PR with a possible fix for the Media Storage, would you mind having a look at it?

[hostep]

Thanks, I won't have time to test this today, maybe tomorrow.

Just out of curiosity, what's the reason for changing the constructor of the Magento\Framework\App\Response\Http class? When I tested it yesterday, those changes weren't necessary to fix the problem. So I'm just curious why you think this is needed?

Thanks!

[fmarangi]

It is not really needed, but I find it quite it quite ugly to use the ObjectManager in a constructor when you can define everything via DI and have more control, I thought I'd tidy that up too. It's definitely going to be refactored at some stage, since it's a bit against M2 principles.

The reason why it was done like that is probably because of the error you found, instead of changing the subclass the OM was used directly as a workaround / quick win.

[hostep]

Ah ok, I suppose it was done for backwards compatible reasons, see this document: https://devdocs.magento.com/guides/v2.2/contributor-guide/backward-compatible-development/, the section 'Adding a constructor parameter'.

So I think it would be best to only keep the changes to the di.xml file, and remove all the other changes, otherwise I'm afraid this PR will be denied, but I'm not 100% sure. The Magento Community Maintainers can probably tell you more about this (@ihor-sviziev for example?)

[fmarangi]

Ok, I see, I'll reset my changes then, thanks for looking into that, learned something new.

[magento-engcom-team]

Hi @ihor-sviziev, thank you for the review.
ENGCOM-1924 has been created to process this Pull Request

[magento-engcom-team]

@fmarangi thank you for contributing. Please accept Community Contributors team invitation here to gain extended permissions for this repository.

[hostep]

@magento-engcom-team, @ihor-sviziev: I see you've added this to the 2.2.6 milestone, but I think it would be better to add this fix to 2.2.5 if it gets accepted.

Reason being: 2.2.5 is a security release, so you want as much people possible upgrading to that version. If big major bugs were introduced in 2.2.4 (like the one which is fixed by this PR), a fix for such major bugs should get included in 2.2.5, otherwise there will be people not wanting to upgrade to 2.2.5, which is not what you want since it's a security release.

I would also like to propose a new way for releasing new versions (using high version examples, just because):

• 2.2.105 (bugfixes, new features, performance improvements, new bugs)
• 2.2.106 (no new features, no performance improvements, only fixes for major bugs released in previous release, so this one is a more stable version for adding security fixes on top)
• 2.2.107 (security release, on top of a stable version without any major bugs)

Would something like this make more sense? Because bringing out security releases on top of a version with major bugs, is not the right way to go I think.

Just my 2 cents :)

[VladimirZaets]

Hi @fmarangi, thanks for collaboration. The PR introduces about 15% performance degradation. Unfortunately, we can't accept this PR. The result of performance tests in the attachment.

[hostep]

@VladimirZaets: please tell me Magento is working on a proper solution for fixing #15205, #15245 & #15873 then. This is really a big priority which needs to be fixed before 2.2.5 is released. Thanks!

[fmarangi]

@VladimirZaets I really cannot imagine how this fix (using a Proxy for the session config) would cause the figures you posted.

My guess is that the bug introduced in c18e36b had the side effect of causing a considerable performance improvement since store-specific and website-specific configurations were never loaded, thus saving some time and memory. Anyway it is a serious issue in my opinion, since language configs, payment configs, themes etc are not loaded correctly in a multiple storeview environment.

Could you please tell me how the performance tests are executed? Did repeated tests have the same results? I'd like to look into that and help provide a solution.

[ishakhsuvarov]

Hi @fmarangi

Performance tests are executed using jMeter and multiple variations of performance scenarios, you can see one in setup/performance-toolkit/benchmark.jmx. By default it is at least 60 loops on each scenario with disabled full page cache. You may learn more on the scenarios here: https://github.com/magento/magento2/tree/2.2/setup/performance-toolkit

By default, we use Magento installation with a generated "small profile" of data, which contains around 800 products and small amounts of other entities. You may find more info on generation here: https://devdocs.magento.com/guides/v2.2/config-guide/cli/config-cli-subcommands-perf-data.html

The Pull Request in question was measured by performance CICD job not 1 or 2 times, but multiple, all with the same result, to eliminate possible errors or deviations. From our experience, degradations of 10-15% are not caused by deviations but real issues.

@hostep I am now checking internal backlog to figure out status on the issue. I'll get back with the details, if I find any.

[ishakhsuvarov]

@fmarangi

My guess is that the bug introduced in c18e36b had the side effect of causing a considerable performance improvement since store-specific and website-specific configurations were never loaded, thus saving some time and memory.

I'll investigate performance numbers history for 2.2.x to find out if this is the case.

[ishakhsuvarov]

@fmarangi @hostep Reopening for additional investigation

[magento-engcom-team]

Hi @fmarangi. Thank you for your contribution.
Changes from your Pull Request will be available with the upcoming 2.2.6 release.

Please, consider to port this solution to 2.3 release line.
You may use Porting tool to port commits automatically.

[hostep]

@magento-engcom-team or @ishakhsuvarov : no chance you can include this fix in Magento 2.2.5?

The Release Notes for Magento 2.2.4 already include that it's not recommended to upgrade to that version because of the bugs this PR fixes:

Known issue
Customers have reported the following behavior after upgrading to Magento 2.2.4 in deployments that span multiple websites:

Magento multi-store installations do not use the store view-specific values from the store configuration settings if these settings have different values than the global default configuration settings. Instead, Magento uses the default configuration for all store views. GitHub-15205, GitHub-15245

We do not recommend upgrading to Magento 2.2.4 if you deploy across multiple websites. Note that this problem is not triggered if you have only a single website with multiple stores or store views.

So if 2.2.5 is a security release, you want people upgrading to it and not repeat that same message that it's not recommended to upgrade to that version I think, that would look very weird.

[sidolov]

Hi @hostep , fix will be available in 2.2.5 release. The comment above was autogenerated due to delivery to 2.2-develop branch, this changes were delivered to 2.2.5 release line in the separate internal pull request.

[hostep]

@sidolov: excellent, thanks for the update!

Also big thanks to @fmarangi for providing the fix! :)

[stkams]

A similar issue occurs in 2.2.5 when you switch "Add Store Code to Urls" to "Yes" after browsing first with the option set to "No". A cookie "store" was set which should be completely ignored when "Add Store Code to Urls" is Yes

[ihor-sviziev]

Hi @stkams,

Could you create new issue with steps to reproduce and maybe some additional details?