Skip to content

Set correct Content-Type header for ajax responses in the Magento_UI module. #10521

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Sep 8, 2017
Merged

Set correct Content-Type header for ajax responses in the Magento_UI module. #10521

merged 4 commits into from
Sep 8, 2017

Conversation

hostep
Copy link
Contributor

@hostep hostep commented Aug 13, 2017 •
edited by ishakhsuvarov
Loading

Description

We started noticing issues when New Relic's Browser profiling was enabled, which inserts a javascript block into text/html responses whenever a <head> tag was found in such a response.

This has the possibility to break certain ajax responses in Magento's backend, since they have Content-Type: text/html instead of application/json.

I'm not sure if this fix is the correct fix, and if there aren't other violations in the code for this same problem. Feel free to let me know if that should be the case and I'll try to add them to the PR.

Fixed Issues (if relevant)

None that I could find

Manual testing scenarios

  1. Tested on Magento CE 2.1.7 and develop branch (rev: 45896ba)
  2. Make sure you have a New Relic integration setup with your server and that the Browser Profiling is enabled.
  3. Edit or create a new CMS Page and add the following to the 'Layout Update XML' field:
<head>
    <meta name="robots" content="NOINDEX,NOFOLLOW"/>
</head>
  1. Save the page and return to the cms pages list
  2. The cms page grid no longer loads and an error: "Something went wrong" appears

screen shot 2017-08-13 at 17 02 23

6. Inspecting the response, it contains invalid json, because New Relic inserted its javascript in there: 
...
"layout_update_xml":"<head><script type="text/javascript">window.NREUM||(NREUM={}) ... </script>\r\n    <meta name=\"robots\" content=\"NOINDEX,NOFOLLOW\"\/>\r\n<\/head>"
...

Changing the Content-Type to application/json no longer makes New Relic insert its javascript into the response and the grid loads perfectly fine.

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 on Travis CI are green)

@ishakhsuvarov ishakhsuvarov self-assigned this Aug 14, 2017
@ishakhsuvarov ishakhsuvarov added this to the August 2017 milestone Aug 14, 2017
ishakhsuvarov
ishakhsuvarov previously requested changes Aug 14, 2017
View reviewed changes
app/code/Magento/Ui/Controller/Adminhtml/Index/Render.php Outdated
@@ -30,6 +30,7 @@ public function execute()
$component = $this->factory->create($this->_request->getParam('namespace'));
$this->prepareComponent($component);
$this->_response->appendBody((string) $component->render());
$this->_response->setHeader('Content-Type', 'application/json', true);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Main problem with hard-coding Content-Type here is that the $component->render() has multiple types of renderers.

You may refer to \Magento\Ui\Component\AbstractComponent::render which in place selects the render engine from the classes which implement \Magento\Framework\View\Element\UiComponent\ContentType\ContentTypeInterface.

This list is not limited to Json, but also has an Html and Xml implementations out of the box.
I suggest looking into different approach to solve this issue, such as:

  • Possibly limiting the Content-Type header to the correct renderer
  • Working with the Accept header provided by client to ensure the correct response.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, I wasn't sure if all responses were in json format (didn't take my time to look it through, my mistake).
I'll have a look at a better solution.

@hostep hostep force-pushed the use-json-content-type-header-for-ajax-responses branch from 3752054 to 1e3181c Compare August 15, 2017 11:59
@hostep hostep changed the title Added 'application/json' Content-Type to Ajax responses in the Magento_UI module. Set correct Content-Type header for ajax responses in the Magento_UI module. Aug 15, 2017
@hostep
Copy link
Contributor Author

hostep commented Aug 15, 2017

I force pushed a new commit, this is probably a better way to handle it.
I only tested json responses, couldn't quickly find xml or html responses, so that's not tested.

Comments are welcome.

@hostep hostep force-pushed the use-json-content-type-header-for-ajax-responses branch from 1e3181c to d0b85b3 Compare August 15, 2017 15:19
@ishakhsuvarov
Copy link
Contributor

Hi @hostep
After discussion with @omiroshnichenko and @okorshenko we found that we could add some more flexibility to the proposed solution. I've pushed the proposed changes back to this PR, if you don't mind.

Please let me know what you think of this.
Thank you.

@ishakhsuvarov ishakhsuvarov dismissed their stale review September 7, 2017 09:38

Nor relevant anymore

@hostep
Copy link
Contributor Author

hostep commented Sep 7, 2017

@ishakhsuvarov: nice, looks good.

So it looks like it is now based on the Accept header? That should probably work fine, I just tested (without this patch) the CMS Pages Grid, and the request includes application/json in the Accept header, so it looks like this will fix the bug I described in the initial post.

Good job, and thank you! :)

@ishakhsuvarov
Copy link
Contributor

Accept header seems logical to follow in this case, since not following it violates standards anyway. I've tested some cases, including the one you have described in the issue and it seems to work fine.

@magento-team magento-team merged commit d47cc77 into magento:develop Sep 8, 2017
magento-team pushed a commit that referenced this pull request Sep 8, 2017
@ishakhsuvarov
MAGETWO-71545: Added 'application/json' Content-Type to Ajax response…
ddfc01e 
…s in the Magento_UI module. #10521
magento-team pushed a commit that referenced this pull request Sep 8, 2017
@ishakhsuvarov
Merge pull request #1469 from magento-engcom/develop-prs
7b6e2a0 
[EngCom] Public Pull Requests
 - MAGETWO-72390: Remove the usage of the DataObject for response management #10808
 - MAGETWO-71545: Added 'application/json' Content-Type to Ajax responses in the Magento_UI module. #10521
 - MAGETWO-72388: Fix spelling mistake in AddressTest.php #10806
 - MAGETWO-72283: Code generate: support variadic parameter #10771
hostep pushed a commit to hostep/magento2 that referenced this pull request Feb 7, 2018
@ishakhsuvarov @hostep
Backport of MAGETWO-71545 for Magento 2.1: Added 'application/json' C…
4c45cd8 
…ontent-Type to Ajax responses in the Magento_UI module. magento#10521

(cherry picked from commit ddfc01e)
@hostep hostep mentioned this pull request Feb 7, 2018
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ishakhsuvarov ishakhsuvarov ishakhsuvarov left review comments

Assignees

@ishakhsuvarov ishakhsuvarov

Labels
Progress: accept
Projects
None yet
Milestone
August 2017
Development

Successfully merging this pull request may close these issues.
3 participants
@hostep @ishakhsuvarov @magento-team