[mmoreram]

• Can be used to create unique context instances related with a specific
  Request

[jcchavezs]

Thanks for this @mmoreram, a couple of comments:

0. I think values should be any type, not only string.
1. I think this object should only have the withValue method. I don't think there is a need for the possibility to delete, clear or list elements from this object. The less we include in the API surface, the easier to maintain and evolve.
2. I think this object should be immutable otherwise we can have weird coupling between upper layers and lower layers. That is, on every time we add a new value we produce a new object that we pass down the layers.
3. Not sure this is possible but I like that golang forces a custom type for the key and hence you can declare private types so only the members of the package can access those values. In PHP there is not possibility of having private classes but maybe what we can do is to change the signature to withValue($class, $key, $value) and use it as withValue(MyClass::class, $key, $value) and getValue(MyClass::class, $key, $value) making it explicit that the context is not a way to throw data into a bucket that everyone can consume but that as a user you are aware of the class creating the value. This also helps for avoiding name clashing.

Final thought, since this object can be passed down through layers, does it make sense to decouple it from http? Could this be somewhere else like in driftphp-core?

[mmoreram]

@jcchavezs Thanks so much for this review.

0. What kind of non-string value do you think we could save here?
1. Agree on removing delete() and clear(), but we should have a all() in order to have them all and iterate them, right? Let's say for debugging.
2. Ok on immutable object.
3. I don't understand this last point. Each value inserted by someone should be registered with the caller? Said that, I don't understand neither the method getValue(MyClass::class, $key, $value). Shouldn't be getValue(MyClass::class, $key) : $value ?

About the package, yes, you might be right.
I'm not a defender of a package named whatever/core, because basically, this package will be part of the domain as well. A core of a Framework should never be part of your domain. Same for placing the class into this package. You're completely right.

Maybe a small package for that would be enough, or even using an existing one.

[jcchavezs]

Hi thanks for the answer.

What kind of non-string value do you think we could save here?

It can be a number or an array. I think restricting to strings will cause unnecessary casting on write and read.

Agree on removing delete() and clear(), but we should have a all() in order to have them all and iterate them, right? Let's say for debugging.

This is the thing, if you accept arrays you just retrieve the array and no need to iterate. In my opinion it is easier to allow arrays rather than having to iterate and consider that if one wants to update that list then there is no way to remove entries.

I don't understand this last point. Each value inserted by someone should be registered with the caller? Said that, I don't understand neither the method getValue(MyClass::class, $key, $value). Shouldn't be getValue(MyClass::class, $key) : $value ?

Totally, I meant withValue(MyClass:class, $key, $value) and getValue(MyClass::class, $key): $value

Maybe a small package for that would be enough, or even using an existing one.

Long time ago I created a package for prototyping: https://github.com/jcchavezs/tracing-context-php. Many things changed both in my understanding of the problem and the use cases itself. I think we could come up with a new package and evolve it independently?

[seregazhuk]

@mmoreram Just a small comment. 😉
The return type here will fail when you create an object like this: $context = new Context();. For example in the constructor of the RequestWithContext class. Being created this way property $values contains null.

[mmoreram]

Touché!
Thanks for that!
This class needs and deserves Unit Testing.

[mmoreram]

@jcchavezs I'm OK with everything, but with the all() method.

What will you use this class for in the future?
Can you give me an example of an usage of the package once you have filled it with several entries?
For example, what if you want to pass this context across HTTP services? You will have to pass the whole object in the body or as a number of headers. How would you do that without an all() method?

I will create a Context package under DriftPHP for that purpose.

[mmoreram]

Closing this PR as is completely outdated. We can reopen it if needed.