Extract functionality of AbstractSpan to ensure separation of concerns.

[tidal]

At the moment AbstractSpan doesn't really have a separation of concerns as it is doing too many different things. Different responsibilities of the class should be extracted and the class be replaced with a BC Layer.

• Move methods activate and storeInContext in a trait (eg.: "SpanTrait) which concreate Span implementation will use.
• Move the lookup methods in a "Manager" class.
• Move factory methods into a Factory class (or the Manager)
• Create a BC Layer class for AbstractSpan
• Retire AbstractSpan

[bobstrecansky]

@Grunet mentioned he wanted to work on this issue

[Grunet]

Yeah let me see if I can do this while tackling #703, since that seemed like they'd depend on the 2nd and 3rd bullet point

Otherwise I'll try it out on a separate PR afterwards

[Grunet]

Yeah looking at it more closely it seems like it will probably deserve its own PR. I can still plan to give it a try though

[Grunet]

@tidal it looks like what's currently living in AbstractSpan at the moment almost exactly mirrors what's in Java's version (this PR and this other PR seem to have some discussion on why they took this approach), that @brettmc and @Nevay pointed out in the discussion on #720.

Do you think it's worth keeping it as-is to stay consistent with Java? Even if it does violate separation of concerns.

Also as @Nevay pointed out the 1 discrepancy seems to be the "activate" method PHP has but Java doesn't, and that removing that and renaming the "activate" method on Context to "makeCurrent" (like Java has) seems like it would make them match up 1-for-1 (well, minus the static methods being on the abstract class rather than on SpanInterface itself like Java can do). So we could try doing that here too to maximize similarity in the surface.

I don't have a great enough sense for how powerful consistency is here, so I don't really have strong feelings either way.

[tidal]

@Grunet Sorry, I thought I already answered you, but I must have forgotten to hit the submit button. However I don't want to repeat that wall-of-text, so I make it short.

@tidal it looks like what's currently living in AbstractSpan at the moment almost exactly mirrors what's in Java's version (this PR and this other PR seem to have some discussion on why they took this approach), that @brettmc and @Nevay pointed out in the discussion on #720.

Do you think it's worth keeping it as-is to stay consistent with Java? Even if it does violate separation of concerns.

Absolutely not!!! First of all staying consistent with anything Java is rarely a good idea, very often the opposite. (Been there done that.) I don't see any benefit in copying questionable design choices (like for example adding "default" implementations to interfaces as in the example). Languages should focus on their strength, which is rarely the case when copying stuff verbatim.
We'd rather learn from the mistakes of other languages' implementations, than copying them

Also as @Nevay pointed out the 1 discrepancy seems to be the "activate" method PHP has but Java doesn't, and that removing that and renaming the "activate" method on Context to "makeCurrent" (like Java has) seems like it would make them match up 1-for-1 (well, minus the static methods being on the abstract class rather than on SpanInterface itself like Java can do). So we could try doing that here too to maximize similarity in the surface.

A method name "makeCurrent" is even less intuitive and less meaningful than "activate", especially because of the English language's ambiguity where current is an adjective and a noun with completely different meanings. Since this issue is about AbstractSpan/Spans which are user facing, the method name should clearly commuicate what it does, so smth like "makeRoot", etc.would probably be easier to understand by users.

I don't have a great enough sense for how powerful consistency is here, so I don't really have strong feelings either way.

Consistency with the spec is relevant, consistency with other languages' implementations is not.

[Grunet]

Yeah that all seems reasonable to me. @brettmc @Nevay does @tidal's reasoning above seem appropriate to you to deviate from the Java implementation's patterns?

If so, I can go ahead and work on drafting changes to do the separating of concerns @tidal outlined in the issue description

[brettmc]

ok with me. I do agree that makeCurrent is not an intuitive method name, I personally prefer keeping activate FWIW.

[Nevay]

ok with me too. I have no issues with keeping ::activate() (I mentioned/like ::makeCurrent() because its name is a direct reference to ::current()).

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[stale[bot]]

This issue has been automatically closed because it has not had recent activity, but it can be reopened. Thank you for your contributions.