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

[Yaml]  Inline::parse(null) not allowed #52334

Merged
merged 1 commit into from
Oct 28, 2023
Merged

Conversation

GromNaN
Copy link
Member

@GromNaN GromNaN commented Oct 27, 2023

Q A
Branch? 6.4
Bug fix? no
New feature? no
Deprecations? no
Issues Fix #52326
License MIT

It's only called with string values in the Yaml component.

This class is internal. It allows to change the arg type.
The arg type was documented as string.
7b1715b#diff-1d97fcfb29e99f0200c3952e9a1a11c4b9f825c0f5ad4d4515b9b1805f8c988eR47

@GromNaN GromNaN added the Yaml label Oct 27, 2023
@GromNaN GromNaN requested a review from xabbuh as a code owner October 27, 2023 19:15
@carsonbot carsonbot changed the title [Yaml] Inline::parse(null) not allowed [Yaml]  Inline::parse(null) not allowed Oct 27, 2023
@carsonbot carsonbot added this to the 6.4 milestone Oct 27, 2023
@xabbuh
Copy link
Member

xabbuh commented Oct 28, 2023

For 5.4?

@GromNaN
Copy link
Member Author

GromNaN commented Oct 28, 2023

For 5.4?

Even if the class is internal, people may have used it directly. I don't want to break things in a bugfix version. This is not a bug fix. This is only a better error handling for PHP 8.3+

@xabbuh
Copy link
Member

xabbuh commented Oct 28, 2023

Fine for me, but then we should additionally merge #52332 into 5.4, right?

@derrabus
Copy link
Member

Fine for me, but then we should additionally merge #52332 into 5.4, right?

Let's merge #52332 first, merge it up and then undo it in this PR.

@fabpot
Copy link
Member

fabpot commented Oct 28, 2023

Thank you @GromNaN.

@fabpot fabpot merged commit bb5d49c into symfony:6.4 Oct 28, 2023
6 of 9 checks passed
@fabpot
Copy link
Member

fabpot commented Oct 28, 2023

Revert done in eaff34a

@GromNaN GromNaN deleted the yaml-parse-null branch October 29, 2023 09:49
@GromNaN
Copy link
Member Author

GromNaN commented Oct 29, 2023

Thank you @fabpot.
The test must also be deleted. #52353

xabbuh added a commit that referenced this pull request Oct 29, 2023
This PR was merged into the 6.4 branch.

Discussion
----------

[Yaml] Remove test on `Inline::parse(null)`

| Q             | A
| ------------- | ---
| Branch?       | 6.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Issues        | Fix #52334
| License       | MIT

This test was added by #52332
It becomes wrong with #52334
It was not reverted by eaff34a

Commits
-------

9b10106 Fix wrong yaml parse null test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

symfony/yaml PHP 8.1 Compatibility
5 participants