Support both D9 and D10 - BOTH versions of EncoderInterface::encode CAN be supported!

@joachim

Assuming that the links to Symfony source code in the issue creation help are correct for the versions, then this PHP code works:

The message may be a bit misleading, as it is possible, but I am not sure if the effort is worth it given that Composer will handle the upgrade and D9 has reached end-of-life. Maybe I should be thinking about that differently though?

Another way that we could do it is by creating a separate package with a trait, and on two separate branches (one tied to Symfony 4, one tied to Symfony 6), implement the methods related to that interface. In the module, we would then require the package with version constraints that allow either of the branches and then use the trait to pull in the code. TLDR: Composer could help us solve this. We achieved this on the mpdf package.

With regard to your example, are you suggesting that we reimplement the interfaces with unique names in the module code and then conform to those instead of to the upstream EncoderInterface?

> With regard to your example, are you suggesting that we reimplement the interfaces with unique names in the module code and then conform to those instead of to the upstream EncoderInterface?

No, I'm suggesting that the implementation in this module use the signature in the Child class in the sample code:

> public function encode($data, $format, array $context = []): string {

The interfaces in my code are just to demonstrate that this function is compatible with both the Symfony 4 and the Symfony 6 interfaces.

Upgrading from 9 to 10 with Composer is pretty unpleasant, and the general consensus is that it's a huge help if the following workflow can be followed:

1. update all contrib modules to the highest D9 version, which has dual support for 9/10
2. update core
3. update contrib modules which have further versions which support only D10

Having to do core and contrib in the same step is much more complicated.

No, I'm suggesting that the implementation in this module use the signature in the Child class in the sample code:

I understand how that would work in your example since the interfaces have different names. In Symfony, the interface names are the same, but the signature has changed across branches/releases. It's not clear to me how that would work. I'd be happy to review an MR if you'd like to mock it up.

That was to show that both interfaces can be supported without requiring changing the code in the example.

What it shows is that you can have:

// Symfony 4
class Child implements Symfony {
  public function encode($data, $format, array $context = []): string {

and:

// Symfony 6
class Child implements Symfony {
  public function encode($data, $format, array $context = []): string {

-- same method signature, and both will work.

In other words, a release of this module could have that method signature and support for Symfony 4 and 6, and therefore both D9 and 10.

This class works on both Symfony 4 and 6:

<?php

namespace Joachim\CsvSerializationTest;

use Symfony\Component\Serializer\Encoder\EncoderInterface;

class Encoder implements EncoderInterface {

  public function encode($data, $format, array $context = []): string {
    return 'cake';
  }

  public function supportsEncoding($format): bool {
    return TRUE;
  }

}

I believe what you are describing is what we had in our 3.0.0-beta 1 release which was marked as having Drupal 9 support. It did not work for folks on Drupal 9.

3.0.0-beta1 has this:

  public function encode($data, string $format, array $context = []):string {

which indeed won't work on D9, because Symfony 4 has:

  public function encode($data, $format, array $context = []);

You can't make an implementation be more restrictive in what it accepts. So if the interface has no typehint on a parameter, you can't add it, because the interface is saying 'you can pass anything you like for $format' and the 3.0.0-beta1 implementation is saying '$format must be a string'. That is breaking the contract.

However, you CAN make an implementation less restrictive, so where Symfony 6 says '$format must be a string', my version says '$format can be anything'. That's ok on both Symfony 4 and 6.

Conversely, the return type IS needed, because you can't make an implementation be less restrictive in what it returns. Symfony 4 says 'encode() returns a string', and so we can't say 'encode() can return anything', because that's breaking the contract too.

Therefore, what works for both is this:

  public function encode($data, $format, array $context = []): string {

No parameter type, but a return type.

Ahh, I see. I was overly focused on the return type and missing the difference in the parameter type. My apologies. I will create an MR. If there is interest in testing it on D9 and it works, I don't see a reason not to commit it.

I wish I had learned this before we dealt with 🐛 Drupal 9 users being updated to 3.0.0-beta1 incorrectly Fixed !

I wonder whether it is better to add support for D9 on the 4.x branch or add support for D10 on the 3.x branch.

My initial thought would be the former, but given the D9 has already reached end-of-life, then maybe we make the changes on the 3.x branch to provide initial D10 support and ease people's upgrades. That way we don't add D9 support on 4.x and then need to cut another new major version when we drop it in the future.

> If there is interest in testing it on D9 and it works, I don't see a reason not to commit it.

I'll test on Monday when I'm back on the D9 project that uses this.

> I wonder whether it is better to add support for D9 on the 4.x branch or add support for D10 on the 3.x branch.

I was thinking make a 3.0.2 which is D9 & D10. That way D9 users upgrade to that first, then once they've upgraded to D10, they upgrade to the 4.x branch.

I was thinking make a 3.0.2 which is D9 & D10. That way D9 users upgrade to that first, then once they've upgraded to D10, they upgrade to the 4.x branch.

OK. We are on the same page.

I've pushed a fix to the 3405001-support-d10-3x branch. (Sorry I might have accidentally made a new branch as I hadn't refreshed the page and hit the 'create fork' button but there already was one!)

I've installed the module from that branch into both a 9.5 site and a 10.1 site and I can confirm I see CSV output from a view in both sites.

This is looking good, but I would like to be very confident before we merge this. I am nervous about shipping a version that declares D10 support and then having to roll that back. I don't want to relive 🐛 Drupal 9 users being updated to 3.0.0-beta1 incorrectly Fixed 🤦

TLDR: More testing welcome!

What other pathways need to be tested apart from a CSV export with Views Data Export?

Can this not be merged but wihtout a release so the dev can be installed easiliy via composer for a D9 -> D10 upgrade (which is my case) but people would not be updated unexpectadly?
Maybe I am missing something?

Patch for 3.0.2

@2pha: I am not sure what scenario you are concerned about. Could you be more specific?

@markdorison I was referring to #17.
Merging but not adding a release tag should stop peple unexpectadly being updated.

@2pha but to what end? Are you suggesting that for further testing? Is that testing not possible before we merge?

@markdorison basically, just to keep the upgrade_statue module happy when going from D9 to D10.

@2pha Got it. The biggest benefit that I see in committing this change would be to ease that transition by providing a release that will run on both major version. I am less concerned about the Upgrade Status module (in this case); if it was just that I would be inclined to leave things as is with 3.x for D9 and 4.x for D10.

I was able to apply the patch #20 with no issues, I'll test the functionality