Skip to content

Conversation

joelwurtz
Copy link
Member

Actually don't know if this "normalizer" should be in the plugins library, WDYT ?

Copy link
Contributor

Choose a reason for hiding this comment

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

so the promise is handling the throwing?

Copy link
Contributor

Choose a reason for hiding this comment

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

by catching the exception, i mean.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes when calling one of the callable passed to the then method, if it returns something then the value will go into the next onFulfilled callable, if it throw an exception it will go into the next onRejected callable.

@dbu
Copy link
Contributor

dbu commented Nov 13, 2015

i think it would make sense to have an own thing for that. did you know https://github.com/namshi/cuzzle ? something like this for PSR-7 would be really cool. i would create a RequestFormatter and ResponseFormatter interface in the utils and implement it with a StringFormatter for now. and later we can do the curl plugin. the formatters would have just one method format, keeping things interchangeable. normalizeToString and the like are tighter coupling.
the array conversion is a special case, we might want that simply as a method in the logger plugin even.

@sagikazarmark
Copy link
Member

Do we really need this? I mean, the nornalizer could work (probably in the utils repo), but in case of Monolog for example, we could have a Monolog Formatter.

The point is that it is not really the plugin's responsibility IMHO.

Haven't know cuzzle before, but it's great.

@dbu
Copy link
Contributor

dbu commented Nov 13, 2015

yeah, i really love the idea of formatting a log so you can simply paste it on the cli to see what that request returned.

@dbu
Copy link
Contributor

dbu commented Nov 13, 2015

yeah, formatting belongs into utils. except the monolog (or maybe psr log?) normalizing, because that is specific to the task of logging. i think a formatter should return only string, not mixed or string|array, it would make little sense. we can also have an array normalizer in utils if we think there are other use cases, but i would not implement the Formatter interface with that.

@sagikazarmark
Copy link
Member

Actually we could even format it to plain HTTP, so that we can directly send it with local testing clients.

I think request/response formatter interfaces should go into a separate package. With some basic implementations. Also, formatters should always format to string I think.

@sagikazarmark
Copy link
Member

The logger plugin should probably not require the formatter to be injected. The underlying PSR3 implementation should provide a way to add custom formatters. Not sure if it is possible though.

@dbu
Copy link
Contributor

dbu commented Nov 13, 2015 via email

@sagikazarmark
Copy link
Member

its a psr 7 utility to format requests and responses as strings.

I have a feeling that this utils repo will eventually get a bit fat. Also, I imagine the utils repo as a collection of utilities for httplug and clients. Putting too much stuff in it is not a good idea IMO. We could have a psr7-utils repo for strictly PSR-7 utilities, without Httplug and other client related stuff in it.

@dbu
Copy link
Contributor

dbu commented Nov 13, 2015 via email

@sagikazarmark
Copy link
Member

having several X-utils repositories sounds confusing too.

psr7-utils, httplug-utils....I can't imagine more utils for now.

i am more afraid of too many repositories than of that thing becoming too fat

Thinking about this: Maybe a psr7-utils package would be good, because there can be several cases when we just want one small psr7 thing to be somewhere. So a common psr7-utils repo could mitigate this problem.

oh, or does something like that already exist?

Don't think so, at least I don't know about it.

@dbu
Copy link
Contributor

dbu commented Nov 13, 2015 via email

@joelwurtz
Copy link
Member Author

The Formatter thing in ivory http adapter was also used for that : https://github.com/egeloen/ivory-http-adapter/blob/master/src/Event/Formatter/Formatter.php

@joelwurtz
Copy link
Member Author

So to make it short on the changes :

  • Remove object to array transform and use the object directly in context
  • Keep string normalization for the moment (but make an issue to have something more deep, like cuzzle in the future)

Ok for everyone ? @dbu @sagikazarmark

@dbu
Copy link
Contributor

dbu commented Nov 16, 2015

agreed. but the Normalizer should have an @internal annotation and clearly state its not meant to be reused. i guess that is still better than moving those methods into the plugin class.

@sagikazarmark
Copy link
Member

👍

@joelwurtz joelwurtz force-pushed the feature/logger-plugin branch 3 times, most recently from 898c284 to 908f761 Compare November 16, 2015 21:52
Copy link
Member

Choose a reason for hiding this comment

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

I think this begs the question: logs...what?

@joelwurtz
Copy link
Member Author

I have moved normalizer to @internal should be good to merge.

sagikazarmark added a commit that referenced this pull request Nov 18, 2015
@sagikazarmark sagikazarmark merged commit 10d41a7 into master Nov 18, 2015
@sagikazarmark sagikazarmark deleted the feature/logger-plugin branch November 18, 2015 00:42
@sagikazarmark
Copy link
Member

Nice job @joelwurtz

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.

3 participants