maknz / slack

A simple PHP package for sending messages to Slack, with a focus on ease of use and elegant syntax.
BSD 2-Clause "Simplified" License
1.17k stars 204 forks source link

Added .env for portability #29

Closed sonnygauran closed 8 years ago

sonnygauran commented 9 years ago

Settings are now .env customizable.

SLACK_ENDPOINT=https://hooks.slack.com/services/
SLACK_CHANNEL=#general
SLACK_USERNAME=Slack-News
SLACK_ICON=
SLACK_LINK_NAMES=true
SLACK_UNFURL_LINKS=false
SLACK_UNFURL_MEDIA=true
SLACK_MARKDOWN=true

After adding settings to .env, you can now use the Slack facade directly.

Slack::send('🎉');
maknz commented 9 years ago

I feel this should be something implemented per-project if the user wants it. Environment variables aren't common in Laravel 4. Is there any literature you can point me to, or other Laravel projects that do this?

sonnygauran commented 9 years ago

I'm not familiar with Laravel 4, but these would now revert the existing config (to be compatible with Laravel 4) and then provide a separate config for Laravel 5 that makes use of .env for projects only needing 1 Slack client, which is provided by your Slack facade.

maknz commented 9 years ago

Cool, thanks. Do you know of any projects that currently take this approach with V5 config? It seems reasonable, but unless it's a very common paradigm across plenty of Laravel 5 packages, I still think this would be best implemented just for your projects. Sure, people can change them back to strings, but that might not be clear to novices.

sonnygauran commented 9 years ago

For multiple version support, other Laravel project versions are implemented as separate branches. In this case though, your approach is better because of a single composer require that's applicable for both 4 and 5 versions. I only started with Laravel 5 (not 4) a couple of months ago and as a novice on Laravel 5, configurations are usually loaded via env(), and not-commited to code at config/app.php. I added this simply because it makes use of the project seamlessly for 1 to 1 project and slack setups where I would just do the following:

  1. composer require
  2. add to service provider
  3. add settings to .env
  4. use Slack::send() anywhere in the app.

I wouldn't need to modify anything at all in the application afterwards. Other than that, nice work @maknz! :+1:

maknz commented 9 years ago

I was more meaning how many L5 projects publish their config file with environment variables already in there. I modelled off intervention/image for the config files, and they just had strings for the default/template config IIRC.

Happy to keep this open for more input on whether or not this is a good default; but in the meanwhile you shouldn't have any problem using the environment variables in your config/slack.php file?

sonnygauran commented 9 years ago

I'd prefer to leave the config strings empty too, but it was already populated from the start. I just added the .env wrapper for these settings. :smile:

I'm happy with your work and it works for me, and probably would speed up re-use of the project when these are placed. Because if I use Envoyer or envoy, I'd only need to setup .env variables once, and can re-deploy the project with the settings already placed neatly:

# with /var/www/laravel/.env already prepared on deployed server:
composer install;
# not only slack settings, but settings also for all other service providers
ln -s /var/www/laravel/.env /var/www/laravel/current/.env;
# done

Instead of commanding the deployer to do something like:

# Slack config customized and placed at /var/www/laravel/slack.config.php
composer install;
# For each service provider not using .env, copy over config
cp /var/www/laravel/slack.config.php /var/www/laravel/current/config/slack.php;

Cheers!

jessedc commented 9 years ago

+1 for this change, as it also suits modern Lumen projects that use dotenv almost exclusively for configuration.

maknz commented 9 years ago

Are arrays supported for .env? See the last config option which is an array.

sonnygauran commented 9 years ago

Arrays in .env are currently not supported, that's why I left the markdown_in_attachments config as-is.

commandantp commented 8 years ago

+1 for this change too. Just makes it faster to configure.

soullivaneuh commented 8 years ago

I feel this should be something implemented per-project if the user wants it.

Completely agree. This is a library, not a ready to use project.

I wrote a bundle integration for Symfony: https://github.com/nexylan/NexySlackBundle

In this case, .env is not needed. It should be per project IMHO.

Or, I'm missing something? :wink:

Gummibeer commented 8 years ago

I don't know where the problem is to add the .env entries by your own if you need them!? There is a config where you cand efine the values - by doing so you can use env() array_get() any ini or what ever you want to set the configs. We did it in all our projects and there was rly no problem with it - set some default values in the env() function and customize it in the .env files per instance if needed.

maknz commented 8 years ago

So there's a few reasons I'm not keen on merging this:

  1. It adds cognitive overhead to the config files. This library is all about simplicity, and I like the readability of the config files at the moment. A novice developer who might not yet be using ENV variables for config might get confused.
  2. This package is compatible with both Laravel 4 and 5. At the moment, I can keep the one config file which is nice.
  3. Arrays are not supported with env(), so you'll have to edit the config file anyway.
  4. If you're a developer using ENV variables, it would take you a trivial amount of time to change out the strings for env() calls to suit your project. Of course, we're not debating whether or not to use env() at all, we're talking about making it the default.

I'll keep this issue open for discussion as there might be a compelling argument yet to be heard, but that's my feeling at this stage.

soullivaneuh commented 8 years ago

This could maybe be added as documentation IMO.

Gummibeer commented 8 years ago

If this PR will not be merged - coul you @maknz reject it? Open PRs looks like open tasks there for the community. If the open task here is the hint in docu for custom env - open an issue!?