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

Type hint interface instead of class #107

Merged
merged 1 commit into from
Oct 29, 2017

Conversation

vojtasvoboda
Copy link
Contributor

@vojtasvoboda vojtasvoboda commented Oct 8, 2017

Introduction: I'm using OctoberCMS, which is made on the top of Laravel 5.5 framework.

Problem: OctoberCMS uses own implementation of Event Dispatcher and when using Sentry in OctoberCMS, I'm getting this error:

TypeError: Argument 1 passed to Sentry\SentryLaravel\SentryLaravelEventHandler::subscribe() must be an instance of Illuminate\Events\Dispatcher, instance of October\Rain\Events\Dispatcher given, called in /home/ubuntu/sites.com/vendor/sentry/sentry-laravel/src/Sentry/SentryLaravel/SentryLaravelServiceProvider.php on line 72

Solution: Type-hint interface instead of particular class.

@stayallive
Copy link
Collaborator

We would need to split our event handler to maintain 4.x support since 4.x does not implement those contracts (since they were introduced in 5.0). This change as is would break 4.x.

Might be better to not typehint at all maybe?

@vojtasvoboda
Copy link
Contributor Author

How about creating special 4.x branch for a version compatible with old Laravel version. And cherry picking only bug fixes from master (L5.x) version?

@stayallive
Copy link
Collaborator

Yeah, that would possibly be the better plan. Let me work that out.

@stayallive stayallive merged commit 4166a9b into getsentry:master Oct 29, 2017
@stayallive
Copy link
Collaborator

Sorry for the delay. There will have to be some more cleanups before I release the >5.x version (need te remove all 4.x code), but you can use master or a specific commit for now to fix this particular issue.

@vojtasvoboda
Copy link
Contributor Author

Thanks! I'm using this:

"require": {
    "sentry/sentry-laravel": "dev-master#4166a9b"
}

and works well.

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.

4 participants