[WebProfilerBundle] add a way to limit ajax request #25557
Conversation
|
Status: Needs Work |
a97f89e to
d8a00a8
Compare
|
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. |
|
I like idea about cutting off oldest requests, rather than stopping flow of new ones |
|
@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. |
|
@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 ? |
|
Please, don't make it configurable. It does not make sense. |
|
So we agree for a limit like 100 requests, after that we drop the olds one to have the new one ? |
|
LGTM |
d8a00a8 to
1e21481
Compare
|
Status: Needs Review |
|
@fabpot @javiereguiluz @ostrolucky could you please review this one ? |
|
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? |
|
I agree, let's look on what other are thinking and ill rebase it. |
|
I think you can go ahead and rebase on 2.7. |
1e21481 to
ef7bfd1
Compare
ef7bfd1 to
5c6e7f1
Compare
5c6e7f1 to
9ff86d6
Compare
|
rebased. |
|
Thank you @Simperfit. |
…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
|
@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. |
|
i'm doing it
2018-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>
.
|
There was a problem hiding this comment.
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)
I need to add the doc entry and the reproducer to test that everything is ok.