Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

[WebProfilerBundle] add a way to limit ajax request#25557

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

Merged

Conversation

@Simperfit
Copy link
Contributor

@SimperfitSimperfit commentedDec 20, 2017•
edited
Loading

QA
Branch?master
Bug fix?yes
New feature?yes
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed tickets#22688
LicenseMIT
Doc PRsymfony/symfony-docs#...

I need to add the doc entry and the reproducer to test that everything is ok.

@Simperfit
Copy link
ContributorAuthor

Status: Needs Work

@SimperfitSimperfitforce-pushed thefeature/add-max-ajax-for-web-profiler branch froma97f89e tod8a00a8CompareDecember 20, 2017 05:31
@SimperfitSimperfit changed the title[WIP][WebProfilerBundle] add a way to limit ajax request[WebProfilerBundle] add a way to limit ajax requestDec 20, 2017
@nicolas-grekasnicolas-grekas added this to the4.1 milestoneDec 20, 2017
@javiereguiluz
Copy link
Member

javiereguiluz commentedDec 20, 2017•
edited
Loading

I don't like adding a new config option for this not-so-important feature. What if we set a non-configurable and common sense limit (e.g.100 requests) and when you get MAX + 1 requests, the first one is dropped and so on.

Koc and imiroslavov reacted with thumbs up emoji

@ostrolucky
Copy link
Contributor

ostrolucky commentedDec 20, 2017•
edited
Loading

I like idea about cutting off oldest requests, rather than stopping flow of new ones

@Simperfit
Copy link
ContributorAuthor

@javiereguiluz I guess it makes more sens.

@ostrolucky Yes, why not I didn't think of that in the first place but it could be nice too.

@Simperfit
Copy link
ContributorAuthor

Simperfit commentedJan 26, 2018•
edited
Loading

@javiereguiluz so even if it's not the requested feature, is it really worth it to add this non-configurable limit ? I guess we could let it configurable, it make sense to me too WDYT ?

@fabpot
Copy link
Member

Please, don't make it configurable. It does not make sense.

@Simperfit
Copy link
ContributorAuthor

Simperfit commentedJan 26, 2018•
edited
Loading

So we agree for a limit like 100 requests, after that we drop the olds one to have the new one ?

@fabpot
Copy link
Member

LGTM

@SimperfitSimperfitforce-pushed thefeature/add-max-ajax-for-web-profiler branch fromd8a00a8 to1e21481CompareJanuary 26, 2018 15:14
@Simperfit
Copy link
ContributorAuthor

Status: Needs Review

@Simperfit
Copy link
ContributorAuthor

@fabpot@javiereguiluz@ostrolucky could you please review this one ?

@fabpot
Copy link
Member

It looks good to me. The current patch looks like a bug fix now. So, I would merge it into 2.7 instead of master. WDYT?

@Simperfit
Copy link
ContributorAuthor

I agree, let's look on what other are thinking and ill rebase it.

@fabpot
Copy link
Member

I think you can go ahead and rebase on 2.7.

Copy link
Member

@nicolas-grekasnicolas-grekas left a comment

Choose a reason for hiding this comment

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

for 2.7

@SimperfitSimperfitforce-pushed thefeature/add-max-ajax-for-web-profiler branch from1e21481 toef7bfd1CompareFebruary 19, 2018 14:15
@SimperfitSimperfit changed the base branch frommaster to2.7February 19, 2018 14:15
@SimperfitSimperfitforce-pushed thefeature/add-max-ajax-for-web-profiler branch fromef7bfd1 to5c6e7f1CompareFebruary 19, 2018 14:16
@SimperfitSimperfitforce-pushed thefeature/add-max-ajax-for-web-profiler branch from5c6e7f1 to9ff86d6CompareFebruary 19, 2018 14:18
@Simperfit
Copy link
ContributorAuthor

rebased.

@fabpot
Copy link
Member

Thank you@Simperfit.

@fabpotfabpot merged commit9ff86d6 intosymfony:2.7Feb 19, 2018
fabpot added a commit that referenced this pull requestFeb 19, 2018
…rfit)This PR was merged into the 2.7 branch.Discussion----------[WebProfilerBundle] add a way to limit ajax request| Q             | A| ------------- | ---| Branch?       | master| Bug fix?      | yes| New feature?  | yes <!-- don't forget to update src/**/CHANGELOG.md files -->| BC breaks?    | no| Deprecations? | no <!-- don't forget to update UPGRADE-*.md files -->| Tests pass?   | yes| Fixed tickets |#22688| License       | MIT| Doc PR        | symfony/symfony-docs#... <!--highly recommended for new features-->I need to add the doc entry and the reproducer to test that everything is ok.Commits-------9ff86d6 [WebProfilerBundle] limit ajax request to 100 and remove the last one
@nicolas-grekas
Copy link
Member

@Simperfit I did not manage to merge this change into 3.4, as the JS has changed too much, please submit another PR for 3.4. Sorry about that.

@Simperfit
Copy link
ContributorAuthor

Simperfit commentedFeb 22, 2018 via email

i'm doing it2018-02-22 12:00 GMT+01:00 Nicolas Grekas <notifications@github.com>:
…
@Simperfit <https://github.com/simperfit> I did not manage to merge this change into 3.4, as the JS has changed too much, please submit another PR for 3.4. Sorry about that. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#25557 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADSq8o0sTKF3CEnVKfkASocKJ_aTUUsYks5tXUjqgaJpZM4RH646> .

Copy link

@pmoubedpmoubed left a comment

Choose a reason for hiding this comment

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

This code is broken for v2.7.42

overview:1306 Uncaught TypeError: Cannot read property 'count' of undefined
at Object.renderAjaxRequests (overview:1306)
at Sfjs.load.maxTries (overview:1306)
at overview:1306
at XMLHttpRequest.xhr.onreadystatechange (overview:1306)
renderAjaxRequests @ overview:1306
Sfjs.load.maxTries @ overview:1306
(anonymous) @ overview:1306
xhr.onreadystatechange @ overview:1306
XMLHttpRequest.send (async)
request @ overview:1306
load @ overview:1306
(anonymous) @ overview:1306
(anonymous) @ overview:1306
overview:1306 Uncaught TypeError: Cannot read property 'count' of undefined
at Object.renderAjaxRequests (overview:1306)
at XMLHttpRequest. (overview:1306)

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@fabpotfabpotfabpot approved these changes

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

+1 more reviewer

@pmoubedpmoubedpmoubed left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

4.1

Development

Successfully merging this pull request may close these issues.

7 participants

@Simperfit@javiereguiluz@ostrolucky@fabpot@nicolas-grekas@pmoubed@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp