Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Symfony 7 support #766

Closed
nunomaduro opened this issue Oct 12, 2023 · 26 comments
Closed

Symfony 7 support #766

nunomaduro opened this issue Oct 12, 2023 · 26 comments

Comments

@nunomaduro
Copy link
Contributor

nunomaduro commented Oct 12, 2023

Hi! It's Nuno here from the Laravel team. I was trying to upgrade your package to support Symfony 7, but it seems that, as it is deeply coupled with Symfony Console, and Symfony Console added types everywhere, it's pretty much impossible to do this upgrade without:

  • Requiring PHP 7.4.
  • Dropping Symfony versions (e.g 3.x, and 4.x)
  • Making breaking changes in protected properties and protected methods (return types, etc.).

Do you have any particular opinion / or plan on this?

@bobthecow
Copy link
Owner

33b7335 👀

I haven't looked at the changes required for Symfony 7 changes yet, but will already be requiring PHP 7.4+ for the next major version (do you call it that for v0.x in semver?).

I don't have a strong opinion about dropping support for older Symfony versions, but in general prefer to keep support for as many as we can for as long as possible.

Similarly, even on a backwards compatibility breaking release, I'd like to keep protected properties and methods as compatible as we can for as long as possible. Specifically, I try to deprecate but still support extension points after an update like this, and remove support on the next backwards compatibility breaking release.

Let's do it, keeping those principles in mind?

@nunomaduro
Copy link
Contributor Author

Sure. I will prepare a pull request, with those principles in mind, after finishing my current task. Thanks!

@nunomaduro
Copy link
Contributor Author

@bobthecow There is a few dozens of cases where that's literally impossible to support both Symfony 6 and Symfony 7 at the same time. E.g:

-    public function enterHash(Cursor $cursor, $type, $class, $hasChild)
+    public function enterHash(Cursor $cursor, int $type, string|int|null $class, bool $hasChild): void
    {
        if (Cursor::HASH_INDEXED === $type || Cursor::HASH_ASSOC === $type) {
            $class = 0;
        }
        parent::enterHash($cursor, $type, $class, $hasChild);
    }

The code above (like many other situations) when typed will be broken on Symfony 6. However, when not typed, I will be broken on Symfony 7. There is some ways to get around this, like conditional require traits/classes depending on the symfony version, but it requires some sort of copy/pasting between files.

Before explore this path, can you consider only supporting Symfony 7?

@bobthecow
Copy link
Owner

bobthecow commented Oct 13, 2023

I was worried about this, and I don't love that they've forced us into this choice. I think the minimum we could reasonably support for the next release is Symfony 6 and 7. That doesn't leave us with any great options :(

The one thing I've been considering—but haven't had a huge reason to do up until now—is a refactor to limit our direct exposure to symfony/console and its internals.

@nunomaduro
Copy link
Contributor Author

a refactor to limit our direct exposure to symfony/console and its internals.

Right! Unfortunately - my knowledge of this codebase is very limited, so it would have to be something probably done in our side when you find time.

Alternatively, could you, consider only support the latest version of this package? For example:

v0.11.x: Bug fixes only. (it's the current version, Symfony 6, 5, 4, 3, etc)
v0.12.x: Bug fixes only, and new features. (Next version, it's the current code on main, and it's for Symfony 7, PHP 8.2, etc).

@bobthecow
Copy link
Owner

I don't think it's acceptable to limit new features to people who adopt a version of a framework that hasn't even been released yet, and not the version which has long-term support through November 2027—or even the older version of that framework which has long-term support through November 2025.

If we end up needing to go this route, it'd be a commitment to fully support v0.11.x and v0.12.x, including new features, for at least that long.

My worry, though, is that the next version of Symfony would bring even more incompatibilities, making it basically impossible for us to support all their LTS versions. Refactoring to limit that exposure seems like the pragmatic choice.

@nunomaduro
Copy link
Contributor Author

Alright! I won't be able to help much, except ensuring that works as except at Laravel, etc!

@GrahamCampbell
Copy link
Contributor

GrahamCampbell commented Oct 13, 2023

Technically, it's not permitted to add new features in patch releases, if we wanna still follow semver, so we may need to have 0.x as legacy, and 1.x as symfony 7. ;)

@driesvints
Copy link

@GrahamCampbell there are no patch releases in 0.x versioning.

@nunomaduro
Copy link
Contributor Author

nunomaduro commented Oct 13, 2023

@GrahamCampbell When using 0.x.y, typically the x represents a major version.

@bobthecow
Copy link
Owner

Realistically we should be on 11.x rather than 0.11.x at this point, but ¯\_(ツ)_/¯

@bobthecow
Copy link
Owner

@nunomaduro This change actually looks good for Symfony 5-7 and PHP 7.4-8.3. Were you seeing other things break?

@nunomaduro
Copy link
Contributor Author

Did you tried that branch with Symfony 7 dependencies? Because if you run make test, you will see a all new range of missing types.

@bobthecow
Copy link
Owner

Screenshot 2023-10-13 at 12 42 48 PM

@bobthecow
Copy link
Owner

i added minimum-stability: dev to the config for that WIP branch, so it pulled in 7.0.x-dev for any PHP version it could on the @stable deps tests, e.g. https://github.com/bobthecow/psysh/actions/runs/6510758724/job/17685127387

@bobthecow
Copy link
Owner

The symfony7-support branch seems like it's good. I realized CI wasn't testing against PHP 8.4 so I added that, and everything is still passing.

@nunomaduro do you mind pulling that branch and seeing if it works for you?

@nunomaduro
Copy link
Contributor Author

On it.

@nunomaduro
Copy link
Contributor Author

All good. Tests passing on my machine. Also tested it with Laravel 11 (based on symfony 7). Let me know when is merged and tagged.

@bobthecow
Copy link
Owner

Thanks for checking!

It's been merged into main but I'm not sure what else I want to deprecate or clean up before cutting the new major version. At the very least I want to see if tightening up minimum dependency versions a bit makes sense.

When were you all planning on a stable release?

@bobthecow
Copy link
Owner

Since it's still possible for types to change before Symfony 7 lands—and create more of these incompatibilities—it probably makes sense to release the next PsySH version just after that.

@nunomaduro
Copy link
Contributor Author

nunomaduro commented Oct 13, 2023

Alright! We are using the branch alias for now. Thank you for your work on this.

Unclear for now when we are doing the stable release btw.

@bobthecow
Copy link
Owner

Great! Thanks for raising the issue, and your help testing it out.

@bobthecow
Copy link
Owner

Actually! If I can convince myself that 1782c48 isn't a backwards compatibility break, I can add Symfony 7 compatibility to v0.11.x … without dropping support for older PHP versions :)

@bobthecow
Copy link
Owner

Hmm. Never mind. That'd require a v0.12 to drop PHP 7.0 support, because we can't use void return types until 7.1. Probably not worth the hassle at that point.

@allan-simon
Copy link

hello symfony 7 has now been released , is there a plan for a v0.12 soon (my understanding was that in October symfony 7.0 was still in beta )

I don't mean to push you (I know what maintaining an opensource library like yours is), It's just to know if I wait or I can start considering using the main branch :)

@bobthecow
Copy link
Owner

bobthecow commented Dec 6, 2023

Dev stability / main branch is more or less stable and will be the next stable release. I was planning to wait for Symfony 7 and (probably) PHP-Parser 5.0 before the v0.12 release.

I'm checking on the PHP-Parser release. If it's not imminent, I'll drop PHP-Parser 5.0 support from composer.json and ship v0.12.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants