-
Notifications
You must be signed in to change notification settings - Fork 154
Added validation for store id in CMS Block #870
Conversation
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.
Please see my comments.
app/code/Magento/CmsGraphQl/Model/Resolver/DataProvider/Block.php
Outdated
Show resolved
Hide resolved
app/code/Magento/CmsGraphQl/Model/Resolver/DataProvider/Block.php
Outdated
Show resolved
Hide resolved
app/code/Magento/CmsGraphQl/Model/Resolver/DataProvider/Block.php
Outdated
Show resolved
Hide resolved
app/code/Magento/CmsGraphQl/Model/Resolver/DataProvider/Block.php
Outdated
Show resolved
Hide resolved
@@ -5,6 +5,7 @@ | |||
"require": { | |||
"php": "~7.1.3||~7.2.0||~7.3.0", | |||
"magento/framework": "*", | |||
"magento/module-store": "*", |
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.
"magento/module-store": "*", | |
"magento/module-store": "*", |
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 see any changes or comment @lenaorobei
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.
magento/module-store
is redundant dependency
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.
If I don't set it, I get the error: Module Magento\CmsGraphQl has undeclared dependencies: hard [Magento\Store]
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.
Please see address comments.
@magento run Functional Tests build |
Hi @lenaorobei, thank you for the review.
|
Hi @lenaorobei, thank you for the review. |
Hi @lenaorobei, thank you for the review. |
Could you please merge the latest mainline to this PR? In production mode, I am getting this error on schema loading
I have seen this error several times on branches that are oudated. |
devilbox seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. |
Signed-off-by: Tomash Khamlai <tomash.khamlai@gmail.com>
899fbb9
to
d606fcd
Compare
Well done! 👍 Checked. Works as expected✔️ The title is according to Store headers The problem❌ If the block has the scope All store views it is not found. Details of the problemRequest 1query showCmsBlock(
$identifiers: [String]
) {
storeConfig {
code
}
cmsBlocks(
identifiers: $identifiers
) {
items {
content
title
identifier
}
}
} Headers 1No headers Variables 1{
"identifiers": ["test-block-2"]
} Response 1{
"errors": [
{
"message": "The CMS block with the \"test-block-2\" ID doesn't exist.",
"extensions": {
"category": "graphql-no-such-entity"
},
"locations": [
{
"line": 6,
"column": 5
}
],
"path": [
"cmsBlocks",
"items",
0
]
}
],
"data": {
"storeConfig": {
"code": "default"
},
"cmsBlocks": {
"items": [
null
]
}
}
} Request 2query showCmsBlock(
$identifiers: [String]
) {
storeConfig {
code
}
cmsBlocks(
identifiers: $identifiers
) {
items {
content
title
identifier
}
}
} Headers 2{
"Store": "germstv"
} Variables 2{
"identifiers": ["test-block-2"]
} Response 2{
"errors": [
{
"message": "The CMS block with the \"test-block-2\" ID doesn't exist.",
"extensions": {
"category": "graphql-no-such-entity"
},
"locations": [
{
"line": 6,
"column": 5
}
],
"path": [
"cmsBlocks",
"items",
0
]
}
],
"data": {
"storeConfig": {
"code": "germstv"
},
"cmsBlocks": {
"items": [
null
]
}
}
} Blocks enabledBlock that was requested |
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.
@Rus0, could you please address found issues?
Sure, I will later this week
…On Tue, Oct 1, 2019 at 5:47 AM Lena Orobei ***@***.***> wrote:
***@***.**** requested changes on this pull request.
@Rus0 <https://github.com/Rus0>, could you please address found issues?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#870?email_source=notifications&email_token=ABS3RGBXOUVMD7NOAA72QFTQMMTF3A5CNFSM4IRV26P2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCGOTFVQ#pullrequestreview-295514838>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABS3RGHOT4VC3AB2TDPHBFLQMMTF3ANCNFSM4IRV26PQ>
.
|
@Rus0 unfortunately, I need to close this PR since there is no activity. Please feel free to reopen if you want to continue working on it. Thank you. |
Description (*)
Adding a filter for store id in the blocks, using the store send in the headers
Fixed Issues (if relevant)
Manual testing scenarios (*)
Questions or comments
Contribution checklist (*)