Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Webprofiler add status code to search form #17125

Closed
wants to merge 2 commits into from

Conversation

Projects
None yet
9 participants
@oktapodia
Copy link
Contributor

commented Dec 23, 2015

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #13034
License MIT

With this PR, in the web profiler, you can filter by HTTP status codes.

Before filter
image

After filter
image

@oktapodia

This comment has been minimized.

Copy link
Contributor Author

commented Dec 23, 2015

Following the PR #15111

$end = $request->query->get('end', $session->get('_profiler_search_end'));
$limit = $request->query->get('limit', $session->get('_profiler_search_limit'));
$token = $request->query->get('token', $session->get('_profiler_search_token'));
$ip = $session->get('_profiler_search_ip');

This comment has been minimized.

Copy link
@sstok

sstok Dec 26, 2015

Contributor

Are these changes correct?

list($csvToken, $csvIp, $csvMethod, $csvUrl, $csvTime, $csvParent) = $values;
$csvStatusCode = isset($values[6]) ? $values[6] : null;
list($csvToken, $csvIp, $csvMethod, $csvUrl, $csvTime, $csvParent,$csvStatusCode) = $values;

This comment has been minimized.

Copy link
@sstok

sstok Dec 26, 2015

Contributor

Missing space after comma.

@sstok

This comment has been minimized.

Copy link
Contributor

commented Dec 26, 2015

Status: Needs work

@oktapodia

This comment has been minimized.

Copy link
Contributor Author

commented Dec 26, 2015

@sstok Oops ! Mistake after the rebase, FIxed.

@@ -16,6 +16,11 @@
</div>

<div class="form-group">
<label for="status_code">Status</label>
<input type="text" name="status_code" id="status_code" value="{{ status_code }}">

This comment has been minimized.

Copy link
@GuilhemN

GuilhemN Dec 27, 2015

Contributor

Why not using a number input here?

@@ -181,7 +181,7 @@ public function setStatusCode($statusCode)
}
/**
* @return int
* @return string

This comment has been minimized.

Copy link
@GuilhemN

GuilhemN Dec 27, 2015

Contributor

Why did you change this?

@oktapodia

This comment has been minimized.

Copy link
Contributor Author

commented Jan 11, 2016

ping @sstok Can I have some news please ?

{
$profiler = new Profiler($this->storage);
$this->assertCount(0, $profiler->find(null, null, null, null, null, null, 204));

This comment has been minimized.

Copy link
@fabpot

fabpot Jan 25, 2016

Member

AFAIU, you do not test anything here. Can you add a test where the count is non-zero?

@javiereguiluz

This comment has been minimized.

Copy link
Member

commented Jan 28, 2016

@oktapodia your pull request looks good and the proposed feature is nice! We just need a final push to finish it. Do you think you can add the test asked for by Fabien? Thanks.

@stof

This comment has been minimized.

Copy link
Member

commented Jan 28, 2016

Status: needs work

@oktapodia

This comment has been minimized.

Copy link
Contributor Author

commented Feb 1, 2016

I think there are some problems with your tests.

screen

@GuilhemN

This comment has been minimized.

Copy link
Contributor

commented Feb 1, 2016

Try to rebase on master.

@oktapodia

This comment has been minimized.

Copy link
Contributor Author

commented Feb 1, 2016

@Ener-Getick Thanks but my PR is not mergeable if I rebase on master

@GuilhemN

This comment has been minimized.

Copy link
Contributor

commented Feb 1, 2016

This is a new feature so it will be merged in master anyway. Try to rebase it on 3.0 otherwise.

@oktapodia

This comment has been minimized.

Copy link
Contributor Author

commented Feb 1, 2016

Same problem :(

@GuilhemN

This comment has been minimized.

Copy link
Contributor

commented Feb 1, 2016

It's apparently not related to your PR (see https://github.com/symfony/symfony/branches). So don't worry ;)

@javiereguiluz javiereguiluz modified the milestone: 3.1 Feb 2, 2016

@javiereguiluz

This comment has been minimized.

Copy link
Member

commented Feb 9, 2016

Failed tests have been fixed. I've played with this feature and it works as expected:

200

404

👍

Status: reviewed

@xabbuh

This comment has been minimized.

Copy link
Member

commented Feb 9, 2016

👍

@oktapodia

This comment has been minimized.

Copy link
Contributor Author

commented Feb 9, 2016

@fabpot

This comment has been minimized.

Copy link
Member

commented Feb 9, 2016

Thank you @oktapodia.

@fabpot fabpot closed this Feb 9, 2016

fabpot added a commit that referenced this pull request Feb 9, 2016

feature #17125 Webprofiler add status code to search form (oktapodia)
This PR was submitted for the 3.0 branch but it was merged into the 3.1-dev branch instead (closes #17125).

Discussion
----------

Webprofiler add status code to search form

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #13034
| License       | MIT

With this PR, in the web profiler, you can filter by HTTP status codes.

*Before filter*
![image](https://i.gyazo.com/8cb99295b12489cc9024ccc07fabf805.png)

*After filter*
![image](https://i.gyazo.com/4caaf032b56ccfe84198a230dbb211a2.png)

Commits
-------

7d3700a Webprofiler add status code to search form
@rvanlaak

This comment has been minimized.

Copy link
Contributor

commented Feb 14, 2016

👍

@oktapodia oktapodia deleted the oktapodia:webprofiler-add-status-code-to-search-form branch Feb 14, 2016

@fabpot fabpot referenced this pull request May 13, 2016

Merged

Release v3.1.0-BETA1 #18776

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.