8000 [Translation] fix support for nikic/php-parser 5.0 by xabbuh · Pull Request #53453 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Translation] fix support for nikic/php-parser 5.0 #53453

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jan 8, 2024
Merged

[Translation] fix support for nikic/php-parser 5.0 #53453

merged 1 commit into from
Jan 8, 2024

Conversation

xabbuh
Copy link
Member
@xabbuh xabbuh commented Jan 7, 2024 •
edited by nicolas-grekas
Loading
Q A
Branch? 6.3
Bug fix? yes
New feature? no
Deprecations? no
Issues
License MIT

@xabbuh xabbuh requested a review from welcoMattic as a code owner January 7, 2024 20:32
@carsonbot carsonbot added this to the 7.1 milestone Jan 7, 2024
@nicolas-grekas
Copy link
Member

What about merging this on 5.4? This would reduce the pressure on deps constraints.

@xabbuh
Copy link
Member Author
xabbuh commented Jan 8, 2024

Not sure, this is not a bugfix to me.

@nicolas-grekas
Copy link
Member

We also merge deps-related changed to 5.4: support for latest Doctrine, latest PHP, etc. This is similar to me. This makes Symfony a low contributor to deps-hell (aka makes upgrading easier)

@GromNaN
Copy link
Member
GromNaN commented Jan 8, 2024

If it's compatible with 5.4, I don't see any reason to not allowing this new version.

@xabbuh xabbuh changed the base branch from 7.1 to 6.3 January 8, 2024 08:56
@xabbuh
Copy link
Member Author
xabbuh commented Jan 8, 2024

fair enough, rebased on 6.3 as the PhpAstExtractor class was introduced in 6.2

@nicolas-grekas nicolas-grekas modified the milestones: 7.1, 6.3 Jan 8, 2024
@xabbuh xabbuh mentioned this pull request Jan 8, 2024
Closed
@wouterj
Copy link
Member
wouterj commented Jan 8, 2024

Note that currently, we explicitly say that this is not allowed as a bug fix in the maintenance docs:

[...] the following changes are never accepted in a patch version:

  • Support for newer major versions of Composer dependencies: Taking into account support for newer versions of an existing dependency is not acceptable.

@nicolas-grekas
Copy link
Member
nicolas-grekas commented Jan 8, 2024 •
edited
Loading

@wouterj we have some contradiction on the topic, because we also have this "seamless deps" rule that makes us fix compat with Doctrine / PHP / Windows / infra-level stuff on lower branches, the reason being that wider deps ranges makes it easier to upgrade for end users.
Here, we fall in this categry to me, especially because we don't set the version boundaries. But also because php-parser is a widely used deps and making Symfony restrict which versions are allowed is contributing to deps hell.

@wouterj
Copy link
Member
wouterj commented Jan 8, 2024

Yes, "Newer versions of PHP" and "Newer versions of popular OSes" are mentioned in the "allowed on a case by case basis" section :)

Btw, not saying I'm against this change, especially as we don't set version ranges explicitly for this optional dep. But I think we should then update our policy on this (and find the reason why we set the current policy like this, is there something non-obvious that bit us in the past and we shouldn't forget now?)

@xabbuh xabbuh force-pushed the phpparser5 branch 3 times, most recently from 48ccd5e to 3025d5a Compare January 8, 2024 12:51
@nicolas-grekas nicolas-grekas changed the title [Translation] add support for nikic/php-parser 5.0 [Translation] fix support for nikic/php-parser 5.0 Jan 8, 2024
@nicolas-grekas nicolas-grekas added Bug and removed Feature labels Jan 8, 2024
@xabbuh
Copy link
Member Author
xabbuh commented Jan 8, 2024

tests pass, ready to be reviewed

@nicolas-grekas
Copy link
Member

Thank you @xabbuh.

@nicolas-grekas nicolas-grekas merged commit 512f8c4 into symfony:6.3 Jan 8, 2024
@xabbuh xabbuh deleted the phpparser5 branch January 8, 2024 13:04
@xabbuh xabbuh mentioned this pull request Jan 9, 2024
Merged
nicolas-grekas added a commit that referenced this pull request Jan 11, 2024
@nicolas-grekas
minor #53475 [Translation] simplify the parser factory creation (xabbuh)
aa232c4 
This PR was merged into the 6.3 branch.

Discussion
----------

[Translation] simplify the parser factory creation

| Q             | A
| ------------- | ---
| Branch?       | 6.3
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Issues        | continuation of #53453
| License       | MIT

Commits
-------

509ca9b simplify the parser factory creation
This was referenced Jan 30, 2024
Merged
Merged
Merged
@xabbuh xabbuh mentioned this pull request Nov 9, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas left review comments

@stof stof stof left review comments

@alexandre-daubois alexandre-daubois alexandre-daubois left review comments

@chalasr chalasr chalasr approved these changes

@welcoMattic welcoMattic Awaiting requested review from welcoMattic

Assignees
No one assigned
Labels
Bug Status: Reviewed Translation
Projects
None yet
Milestone
6.3
Development

Successfully merging this pull request may close these issues.
8 participants
@xabbuh @nicolas-grekas @GromNaN @wouterj @stof @alexandre-daubois @chalasr @carsonbot
0