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

Add fluent setters for parameters in the slack handler #801

Merged

Conversation

EspadaV8
Copy link
Contributor

@EspadaV8 EspadaV8 commented May 31, 2016

The current slack handler has way too many constructor parameters and no clear order as to their position. This makes it confusing to work out what is actually trying to be set when constructing a new class, e.g,

$slack = new SlackHandler(
    'xoxp-0000000000-1111111111-22222222222-3333333333',
    '#general',
    'Monolog',
    false,
    null,
    Logger::ERROR,
    true,
    false,
    true
);

This PR makes things a bit easier by adding some fluent setters into the class.

$slack = new SlackHandler(
        'xoxp-0000000000-1111111111-22222222222-3333333333',
        '#general',
        'Monolog'
    )
    ->setErrorLevel(Logger::ERROR)
    ->useAttachment(false)
    ->includeContextAndExtra(true);

It would be nice to cut back the number of constructor parameters and reorder them to make a bit more sense

// Current
public function __construct(
    $token,
    $channel,
    $username = 'Monolog',
    $useAttachment = true,
    $iconEmoji = null,
    $level = Logger::CRITICAL,
    $bubble = true,
    $useShortAttachment = false,
    $includeContextAndExtra = false
)

// Proposed
public function __construct(
    $token,
    $channel,
    $username = 'Monolog',
    $level = Logger::CRITICAL,
    $bubble = true
)

Since that would be a fairly major breaking change though I've not included it in this PR. If it would be okay to do in another PR I'll open one up for it.

I've included scalar type hints for the methods because the file include the declare(strict_types=1); which is only valid for PHP7 so I assume that is okay. If not then I can easily remove them.

@Seldaek
Copy link
Owner

Seldaek commented May 31, 2016

Actually refactoring constructors is one of the few tasks left to do before 2.0 can be released. I am not sure if they should just all change to something like __construct($mandatoryArgs, $level, $optionsArray = []) because and move bubble in the options as it's not often useful. Or maybe have objects for configuration but that's also quite verbose. I am not sure what the best approach is, but right now yeah some handlers are absurdly long. Any better ideas would be appreciated :)

@EspadaV8
Copy link
Contributor Author

Does bubble even need to be in the constructor? Could it just be left as a setter in AbstractHandler? It already has a default of true, anything that needs to change it can already call ->setBubble(bool). Honestly, level isn't even really needed in the constructor either, again it defaults to Logger::DEBUG and has a setter in AbstractHandler too.

@Seldaek
Copy link
Owner

Seldaek commented May 31, 2016

Well technically we almost don't need anything in the constructor.. but it's a convenience IMO to be able to configure everything in constructor, and especially when dealing with dependency injection containers and such dealing with setters vs constructor argument causes extra hurdles/boilerplate.

@EspadaV8
Copy link
Contributor Author

A big problem with using array for arguments is that it's even harder to work out what are valid options. There's no other way except looking at the code/API docs to see what is allowed for each constructor then. At least with constructor parameters you can get hints from your IDE or just look at the method signature.

@xabbuh
Copy link
Contributor

xabbuh commented Jun 4, 2016

Imo having dedicated configuration classes would be the best solution. Constructors could be reduced to one argument and the use of classes would allow for autocompletion in IDEs as well as type hinting constructor arguments. Furthermore, configuration classes could optionally validate themselves.

@EspadaV8
Copy link
Contributor Author

EspadaV8 commented Jun 7, 2016

Until Monolog 2 is out is it possible to get this merged into the 1.x branch? Or are there no plans to for new v1 releases?

@Seldaek
Copy link
Owner

Seldaek commented Jun 7, 2016

I'd rather not as it then adds more cruft to be removed in 2.0 if we go with configuration classes. Although for non-essential settings (i.e. stuff that can be configured optionally) I don't know if a configuration class brings much over just having setters on the handler itself. Anyway I'd rather have a plan for 2.0 before adding things to 1.x, just so we can avoid introducing future BC breaks where not needed.

@glensc
Copy link
Contributor

glensc commented Jun 7, 2016

slightly offtopic, but will then 2.0 make such projects like monolog-cascade obsolete as it provides itself some kind of config support?

@Seldaek Seldaek added this to the 2.0 milestone Sep 18, 2016
@Seldaek Seldaek mentioned this pull request Sep 25, 2016
30 tasks
@Seldaek
Copy link
Owner

Seldaek commented Jul 5, 2018

Ok I'm gonna abandon the broader changes to all constructors as it doesn't seem worth the breakage. Would be great if this PR can be updated to the latest version of the SlackHandler with all current parameters and taking into account that some getters/setters already exist. If someone can do this I'm up for adding it either in 1.x branch or master for the upcoming 2.0

@EspadaV8 EspadaV8 force-pushed the add-fluent-setters-for-slack-handler branch 5 times, most recently from b9e6b87 to 2d75a3e Compare July 6, 2018 01:44
@EspadaV8 EspadaV8 force-pushed the add-fluent-setters-for-slack-handler branch from 2d75a3e to c9d15bb Compare July 6, 2018 01:47
@EspadaV8
Copy link
Contributor Author

EspadaV8 commented Jul 6, 2018

I think I've updated this correctly now. It's a bit longer because all of the calls need to be passed down to the SlackRecord class which also needs to have the setters created.

@Seldaek
Copy link
Owner

Seldaek commented Nov 4, 2018

@EspadaV8 Sorry but can you rebase on latest master? I'll try and get this merged quickly once that's done :s

@Seldaek Seldaek modified the milestones: 2.0, 2.x Nov 19, 2018
@Seldaek Seldaek merged commit c9d15bb into Seldaek:master Jul 6, 2019
@EspadaV8 EspadaV8 deleted the add-fluent-setters-for-slack-handler branch July 7, 2019 00:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants