NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

[WebProfiler] added support for window.fetch calls in ajax section#19576

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Closed

ivoba wants to merge4 commits intosymfony:masterfromfreshcells:ticket_17444

Closed

[WebProfiler] added support for window.fetch calls in ajax section#19576

ivoba wants to merge4 commits intosymfony:masterfromfreshcells:ticket_17444

Conversation

Copy link

Contributor

ivoba commentedAug 9, 2016•
edited
Loading

Q	A
Branch?	"master"
Bug fix?	yes
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#17444
License	MIT
Doc PR	reference to the documentation PR, if any

This adds support for window.fetch calls to the Ajax section of the WebProfiler toolbar.

Credits to@tbopec for implementation :)

added support for window.fetch calls in ajax section

af9225d

carsonbot added Status: Needs Review Bug Feature labels

Aug 9, 2016

Copy link

Member

fabpot commentedAug 15, 2016

@javiereguiluz Can you validate this one?

Copy link

Member

fabpot commentedAug 15, 2016

@robfrawley Can you test it?

Copy link

Member

nicolas-grekas commentedAug 23, 2016

Could anyone please test this one?

stof reviewed

Aug 23, 2016

View reviewed changes

src/Symfony/Bundle/WebProfilerBundle/Resources/views/Profiler/base_js.html.twig

		stackElement.error=r.status<200\|\|r.status>=400;
		stackElement.statusCode=r.status;
		stackElement.profile=r.headers.get('x-debug-token');
		stackElement.profilerUrl=r.headers.get('x-debug-token-link');

Copy link

Member

Choose a reason for hiding this comment

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

Is this compatible with CORS restriction or will it log a security warning ?

Copy link

Member

Choose a reason for hiding this comment

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

@romainneutron any ideas?

Copy link

Contributor

Choose a reason for hiding this comment

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

I'm gonna try this PR

Copy link

Member

stof commentedAug 23, 2016

Status: needs work

carsonbot added Status: Needs Work and removed Status: Needs Review labels

Aug 23, 2016

Copy link

ContributorAuthor

stofAug 24, 2016•
edited
Loading

Choose a reason for hiding this comment

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

This removal must be reverted. You are breaking the CSP compatibility

Copy link

Member

I will review again when the indentation changes are reverted. They make it very hard to see what actually changed (and if I ignored the whitespace changes, I cannot comment on the diff anymore, which is not convenient)

Copy link

ContributorAuthor

ivoba commentedAug 24, 2016

oops how did that happen?
fix is coming...

ivoba force-pushed theticket_17444 branch fromb8ec1e9 toc4c33f7Compare

August 24, 2016 08:49

Copy link

ContributorAuthor

ivoba commentedAug 24, 2016

@stof done, sorryy

aegyed91 reviewed

Sep 5, 2016

View reviewed changes

src/Symfony/Bundle/WebProfilerBundle/Resources/views/Profiler/base_js.html.twig Outdated

		var promise=oldFetch.apply(null,arguments);

		var method='GET';
		if(arguments[1].method!==undefined){

Copy link

aegyed91Sep 5, 2016

Choose a reason for hiding this comment

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

@ivoba Theinit config object is optional (Promise<Response> fetch(input[, init]);) therefore this can throw an error. First check if the 2nd argument even existsarguments[1] && arguments[1].method !== undefined .

Copy link

ContributorAuthor

ivobaSep 12, 2016

Choose a reason for hiding this comment

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

@tsm91 correct. i fixed this.

ivoba force-pushed theticket_17444 branch fromc4c33f7 tof6c4e52Compare

September 12, 2016 13:22

fabpot reviewed

Sep 14, 2016

View reviewed changes

src/Symfony/Bundle/WebProfilerBundle/Resources/views/Profiler/base_js.html.twig

		}

		{%if excluded_ajax_paths is defined%}

Copy link

Member

fabpotSep 14, 2016•
edited
Loading

Choose a reason for hiding this comment

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

extra blank line here

src/Symfony/Bundle/WebProfilerBundle/Resources/views/Profiler/base_js.html.twig Outdated

		var oldFetch=window.fetch;
		window.fetch=function () {
		if (!arguments[0].match(newRegExp({{ excluded_ajax_paths\|json_encode\|raw }}))) {

Copy link

Member

fabpotSep 14, 2016

Choose a reason for hiding this comment

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

extra blank line here

src/Symfony/Bundle/WebProfilerBundle/Resources/views/Profiler/base_js.html.twig Outdated


		var method='GET';

		if(arguments[1]&&arguments[1].method!==undefined){

Copy link

Member

fabpotSep 14, 2016

Choose a reason for hiding this comment

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

missing space afterif and before{

ivoba force-pushed theticket_17444 branch fromf6c4e52 to0e72c2eCompare

September 15, 2016 12:18

Copy link

RolfBabijn commentedSep 27, 2016

Really looking forward to this fix/feature. Is there any way I can assist?

stof requested changes

Sep 27, 2016

View reviewed changes

src/Symfony/Bundle/WebProfilerBundle/Resources/views/Profiler/base_js.html.twig Outdated

		var oldFetch=window.fetch;
		window.fetch=function () {
		if (!arguments[0].match(newRegExp({{ excluded_ajax_paths\|json_encode\|raw }}))) {
		var promise=oldFetch.apply(null,arguments);

Copy link

Member

stofSep 27, 2016

Choose a reason for hiding this comment

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

this call must be done outside the URL profiling blacklist check (and the return at the end too). Otherwise, you break fetch entirely for URLs blacklisted by the AJAX profiling

Copy link

ContributorAuthor

ivobaSep 29, 2016

Choose a reason for hiding this comment

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

@stof fixed

applied suggested fixes:

e080ca1

added error handling on promise catch,moved fetch block out of XHR block,fixed method detect,matched url against excluded urls

ivoba force-pushed theticket_17444 branch from0e72c2e toe080ca1Compare

September 29, 2016 08:18

romainneutron suggested changes

Sep 30, 2016

View reviewed changes

src/Symfony/Bundle/WebProfilerBundle/Resources/views/Profiler/base_js.html.twig Outdated

		if (window.fetch) {
		var oldFetch=window.fetch;
		window.fetch=function () {
		var promise=oldFetch.apply(null,arguments);

Copy link

Contributor

romainneutronSep 30, 2016

Choose a reason for hiding this comment

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

oldFetch.apply(window, arguments);

Copy link

Member

stofSep 30, 2016•
edited
Loading

Choose a reason for hiding this comment

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

even better:.apply(this, arguments), keeping the same context

Copy link

ContributorAuthor

ivobaOct 5, 2016

Choose a reason for hiding this comment

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

fixed using@stof 's approach

src/Symfony/Bundle/WebProfilerBundle/Resources/views/Profiler/base_js.html.twig Outdated

		stackElement.profile=r.headers.get('x-debug-token');
		stackElement.profilerUrl=r.headers.get('x-debug-token-link');
		Sfjs.renderAjaxRequests();
		}).catch(function(err) {

Copy link

Contributor

romainneutronSep 30, 2016•
edited
Loading

Choose a reason for hiding this comment

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

it's better to use the second argument ofthen instead of usingcatch: if an error is thrown bySfjs.renderAjaxRequests, the request would be tagged as erroneous whereas the error came from the code executed after that.

Copy link

ContributorAuthor

ivobaOct 5, 2016

Choose a reason for hiding this comment

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

fixed

src/Symfony/Bundle/WebProfilerBundle/Resources/views/Profiler/base_js.html.twig

		}).catch(function(err) {
		stackElement.loading=false;
		stackElement.error=true;
		});

Copy link

Contributor

romainneutronSep 30, 2016

Choose a reason for hiding this comment

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

what about adding the error content as a property and display this error in the toolbar?

Copy link

ContributorAuthor

ivobaOct 5, 2016

Choose a reason for hiding this comment

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

its a good idea, but maybe we can put this in another PR since its also not implemented for XHR, afaik, and it would need some more adjustments in how we render the table.

src/Symfony/Bundle/WebProfilerBundle/Resources/views/Profiler/base_js.html.twig Outdated

		if (window.fetch) {
		var oldFetch=window.fetch;
		window.fetch=function () {
		var promise=oldFetch.apply(null,arguments);

Copy link

Contributor

romainneutronSep 30, 2016•
edited
Loading

Choose a reason for hiding this comment

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

Works perfectly, no security error, even incors mode

src/Symfony/Bundle/WebProfilerBundle/Resources/views/Profiler/base_js.html.twig

		Sfjs.renderAjaxRequests();
		}).catch(function(err) {
		stackElement.loading=false;
		stackElement.error=true;

Copy link

Contributor

romainneutronSep 30, 2016

Choose a reason for hiding this comment

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

what about adding atype so we could make distinction betweenajax (xhr) andfetch in the toolbar. As they do not behave the same way, it may be useful, WDYT?

Copy link

ContributorAuthor

ivobaSep 30, 2016

Choose a reason for hiding this comment

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

yeah was thinking about that too, but was bit afraid of the extra effort. i see what i can do.

Copy link

Member

stofSep 30, 2016

Choose a reason for hiding this comment

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

well, the extra work is mainly about figuring out the display. Adding the type property is not hard (it should just be done in both place initializing a stackElement)

Copy link

ContributorAuthor

ivobaOct 5, 2016

Choose a reason for hiding this comment

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

i added type to stackElement and to display

romainneutron reviewed

Sep 30, 2016

View reviewed changes

src/Symfony/Bundle/WebProfilerBundle/Resources/views/Profiler/base_js.html.twig


		if (window.fetch) {
		var oldFetch=window.fetch;
		window.fetch=function () {

Copy link

Contributor

romainneutronSep 30, 2016

Choose a reason for hiding this comment

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

what about usingfetch signature (fetch(url, options):promise) instead of relying on arguments? I think it's easier to read

Copy link

ContributorAuthor

ivobaOct 5, 2016

Choose a reason for hiding this comment

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

absolutly, fixed

stof reviewed

Sep 30, 2016

View reviewed changes

src/Symfony/Bundle/WebProfilerBundle/Resources/views/Profiler/base_js.html.twig Outdated


		{%if excluded_ajax_paths is defined%}

		if (window.fetch) {

Copy link

Member

stofSep 30, 2016

Choose a reason for hiding this comment

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

should we try to detect native fetch here ? If someone uses a fetch polyfill based on XHR (e.g. the polyfill maintained by github), we would register the request twice otherwise (at the fetch level and at the xhr level)

Copy link

ContributorAuthor

ivobaOct 5, 2016

Choose a reason for hiding this comment

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

how would you detect this? just check if the browser supports fetch? this needs to happen before any ployfill is loaded, im not sure how to do this.
also as the profiler is a dev tool, how many people develop on outdated browsers?

Copy link

Member

stofOct 5, 2016

Choose a reason for hiding this comment

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

the polyfill defines awindow.fetch.polyfill property (set to true) to allow to identify itself.

And people are expected to try their site in old browsers from time to time if they support them. Having your dev tools working to debug errors is a good idea there. The Symfony debug toolbar is meant to work in old browsers, even if it is less fancy (as it is embed in the page itself). The profiler does not support them, but you can always open it in a modern browser in parallel.

Copy link

ContributorAuthor

ivobaOct 5, 2016

Choose a reason for hiding this comment

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

ok i see, i added a check on polyfillif (window.fetch && window.fetch.polyfill === undefined)

Copy link

Contributor

HeahDude commentedOct 1, 2016

@ivoba Do you need help with the templates so you can finish this feature for 3.2? :)

ivoba added2 commits

October 5, 2016 10:23

added type to profiler table, use 2nd argument instead of catch, usin…

36cad1e

…g fetch signature instead of arguments

fixed polyfill detect

385279c

stof approved these changes

Oct 5, 2016

View reviewed changes

Copy link

Member

fabpot commentedOct 5, 2016

Thank you@ivoba.

fabpot closed this in2101c10

Oct 5, 2016

patrick-mcdougle mentioned this pull request

Oct 10, 2016

Limiting web-profiler-toolbar AJAX request count#20155

Closed

fabpot mentioned this pull request

Oct 27, 2016

Release v3.2.0-BETA1#20317

Merged

This was referencedDec 1, 2016

[WebProfilerBundle] Fix AJAX panel width for long URLs#20715

Merged

[WebProfilerBundle] Fix toolbar max-width for long ajax url#20713

Closed

ostrolucky pushed a commit to ostrolucky/symfony that referenced this pull request

Mar 25, 2018

featuresymfony#19576[WebProfiler] added support for window.fetch ca…

0ce2b63

…lls in ajax section (ivoba)This PR was squashed before being merged into the 3.2-dev branch (closessymfony#19576).Discussion----------[WebProfiler] added support for window.fetch calls in ajax section| Q             | A| ------------- | ---| Branch?       | "master"| Bug fix?      | yes| New feature?  | yes| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets |symfony#17444| License       | MIT| Doc PR        | reference to the documentation PR, if anyThis adds support for window.fetch calls to the Ajax section of the WebProfiler toolbar.Credits to@tbopec for  implementation :)Commits-------b1b4d70 [WebProfiler] added support for window.fetch calls in ajax section

Labels

Bug Feature Status: Needs Work

		@@ -1,4 +1,4 @@
		<script{%ifcsp_script_nonceisdefinedandcsp_script_nonce %}nonce={{csp_script_nonce }}{%endif %}>/<![CDATA[/

Movatterモバイル変換

Uh oh!

[WebProfiler] added support for window.fetch calls in ajax section#19576

[WebProfiler] added support for window.fetch calls in ajax section#19576

Uh oh!

Conversation

ivoba commentedAug 9, 2016• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

fabpot commentedAug 15, 2016

Uh oh!

fabpot commentedAug 15, 2016

Uh oh!

nicolas-grekas commentedAug 23, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stof commentedAug 23, 2016

Uh oh!

ivoba commentedAug 24, 2016

Uh oh!

stofAug 24, 2016• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stof commentedAug 24, 2016

Uh oh!

ivoba commentedAug 24, 2016

Uh oh!

ivoba commentedAug 24, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabpotSep 14, 2016• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RolfBabijn commentedSep 27, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stofSep 30, 2016• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

romainneutronSep 30, 2016• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

romainneutronSep 30, 2016• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

ivoba commentedAug 9, 2016•
edited
Loading

stofAug 24, 2016•
edited
Loading

fabpotSep 14, 2016•
edited
Loading

stofSep 30, 2016•
edited
Loading

romainneutronSep 30, 2016•
edited
Loading

romainneutronSep 30, 2016•
edited
Loading