pickware / shopware

Shopware Repository
http://www.shopware.com
Other
0 stars 0 forks source link

Stores for config fields are loaded over and over again #33

Closed fixpunkt closed 7 years ago

fixpunkt commented 7 years ago

The settings of most of our plugins each contain multiple selects which are backed by stores, usually Shopware.apps.Base.store.Payment, Shopware.apps.Base.store.PaymentStatus and Shopware.apps.Base.store.Dispatch.

The situation right now is that for each of these selects, the backing store is loaded twice per subshop. This means that for customers who have more than a few subshops (8, for instance), opening the Pickware Mobile settings causes more than 200 HTTP requests to be fired to the server in order to load the same content of three stores. This basically amounts to a DoS on the server and can, depending on server configuration, cause HTTP 503 errors for some of those requests while loading the plugin settings.

Before https://github.com/shopware/shopware/pull/525, a single shared instance of each store was used for config fields. This was problematic because when a third-party plugin filtered that store, the filter would also apply to the config field in our plugin settings. Unfortunately, the caching strategy @svenmuennich originally implemented was removed when merging the pull request (cf. https://github.com/shopware/shopware/commit/3fc607dd19ba4c7092a4b42f2aa632c153a7f67d), instead changing the semantics to always re-instantiate the store.

In addition to this, each store is actually loaded twice, because both https://github.com/VIISON/shopware/blob/39e84ad1a76361c01f23ce24f5ffafec3bf7a1c3/themes/Backend/ExtJs/backend/base/component/element/select.js#L123 and https://github.com/VIISON/shopware/blob/12aba17d8885af3fbe2aa94591faae574a156afe/themes/Backend/ExtJs/backend/base/component/element/select.js#L85 (which is always called for ExtJS Fields) cause the store to be loaded.

We need to create a pull request that enables store re-use for config fields.

However, Sven has since added the ability to filter config element stores in https://github.com/shopware/shopware/pull/699. This means that his original store re-use implementation would no longer be correct since it would cause the issue of config fields getting filtered by other plugins to reappear.

Instead, we've discussed that we should load each store at most once in https://github.com/VIISON/shopware/blob/12aba17d8885af3fbe2aa94591faae574a156afe/themes/Backend/ExtJs/backend/base/component/element/select.js#L119 and remember that store instance. However, instead of returning this master copy of the store to the caller of getStoreById() and thus risking it getting corrupted by filtering, we should only ever return pre-loaded clones of this store. This should both prevent unnecessary requests to the backend from being made and ensure that filtering stores in one place does not cause that filter to be applied to all other places where this store is used.

Related PRs for context: https://github.com/shopware/shopware/pull/525, https://github.com/shopware/shopware/pull/600, https://github.com/shopware/shopware/pull/699.

fixpunkt commented 7 years ago

https://github.com/VIISON/shopware/pull/36 fixes this