Added getScopeCacheKey in order to define the right cache key based on store ID

[matiashidalgo]

Description (*)

This PR adds an small improvement to the instance cache into the categories repository cause it allows to receive store id or store code for attributes data retrieval but at the same time this store id / code is used as cache key which make it a double unnecessary category load when you request the category by same given store, first providing the store id and then providing store code.

Manual testing scenarios (*)

1. Into your own class load the category repository via di.
2. Use the category repository to get a given category providing an specific store ID. Check the given category got stored into the cached instances array of the repository under the store ID.
3. Use the category repository to get a given category (same category as previous point) providing a specific store Code (same store as previous point). Error should be thrown with this fix, otherwise it was loading again the category filtering by store code which is incorrect based on PHP Docs into the interface.

Questions or comments

Have in mind this issue is not reproducible from REST API given it enforces to use the store ID based on the PHP Docs from the interface.

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 are green)

Resolved issues:

1. resolves [Issue] Added getScopeCacheKey in order to define the right cache key based on store ID #28142: Added getScopeCacheKey in order to define the right cache key based on store ID

[m2-assistant]

Hi @matiashidalgo. 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 give me test instance - deploy test instance based on PR changes
• @magento give me 2.4-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Guide documentation.

[ihor-sviziev]

Hi @matiashidalgo,

According to phpdoc block https://github.com/magento/magento2/blob/8c36ea51fab3be7d3e2135c2e4cc386bd453673c/app/code/Magento/Catalog/Api/CategoryRepositoryInterface.php#L29
this class should retrieve only int, so we don't need to support other possible cases. So looks like we don't really need to fix it, as it's not an issue at all

Do you have this issue in magento core or in custom module?

[matiashidalgo]

Hi @matiashidalgo,

According to phpdoc block

https://github.com/magento/magento2/blob/8c36ea51fab3be7d3e2135c2e4cc386bd453673c/app/code/Magento/Catalog/Api/CategoryRepositoryInterface.php#L29

this class should retrieve only int, so we don't need to support other possible cases. So looks like we don't really need to fix it, as it's not an issue at all
Do you have this issue in magento core or in custom module?

As I said into the comments, the interface enforce it for API requests, but it doesn't work that way for instantiation from code, I saw the tests and I'll make a little change to the implementation.

[ihor-sviziev]

@matiashidalgo
In this case maybe it’s better to throw an exception

[lbajsarowicz]

@ihor-sviziev I was the one who reported that issue internally for contribution.
During work with #27035 I faced the situation where as a second argument of the function internally was used Store Code instead of Store ID.

Change to throw Exception (or type hint for argument to ?int would be backward incompatible. But it's also an option, actually.

--- EDIT
@matiashidalgo I spoke to @ihor-sviziev and actually it's good approach to throw an exception on invalid type. Then we'll see which API functional tests fail and fix these references with Store Code.

[ihor-sviziev]

Hi @matiashidalgo,
I've added few comments, please adjust your solution.

There also some integration test failed because it used string store code instead of numeric id. Please fix it.

[ihor-sviziev]

Hi @matiashidalgo,
Your changes looks good to me. Could you cover your changes with tests, for instance unit tests?

[ihor-sviziev]

@magento run Functional Tests B2B, Functional Tests CE, Functional Tests EE

[ihor-sviziev]

Failing functional tests are not related to changes in this pr

[ihor-sviziev]

@magento run all tests

[matiashidalgo]

@magento run all tests

[ihor-sviziev]

@magento run Unit Tests, Functional Tests CE

[ihor-sviziev]

@magento run Functional Tests B2B, Functional Tests CE

[ihor-sviziev]

@magento run Functional Tests CE

[engcom-Dash]

Moving this PR to on hold since we are not picking up any PRs against the 2.5-develop branch right now. Once we pick it up, we will move it to the appropriate status.