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

Extend Slack Notification to work with Slack Apps for Laravel 11 support #1481

Merged
merged 7 commits into from
Jul 26, 2024

Conversation

travisricks
Copy link
Contributor

Reference: laravel/framework#51879

Laravel 11 supports Slack notifications with not only webhook urls but also using a Slack app, but this repository is configured only for webhook urls. Added a new condition to check if the supplied destination is a webhook url or channel id and return the appropriate response.

Screenshot-003236-2024-07-25at22 18 01@2x

@driesvints
Copy link
Member

We'll need to fix the static analysis tests before proceeding.

@driesvints driesvints marked this pull request as draft July 25, 2024 13:45
@travisricks
Copy link
Contributor Author

@driesvints
I (think) I fixed the php stan issues by using // @phpstan-ignore-line on line 114. I only did this because the file was already using that previously for a different type hint issue.

If I understand the issue correctly it's because the type-hinted class only exists in Laravel 11. I tried adding a safety check for that by adding class_exists() checks in the if statement.

But please let me know if this is not the correct solution or if other changes are necessary.

@travisricks travisricks marked this pull request as ready for review July 26, 2024 03:12
@taylorotwell taylorotwell merged commit 8830039 into laravel:5.x Jul 26, 2024
10 checks passed
@siarheipashkevich
Copy link

@travisricks what about updating the documentation for this ability?

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