GraphQL CORS Headers #28713

michalderlatka · 2020-06-12T17:43:40Z

Description (*)

As a web developer, I want Magento to support the CORS standard so that I can develop my applications according to defined best practices.

Related Pull Requests

Fixed Issues (if relevant)

Fixes GraphQL : CORS requests will fail without OPTIONS reponse #28561

Manual testing scenarios (*)

go to admin > stores > configuration > services > graphql
enable CORS headers
Fill fields with data
save and clean cache
run graphQL query and confirm headers are added

Questions or comments

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)

m2-assistant · 2020-06-12T17:43:43Z

Hi @michalderlatka. 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

❗ Automated tests can be triggered manually with an appropriate comment:

@magento run all tests - run or re-run all required tests against the PR changes
@magento run <test-build(s)> - run or re-run specific test build(s)
For example: @magento run Unit Tests

<test-build(s)> is a comma-separated list of build names. Allowed build names are:

Database Compare
Functional Tests CE
Functional Tests EE,
Functional Tests B2B
Integration Tests
Magento Health Index
Sample Data Tests CE
Sample Data Tests EE
Sample Data Tests B2B
Static Tests
Unit Tests
WebAPI Tests

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.

lenaorobei · 2020-06-16T19:34:03Z

@magento run all tests

app/code/Magento/GraphQl/Controller/HttpResponse/Cors/AbstractCorsHeaderProvider.php

app/code/Magento/GraphQl/Controller/HttpResponse/Cors/CorsAllowHeadersHeaderProvider.php

app/code/Magento/GraphQl/Controller/HttpResponse/Cors/CorsAllowMethodsHeaderProvider.php

app/code/Magento/GraphQl/Controller/HttpResponse/Cors/CorsMaxAgeHeaderProvider.php

app/code/Magento/GraphQl/Controller/HttpResponse/Cors/CorsAllowCredentialsHeaderProvider.php

cpartica

just a few "static failures" that need addressing,
you'll see them in the builds that I just just ran https://github.com/magento/magento2/pull/28713/checks?check_run_id=797040005

app/code/Magento/GraphQl/Controller/HttpResponse/Cors/CorsAllowHeadersHeaderProvider.php

cpartica · 2020-06-22T21:31:43Z

@magento run all tests

…ka/magento2 into 28561_graphql_cors_requests

michalderlatka · 2020-06-23T08:26:47Z

@magento run all tests

michalderlatka · 2020-06-23T09:52:08Z

@magento run all tests

YevSent · 2020-06-24T18:08:52Z

app/code/Magento/GraphQl/Model/Cors/ConfigurationInterface.php

+     *
+     * @return bool
+     */
+    public function isEnabled() : bool;


Redundant space after ) and before :.

YevSent · 2020-06-24T18:09:23Z

app/code/Magento/GraphQl/Model/Cors/Configuration.php

+ */
+class Configuration implements ConfigurationInterface
+{
+    const XML_PATH_CORS_HEADERS_ENABLED = 'graphql/cors/enabled';


Constants should have visibility scope.

YevSent · 2020-06-24T18:10:38Z

app/code/Magento/GraphQl/Controller/HttpResponse/Cors/CorsAllowMethodsHeaderProvider.php

+     *
+     * @return string
+     */
+    public function getName()


The method return type is missed.

YevSent · 2020-06-24T18:12:47Z

app/code/Magento/GraphQl/etc/di.xml

@@ -98,4 +98,31 @@
            <argument name="queryComplexity" xsi:type="number">300</argument>
        </arguments>
    </type>
+
+    <preference for="Magento\GraphQl\Model\Cors\ConfigurationInterface" type="Magento\GraphQl\Model\Cors\Configuration" />
+    <type name="\Magento\GraphQl\Controller\HttpResponse\Cors\CorsMaxAgeHeaderProvider">


Redundant \ in FQCN.

YevSent · 2020-06-24T18:13:32Z

dev/tests/api-functional/testsuite/Magento/GraphQl/CorsHeadersTest.php

+
+    protected function tearDown(): void
+    {
+        parent::tearDown(); // TODO: Change the autogenerated stub


@TODO annotations should be removed from code.

YevSent · 2020-06-24T18:15:25Z

dev/tests/api-functional/testsuite/Magento/GraphQl/CorsHeadersTest.php

+        self::assertArrayNotHasKey('Access-Control-Allow-Origin', $headers);
+    }
+
+    private function getHeadersFromIntrospectionQuery()


The method return type is missed.

we don't usually do it on tests, but since we upgraded phpunit we might have to

It doesn't matter if a method is a test class method or a regular code. All code should follow the same semantic, the only exception is doc blocks, they can be omitted for test methods.

cpartica · 2020-06-24T18:20:49Z

dev/tests/api-functional/testsuite/Magento/GraphQl/CorsHeadersTest.php

+        self::assertArrayNotHasKey('Access-Control-Allow-Origin', $headers);
+    }
+
+    private function getHeadersFromIntrospectionQuery()


we don't usually do it on tests, but since we upgraded phpunit we might have to

…o 28561_graphql_cors_requests

michalderlatka · 2020-06-25T18:42:26Z

@magento run all tests

m2-assistant · 2020-06-27T17:50:34Z

Hi @michalderlatka, 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.

ghost added the Progress: pending review label Jun 12, 2020

magento-engcom-team added Component: GraphQL GraphQL Release Line: 2.4 labels Jun 12, 2020

michalderlatka requested review from cpartica and danielrenaud June 12, 2020 17:43

michalderlatka added the Project: GraphQL label Jun 12, 2020

nrkapoor assigned michalderlatka Jun 13, 2020

nrkapoor added this to the 2.4.1 milestone Jun 13, 2020

nrkapoor requested a review from paliarush June 13, 2020 13:50

magento#28561: GraphQL added CORS headers

50635d9

michalderlatka force-pushed the 28561_graphql_cors_requests branch from 3db87dc to 50635d9 Compare June 18, 2020 12:22

cpartica requested changes Jun 18, 2020

View reviewed changes

ghost assigned cpartica Jun 18, 2020

ghost added Progress: needs update and removed Progress: pending review labels Jun 18, 2020

magento#28561: GraphQL added CORS headers

b79c484

michalderlatka requested a review from cpartica June 22, 2020 12:49

cpartica requested changes Jun 22, 2020

View reviewed changes

app/code/Magento/GraphQl/Controller/HttpResponse/Cors/CorsAllowHeadersHeaderProvider.php Show resolved Hide resolved

Merge branch '2.4-develop' into 28561_graphql_cors_requests

69cb58c

michalderlatka added 2 commits June 23, 2020 10:23

magento#28561: GraphQL added CORS headers (static test fix)

5ba8fd7

Merge branch '28561_graphql_cors_requests' of github.com:michalderlat…

c3def13

…ka/magento2 into 28561_graphql_cors_requests

magento#28561: GraphQL added CORS headers (static test fix)

e82cca4

michalderlatka requested a review from cpartica June 23, 2020 12:01

cpartica previously approved these changes Jun 23, 2020

View reviewed changes

ghost removed the Progress: needs update label Jun 23, 2020

YevSent reviewed Jun 24, 2020

View reviewed changes

ghost assigned YevSent Jun 24, 2020

cpartica requested changes Jun 24, 2020

View reviewed changes

ghost added Progress: needs update and removed Progress: pending approval labels Jun 24, 2020

cpartica added the PAP Partners acceleration program label Jun 25, 2020

michalderlatka added 2 commits June 25, 2020 07:03

Merge branch '2.4-develop' of https://github.com/magento/magento2 int…

64b4228

…o 28561_graphql_cors_requests

magento#28561: GraphQL added CORS headers (fixing issues)

88840a7

michalderlatka requested review from cpartica and YevSent June 25, 2020 08:39

cpartica approved these changes Jun 25, 2020

View reviewed changes

ghost added Progress: ready for testing Progress: testing in progress Progress: accept and removed Progress: needs update Progress: ready for testing Progress: testing in progress labels Jun 25, 2020

magento-engcom-team added Component: GraphQL GraphQL and removed Component: GraphQL GraphQL labels Jun 26, 2020

magento-engcom-team pushed a commit that referenced this pull request Jun 27, 2020

ENGCOM-7751: GraphQL CORS Headers #28713

00290cc

magento-engcom-team merged commit 51cf6c8 into magento:2.4-develop Jun 27, 2020

michalderlatka deleted the 28561_graphql_cors_requests branch June 27, 2020 20:07

This was referenced Jul 30, 2020

Add GraphQL CORS screen magento/merchdocs#637

Merged

GraphQL: Add section about CORS magento/devdocs#7629

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GraphQL CORS Headers #28713

GraphQL CORS Headers #28713

michalderlatka commented Jun 12, 2020

m2-assistant bot commented Jun 12, 2020

lenaorobei commented Jun 16, 2020

cpartica left a comment •

edited

Loading

cpartica commented Jun 22, 2020

michalderlatka commented Jun 23, 2020

michalderlatka commented Jun 23, 2020

YevSent Jun 24, 2020

YevSent Jun 24, 2020

YevSent Jun 24, 2020

YevSent Jun 24, 2020

YevSent Jun 24, 2020

YevSent Jun 24, 2020

cpartica Jun 24, 2020

YevSent Jun 24, 2020

cpartica Jun 24, 2020

michalderlatka commented Jun 25, 2020

m2-assistant bot commented Jun 27, 2020

GraphQL CORS Headers #28713

GraphQL CORS Headers #28713

Conversation

michalderlatka commented Jun 12, 2020

Description (*)

Related Pull Requests

Fixed Issues (if relevant)

Manual testing scenarios (*)

Questions or comments

Contribution checklist (*)

m2-assistant bot commented Jun 12, 2020

lenaorobei commented Jun 16, 2020

cpartica left a comment • edited Loading

Choose a reason for hiding this comment

cpartica commented Jun 22, 2020

michalderlatka commented Jun 23, 2020

michalderlatka commented Jun 23, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michalderlatka commented Jun 25, 2020

m2-assistant bot commented Jun 27, 2020

cpartica left a comment •

edited

Loading