387-Test coverage of getting IDs of CMS page/blocks by GraphQL API

[atwixfirster]

Issue: #387

Thank you!

[vpodorozh]

@atwixfirster - thank you for your contribution!
Please review my change request - it is mostly related to backward compatibility.
Thx!

[naydav]

@galaoleksandr @atwixfirster
Looks like we don't need to expand scheme with surrogate(internal/technical) keys.

page_id for Page and block_id for Block are surrogate keys, you should use identifier key for loading of these entities.

The same situation with a Product where sku is a natural identifier but not entity_id (needed just for internal purposes)
That's why we don't provide a possibility to get Product by entity_id in GraphQL

But we still have these fields in Api
\Magento\Cms\Api\Data\BlockInterface::BLOCK_ID
\Magento\Cms\Api\Data\PageInterface::PAGE_ID

@paliarush
Please, share your opinion
Thanks

[vpodorozh]

Hi @naydav - thx for clarifications.
I want to make sure that I've understood you correctly, so we need to do the next:

1. Do not expand type CmsBlock with block_id - identifier should be used instead;
2. type CmsPage should be expanded only with identifier (page_id should not be added);
3. Query interface for cmsPage should be expanded with identifier argument - to make it possible to get data via this key (but not id);
4. Query interface for cmsPage should keep id argument to keep backwards-compatibility;

Am I right?

[naydav]

Hi @naydav - thx for clarifications.
I want to make sure that I've understood you correctly, so we need to do the next:

1. Do not expand type CmsBlock with block_id - identifier should be used instead;
2. type CmsPage should be expanded only with identifier (page_id should not be added);
3. Query interface for cmsPage should be expanded with identifier argument - to make it possible to get data via this key (but not id);
4. Query interface for cmsPage should keep id argument to keep backwards-compatibility;

Am I right?

Correct (and mark it as deprecated with a resolution like use use identifier)
Thanks

[vpodorozh]

Note, I've reported a separate issue for fixing integration tests: #654
1) Magento\GraphQlCache\Controller\Cms\CmsPageCacheTest::testToCheckCmsPageRequestCacheTags Failed asserting that two arrays are equal.

Seems that this failed test caused by changes in cache tag policy for cms page in magento core.

[vpodorozh]

@atwixfirster - could you please take a look at my change request?
Thx!

[vpodorozh]

I'm checking the PR

[vpodorozh]

Hi, last circle of review for backward compatibility policy.
Thx!

[vpodorozh]

@atwixfirster - I've just got insight for @naydav about backwards-compatibility policy in GraphQl project.
For graphql - only schema can be counted as a contract.

So sorry for my snobbism about backwards-compatibility policy :)

[atwixfirster]

@atwixfirster - I've just got insight for @naydav about backwards-compatibility policy in GraphQl project.
For graphql - only schema can be counted as a contract.

So sorry for my snobbism about backwards-compatibility policy :)

sounds like APPROVE? 😊

Thank you for the reviewing!

[ghost]

Hi @atwixfirster, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.