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

Created on 29 November 2023, about 1 year ago
Updated 14 March 2024, 9 months ago

Problem/Motivation

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

<?php


interface Symfony4 {
  // From https://github.com/symfony/symfony/blob/4.4/src/Symfony/Component/Serializer/Encoder/EncoderInterface.php
  public function encode($data, $format, array $context = []);
}

interface Symfony5 {
  // From https://github.com/symfony/symfony/blob/6.2/src/Symfony/Component/Serializer/Encoder/EncoderInterface.php
  public function encode(mixed $data, string $format, array $context = []): string;
}


class Child implements Symfony4, Symfony5 {
  // Implements both!
  public function encode($data, $format, array $context = []): string {
    return 'return';
  }

}

Or is there another factor I've missed?

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

πŸ“Œ Task
Status

Needs review

Version

3.0

Component

Code

Created by

πŸ‡¬πŸ‡§United Kingdom joachim

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @joachim
  • πŸ‡¬πŸ‡§United Kingdom joachim
  • πŸ‡ΊπŸ‡ΈUnited States markdorison

    @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?

  • πŸ‡¬πŸ‡§United Kingdom joachim

    > 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.

  • πŸ‡ΊπŸ‡ΈUnited States markdorison

    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.

  • πŸ‡¬πŸ‡§United Kingdom joachim

    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.

  • πŸ‡¬πŸ‡§United Kingdom joachim

    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;
      }
    
    }
    
  • πŸ‡ΊπŸ‡ΈUnited States markdorison

    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.

  • πŸ‡¬πŸ‡§United Kingdom joachim

    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.

  • πŸ‡ΊπŸ‡ΈUnited States markdorison

    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 !

  • πŸ‡ΊπŸ‡ΈUnited States markdorison

    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.

  • πŸ‡¬πŸ‡§United Kingdom joachim

    > 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.

  • Open in Jenkins β†’ Open on Drupal.org β†’
    Core: 10.1.4 + Environment: PHP 8.1 & MySQL 8
    last update about 1 year ago
    1 fail
  • Status changed to Needs work about 1 year ago
  • πŸ‡ΊπŸ‡ΈUnited States markdorison

    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.

  • πŸ‡¬πŸ‡§United Kingdom joachim

    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.

  • Status changed to Needs review about 1 year ago
  • πŸ‡¬πŸ‡§United Kingdom joachim
  • πŸ‡ΊπŸ‡ΈUnited States markdorison

    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!

  • πŸ‡¬πŸ‡§United Kingdom joachim

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

  • πŸ‡¦πŸ‡ΊAustralia 2pha

    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?

  • πŸ‡¨πŸ‡¦Canada mmaranao Calgary, AB

    Patch for 3.0.2

  • πŸ‡ΊπŸ‡ΈUnited States markdorison

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

  • πŸ‡ΊπŸ‡ΈUnited States markdorison
  • πŸ‡¦πŸ‡ΊAustralia 2pha

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

  • πŸ‡ΊπŸ‡ΈUnited States markdorison

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

  • πŸ‡¦πŸ‡ΊAustralia 2pha

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

  • πŸ‡ΊπŸ‡ΈUnited States markdorison

    @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.

  • πŸ‡¨πŸ‡¦Canada danrod Ottawa

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

Production build 0.71.5 2024