[Discussion] Define a backwards compatibility promise

[Nyholm]

Is your feature request related to a problem? Please describe.

I started to use Temporal a few months ago and I really like it and this SDK. I think you (the maintainers and community) have done an excellent job and I am impressed by many things. The use of Generators in Scope::next() is genius.

I am doing a private Symfony bundle around this package and I've found it a little difficult to work with.

Issue 1

We break backwards compatibility between releases. Here are some examples, but there are many more.

2.9.0

• Temporal\Client\ScheduleClient::getHandle() changed signature.
• Temporal\Client\ServerCapabilities was removed in a way that breaks BC.
• Temporal\Client\WorkflowExecutionHistory was moved to Temporal\Client\Workflow\WorkflowExecutionHistory
• Temporal\Client\WorkflowClient have many methods changing their signatures.

2.10.0

• Temporal\Activity\ActivityCancellationType was converted to enum
• Temporal\Workflow\ChildWorkflowCancellationType was converted to enum
• Temporal\Activity\ActivityOptions::withCancellationType() has changed signature
• Temporal\Worker\ChildWorkflowCancellationType was removed in a way that breaks BC.

Issue 2

We have a bunch of classes in the internal namespace. I suggest all of them should be marked with @internal for better IDE support. But I also see parts of our public API returning internal classes. Example:

Temporal\Worker\Worker::getWorkflows(): RepositoryInterface

Describe the solution you'd like

I would like us, staring with the next minor release, to follow BC very strictly. I suggest to write down a BC promise what it means for us and what third party libraries and application authors can expect from us. Ie, we don't need to be super generous, but we need to be predictable.

Additional context

There is no secret that I am a big fan of Symfony's BC promise. In the best of all worlds, I think we should follow the same promise. Just write a statement in our readme that we follow the same practices.

There is also the Roave/BackwardCompatibilityCheck that will check for any BC breaks as part of the CI workflow.

[roxblnfk]

Hi.

Regarding Issue 1:

I checked your comments on BC between minor releases. Am I correct in understanding that they were obtained automatically using Roave/BackwardCompatibilityCheck?

2.9.0

• Temporal\Client\ScheduleClient::getHandle() changed signature.
  Final class, signature changed without breaking usage.
• Temporal\Client\ServerCapabilities was removed in a way that breaks BC.
  Moved to another location with an alias to the old class.
• Temporal\Client\WorkflowExecutionHistory was moved to Temporal\Client\Workflow\WorkflowExecutionHistory
  Alias to the old class is present.
• Temporal\Client\WorkflowClient has many methods changing their signatures.
  Signatures changed without breaking usage.
  ⚠️ Class is not final, should be made @final.

2.10.0

• Temporal\Activity\ActivityCancellationType was converted to enum
  Was a final class, constants not affected.
• Temporal\Workflow\ChildWorkflowCancellationType was converted to enum
  Was a final class, constants not affected.
• Temporal\Activity\ActivityOptions::withCancellationType() has changed signature
  Type extended, does not affect usage.
  ⚠️ Class is not final, should be made @final.
• Temporal\Worker\ChildWorkflowCancellationType was removed in a way that breaks BC.
  Made an alias, as it was a duplicate.

If we pay attention to the presence of aliases for removed classes or the previous work done with the marshaller, it becomes clear that we strive to adhere to semver and maintain backward compatibility where possible, without making various promises.

If we strictly follow Symfony's BC promise, I'm afraid the PHP SDK code might become legacy-laden, and we won't be able to provide functionality on par with other SDKs.
Moreover, there are things that may look like breaking changes but are not considered as such in the context of the SDK. This includes configuration classes and API gateway interfaces to Temporal, which may and will change their signatures. We expect that these interfaces will not be inherited or implemented by the user (such as ActivityOptions, WorkflowClientInterface, WorkflowClient, etc).
At the same time, interceptor interfaces are also supplemented with new functions, but they come with traits that must be used in user implementations.

Perhaps we should consider marking classes/interfaces as risky for extension/implementation.

[Nyholm]

I checked your comments on BC between minor releases. Am I correct in understanding that they were obtained automatically using Roave/BackwardCompatibilityCheck?

No, most of them were obtained by my Symfony Bundle's CI started to fail. Then I looked at the diff and found some more to give some examples in this issue.

---

Moved to another location with an alias to the old class.

I saw that it was an attempt, but it was incorrectly done.

Was a final class, constants not affected.

What if I have instantiated that class? Then it would break if the class was removed.

---

If we strictly follow Symfony's BC promise, I'm afraid the PHP SDK code might become legacy-laden, and we won't be able to provide functionality on par with other SDKs.

With all respect, I think you are wrong. There are always a way forward with keeping BC, but the "effort" might not be worth it. In such cases, write a good UPGRADE.md and tag a new major release. We wont run out of integers. =)

Perhaps we should consider marking classes/interfaces as risky for extension/implementation.

Yes, I believe so. We want to be helpful to all users of this SDK, that means we must be predictable. If I use an @internal class or extend a class marked with @final, then I only have myself to blame.

If we dont want to adjust how we do code changes, then this issue can be solved with documentation/comments only. But maybe some adjustments are needed.

Suggestion: We add the BC promise CI job, that will make (too many) comments on each PR. These can then be discussed and fixed/ignored.

[roxblnfk]

I saw that it was an attempt, but it was incorrectly done.

What do you mean? May be it should be fixed?

What if I have instantiated that class? Then it would break if the class was removed.

In the case of a DTO or another working class, this would make sense, but in the case of an enum class, it looks like this:
😃

write a good UPGRADE.md and tag a new major release. We wont run out of integers. =)

If it were that simple, the SDK would already be at version 4.x. 🤔

Suggestion: We add the BC promise CI job, that will make (too many) comments on each PR. These can then be discussed and fixed/ignored.

Sounds good 👍 Would you like to take this on? I'll return to working on the SDK next week.

[Nyholm]

What do you mean? May be it should be fixed?

If we want to move a class. we need to make sure the old class is still working. That means as extend, instantiate, return values etc.
I can make a PR and we can discuss the details there.

In the case of a DTO or another working class,

Haha, yes. You are correct, but it is still a BC break.

If it were that simple, the SDK would already be at version 4.x. 🤔

That is fine. There is no harm in that. The most important thing is to be predictable and it should always work for all weird use cases. There is no gold medal if you keep version 2.x for 10 years.

Would you like to take this on?

Sure, I would be happy to.

[root-aza]

I am doing a private Symfony bundle around this package and I've found it a little difficult to work with.

@Nyholm hi. Why weren't you happy with this decision?

https://github.com/VantaFinance/temporal-bundle

[Nyholm]

That looks like a nice bundle.

We (the company I work for) started our own Symfony bundle back in 2021 and we are doing a lot of custom stuff that does not make sense to open source.

If we were just starting our Temporal journey we should definitely consider that bundle.