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

feat: Use laravel client and options #166

Merged
merged 16 commits into from
Jan 15, 2019
Merged

feat: Use laravel client and options #166

merged 16 commits into from
Jan 15, 2019

Conversation

HazAT
Copy link
Member

@HazAT HazAT commented Nov 26, 2018

Lumen and Laravel need to be >= 5.6 because of our requirement of PHP >= 7.1
It also currently uses a local overwrites in the PHP SDK to make it work.

@Jean85
Copy link
Contributor

Jean85 commented Nov 28, 2018

Lumen and Laravel need to be >= 5.6

Are you sure about that? Couldn't you have older versions of Laravel under PHP 7.1 and be happy about that?

@HazAT HazAT changed the title [WIP] feat: Use laravel client and options feat: Use laravel client and options Dec 11, 2018
@HazAT HazAT force-pushed the feat/use-2.0-php-sdk branch from 5201ef9 to b3815af Compare December 11, 2018 17:29
@HazAT HazAT requested a review from stayallive December 11, 2018 17:36
Copy link
Collaborator

@stayallive stayallive left a comment

Choose a reason for hiding this comment

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

  • ofcourse the Logger needs changes for the new Monolog handler (that I saw you already started on).
  • I also need to start testing this a bit, only looked through the code in the PR
  • Fan of removing the Sentry/Laravel folders from src if possible + psr-4

src/Sentry/Laravel/EventHandler.php Outdated Show resolved Hide resolved
'command' => null,
));
Integration::configureScope(function (Scope $scope) use ($event): void {
$scope->setTag('command', '');
Copy link
Collaborator

Choose a reason for hiding this comment

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

Better to add a $scope->unsetTag('command') method to the client maybe?

Then we actually remove the tag instead of setting it to an empty string (which probably has the same affect, but still).

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe removeTag would be better, but I agree.

Copy link
Collaborator

Choose a reason for hiding this comment

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

^ also fine ofcourse 👍

examples/laravel-5.7/composer.json Outdated Show resolved Hide resolved
examples/laravel-5.7/composer.json Outdated Show resolved Hide resolved
@HazAT HazAT merged commit 8fac7c2 into master Jan 15, 2019
@HazAT HazAT deleted the feat/use-2.0-php-sdk branch January 15, 2019 08:41
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.

3 participants