-
Notifications
You must be signed in to change notification settings - Fork 9.4k
magento/magento2#27952: missing store_name in GraphQL resolver - adde… #28855
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
magento/magento2#27952: missing store_name in GraphQL resolver - adde… #28855
Conversation
Hi @sasha19957099. Thank you for your contribution
❗ Automated tests can be triggered manually with an appropriate comment:
You can find more information about the builds here ℹ️ Please run only needed test builds instead of all when developing. Please run all test builds before sending your PR for review. For more details, please, review the Magento Contributor Guide documentation. |
@magento run all tests |
$storeConfigData = array_merge( | ||
$this->getBaseConfigData($store), | ||
$this->getExtendedConfigData((int)$store->getId()) | ||
return array_merge( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should swap these, it will change behavior in a way we may not want. If store_name is getting overwritten, you should remove this in di.xml
<arguments>
<argument name="extendedConfigData" xsi:type="array">
<item name="store_name" xsi:type="string">store/information/name</item>
</argument>
</arguments>
</type>```
@@ -88,9 +87,9 @@ private function getBaseConfigData(StoreInterface $store) : array | |||
'secure_base_url' => $storeConfig->getSecureBaseUrl(), | |||
'secure_base_link_url' => $storeConfig->getSecureBaseLinkUrl(), | |||
'secure_base_static_url' => $storeConfig->getSecureBaseStaticUrl(), | |||
'secure_base_media_url' => $storeConfig->getSecureBaseMediaUrl() | |||
'secure_base_media_url' => $storeConfig->getSecureBaseMediaUrl(), | |||
'store_name' => $storeConfig->getStoreName() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While you're changes look correct, I just wonder if we could do it in a less disruptive way. The current approach introduces a backwards incompatible change because we're adding a new public method to an @api interface.
Could we just use $store->getName() here and avoid the changes outside of GraphQl area?
composer.lock
Outdated
@@ -4,7 +4,7 @@ | |||
"Read more about it at https://getcomposer.org/doc/01-basic-usage.md#installing-dependencies", | |||
"This file is @generated automatically" | |||
], | |||
"content-hash": "e86af25d9a4a1942c437cca58f9f1efb", | |||
"content-hash": "f3674961f96b48fdd025a6c94610c8eb", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't need to update composer.lock for this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, but without it, local static test fails
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, so actually this change matches the hash in mainline, so I don't know why it is showing up as a change here. Maybe just need to sync up your branch. But it should be fine.
@magento run all tests |
@magento run all tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good.
Just a minor static fix for class descriptions.
@@ -7,7 +7,7 @@ | |||
|
|||
/** | |||
* StoreConfig interface | |||
* | |||
* Interface for store config |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For each of these where you update the class description, can you remove the old, useless description. and add a new line
so for this one it would be
/**
* Interface for store config
*
* @api
composer.lock
Outdated
@@ -4,7 +4,7 @@ | |||
"Read more about it at https://getcomposer.org/doc/01-basic-usage.md#installing-dependencies", | |||
"This file is @generated automatically" | |||
], | |||
"content-hash": "e86af25d9a4a1942c437cca58f9f1efb", | |||
"content-hash": "f3674961f96b48fdd025a6c94610c8eb", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, so actually this change matches the hash in mainline, so I don't know why it is showing up as a change here. Maybe just need to sync up your branch. But it should be fine.
@magento run all tests |
Hi @sasha19957099, thank you for your contribution! |
…d store_name
Description (*)
Related Pull Requests
Fixed Issues (if relevant)
Manual testing scenarios (*)
Questions or comments
Contribution checklist (*)