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

Fixed determining the builder name #387

Merged
merged 1 commit into from
Feb 25, 2020
Merged

Fixed determining the builder name #387

merged 1 commit into from
Feb 25, 2020

Conversation

GrahamCampbell
Copy link
Contributor

@GrahamCampbell GrahamCampbell commented Feb 16, 2020

Depends on bugsnag/bugsnag-php#572. Closes #369.

@GrahamCampbell GrahamCampbell changed the title Update DeployCommand.php Fixed determining the builder name Feb 16, 2020
Copy link
Contributor

@tomlongridge tomlongridge left a comment

Choose a reason for hiding this comment

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

What's the reason for moving this to bugsnag-php? I wouldn't have thought we want to add Symfony dependencies there in order to keep in framework-neutral. Can't we keep this functionality in bugsnag-laravel?

@GrahamCampbell
Copy link
Contributor Author

Well, the PHP notifier still doesn't depend on symfony. It just uses it if it's available. We could actually delete the symfony code entirely if we really wanted.

@tomlongridge
Copy link
Contributor

OK, not depend in the Composer sense but I would say for the bugsnag-php library to reference any framework-specific code should be avoided. I don't want to cause a breaking change by removing it - I was anticipating fixing the include path and maybe adding a test of when Process class doesn't exist (do we even support a version of Laravel when this doesn't exist?)

@GrahamCampbell
Copy link
Contributor Author

I wouldn't say deleting this code is a breaking change. It's just the internals of the library. We were not getting the process class instance from the container, so users had no opportunity to mess with it.

@snmaynard
Copy link
Contributor

I would generally favor an approach that means we have bugsnag-php that includes plugins for all the php frameworks we want to support. This is how ruby/python work and it means we don't have dependency issues. If that is the way it could be built, I would be generally supportive of that - cc @kattrali

@tomlongridge
Copy link
Contributor

We've discussed this and agreed that there's no need for the Symfony code as there's no reason the native way of executing process calls wouldn't work. So we can merge this and close bugsnag/bugsnag-php#572.

@GrahamCampbell GrahamCampbell merged commit 14230ce into master Feb 25, 2020
@GrahamCampbell GrahamCampbell deleted the builder-fix branch February 25, 2020 13:06
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.

Deploy command
3 participants