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

ExceptionListener > Add route tag #167

Merged
merged 1 commit into from Dec 7, 2018
Merged

Conversation

ruudk
Copy link
Contributor

@ruudk ruudk commented Nov 7, 2018

This will make it easier to find which controller / route was executed when an exception occurred.

@Jean85
Copy link
Collaborator

Jean85 commented Nov 7, 2018

I'm kinda conflicted on this change. On one hand, I understand how that's useful, but OTOH I think we are deviating from the correct behavior here, since we could use the transaction attribute, see https://docs.sentry.io/clientdev/attributes/

This would change how the errors are logged though...

@ruudk
Copy link
Contributor Author

ruudk commented Nov 7, 2018

Can you point me in the right direct where I could add this transaction attribute?

@Jean85
Copy link
Collaborator

Jean85 commented Nov 7, 2018

From what I can recall, as in https://github.com/getsentry/sentry-php/blob/58172bda837d78de77d6b497f7d372dc2d879115/test/Raven/Tests/ClientTest.php#L822

You can do $client->transaction->push(...)

@ruudk
Copy link
Contributor Author

ruudk commented Nov 7, 2018

I'm not sure if I should make any changes I found that Raven_Client constructor executes this:

        $this->transaction = new Raven_TransactionStack();
        if (static::is_http_request() && isset($_SERVER['PATH_INFO'])) {
            // @codeCoverageIgnoreStart
            $this->transaction->push($_SERVER['PATH_INFO']);
            // @codeCoverageIgnoreEnd
        }

That means that transactions are already working by default?

@ruudk
Copy link
Contributor Author

ruudk commented Nov 7, 2018

The issue that this PR tries to solve, is that as a developer, it really helps to not only know the URL (which is useful) but also know the route name of the url. It's way easier to copy and paste the route name into your IDE and search for the controller than by looking at the url and trying to match the route in the code.

@ruudk
Copy link
Contributor Author

ruudk commented Nov 7, 2018

I'm not sure why Scrutinizer is failing btw, it says:

Fixing PHP Coding Style 11:11
Runs coding-style fixes as per your configured coding-style.

But it doesn't say what is wrong. I ran php-cs-fixer manually and nothing changed.

@Jean85
Copy link
Collaborator

Jean85 commented Nov 8, 2018

The conding style on Scrutinizer may be wrong, it's already checked on Travis, it should be disabled... Who has permission to do that? @mitsuhiko? @dcramer?

@ruudk
Copy link
Contributor Author

ruudk commented Nov 19, 2018

Any news @Jean85 ?

@Jean85
Copy link
Collaborator

Jean85 commented Nov 19, 2018

I do not have permissions on Scrutinizer to fix that. You should add tests to this PR, though.

@ruudk
Copy link
Contributor Author

ruudk commented Nov 19, 2018

What tests? It complains about Runs coding-style fixes as per your configured coding-style. but there is nothing wrong with that. I ran php-cs-fixer manually without changes.
I will add some tests.

@ruudk
Copy link
Contributor Author

ruudk commented Dec 7, 2018

@Jean85 I updated the PR and added the tests.

@Jean85
Copy link
Collaborator

Jean85 commented Dec 7, 2018

CI is failing due to CS, please fix it; it can be done automatically with composer cs-fix.

@ruudk
Copy link
Contributor Author

ruudk commented Dec 7, 2018

Thanks for giving me the command :) Made it super easy to fix!

@Jean85 Jean85 merged commit 0263ae6 into getsentry:master Dec 7, 2018
@ruudk ruudk deleted the patch-2 branch December 7, 2018 17:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants