Problem/Motivation

This module needs to be updated to Drupal 10 compatible. (It's a test dependency of Views Data Export among other things.)

Issue fork csv_serialization-3280951

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

jhedstrom created an issue. See original summary.

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
commented
Status: Active » Needs review
Related issues: +#3266389: Drupal 10 compatibility

jhedstrom opened merge request !8

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Needs work

Left some comments on the MR

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
commented
Status: Needs work » Needs review
berdir’s picture

berdir
Primary language German
Location Switzerland
commented

> I just removed the type hint rather than force php 8 for now...

That, sadly, means that it won't be D10 compatible. But with the moved D10 date, we can probably simply what until its ok-ish to rely on PHP8? official EOL of PHP 7.4 is next year though. Up to the maintainer to decide on that.

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
commented

Oh, I forgot about that lol. I'm fine either way.

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
commented

Actually, each module doesn't need to mark php version (so long as it isn't relying on those features specifically.) The mixin change is optional, so I think this could still technically be D10 compatible as-is?

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

I don't think it's optional, the Symfony 6 validator interface requires it and without it, it's a fatal error? I had the same problem in ERR, even tricks like two different classes with dynamic PHP/symfony version checks are impossible to pull off, php lint check breaks tests.

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
commented

Ah, I was thinking it was optional based on this deprecation notice on the latest 10.x:

  1x: Method "Symfony\Component\Serializer\Encoder\DecoderInterface::decode()" might add "mixed" as a native return type declaration in the future. Do the same in implementation "Drupal\csv_serialization\Encoder\CsvEncoder" now to avoid errors or add an explicit @return annotation to suppress this message.
    1x in CsvEncoderTest::setUp from Drupal\Tests\csv_serialization\Unit
berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Oh, that's probably a different interface then. Fine if that's a deprecation for Symfony 7/D11 then, agreed.

grasmash’s picture

grasmash commented

Since this drops Drupal 8 compatibility, we should create a new major version for it.

  • grasmash committed 0e2d158 on 8.x-2.x 
    Revert "Issue #3280951: Drupal 10 compatibility"

This reverts commit...
grasmash’s picture

grasmash commented
Status: Needs review » Fixed

Created 3.x branch and merged there.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

> Since this drops Drupal 8 compatibility, we should create a new major version for it.

Upping the min version does not require a new major version per se, that is allowed with semver, as long as your own API doesn't change.

The return types are somewhat of an edge case though, you could say that these plugins are an API of this module and can be subclassed, so it would be an API change (core explicitly excludes plugins classes in the BC policy though).

Your call as maintainer though, personally I try to avoid new major versions as much as possible due to the extra amount of work both for you as maintainer, users and maintainers of modules depending on it. That said, if you go with that, we could make 3.x either require PHP 8 or D10 and do the mixed return type as well so it doesn't have to change again.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

juc1’s picture

juc1 commented

hi is there any news on a D10 version?

m.stenta’s picture

m.stenta
he/him
Primary language English
commented

We are also interested in a Drupal 10 compatible release for the farmOS distribution, and this module is one of the few dependencies we are waiting on: #3330490: Update Drupal core to 10.x in farmOS

m.stenta’s picture

m.stenta
he/him
Primary language English
commented

Oh wait - the 3.x branch does not currently work, because of this line in composer.json: https://git.drupalcode.org/project/csv_serialization/-/blob/3.x/composer...

"drupal/core": "^8 || ^9",

Composer will not install it:

- drupal/csv_serialization dev-3.x requires drupal/core ^8 || ^9 -> satisfiable by drupal/core[8.0.0-beta6, ..., 8.9.x-dev, 9.0.0-alpha1, ..., 9.5.x-dev].

Do we need a followup issue for this? Or perhaps there is one already?

m.stenta’s picture

m.stenta
he/him
Primary language English
commented
Related issues: +#3294354: 3.x composer.json was not updated to require D9/10

Ah disregard... I just found #3294354: 3.x composer.json was not updated to require D9/10.

Linking it here for future explorers. :-)