8000 [VarDumper] Adds support for ReflectionUnionType to VarDumper by michaeldnelson · Pull Request #40453 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[VarDumper] Adds support for ReflectionUnionType to VarDumper #40453

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
Mar 17, 2021
Merged

[VarDumper] Adds support for ReflectionUnionType to VarDumper #40453

merged 1 commit into from
Mar 17, 2021

Conversation

michaeldnelson
Copy link
@michaeldnelson michaeldnelson commented Mar 12, 2021 •
edited
Loading

Fixes a bug with VarDumper when dumping a ReflectionUnionType.

PHP Error: Call to undefined method ReflectionUnionType::isBuiltin()

Notes:

  • One of the existing tests relies on its position in the test file. I had to modify its expected line number.
  • There is an existing trailing space around line 367 in an expected value.
    I'm not sure if this was left for BC reasons but it seems like a bug if the dumper is leaving trailing spaces.
Q A
Branch? 4.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets
License MIT
Doc PR

This commit fixes a crash when dumping ReflectionUnionType. The code is minimal but uses a few extra lines to preserve key order for bc and consistency. Additionally, there is an else condition that is currently unreachable but is defensive should they add additional subtypes of ReflectionType. Tests are included. Please let me know if you have any questions or suggestions. Thanks for Symfony it's a wonderful project.

@carsonbot
Copy link

Hey!

I see that this is your first PR. That is great! Welcome!

Symfony has a contribution guide which I suggest you to read.

In short:

  • Always add tests
  • Keep backward compatibility (see https://symfony.com/bc).
  • Bug fixes must be submitted against the lowest maintained branch where they apply (see https://symfony.com/releases)
  • Features and deprecations must be submitted against the 5.x branch.

Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change.

When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor!
If this PR is merged in a lower version branch, it will be merged up to all maintained branches within a few days.

I am going to sit back now and wait for the reviews.

Cheers!

Carsonbot

@michaeldnelson
Copy link
Author
michaeldnelson commented Mar 12, 2021 •
edited
Loading

Looking at ci It makes sense tests would break using php < 8. I'm not quite sure the best way to add this without breaking and I don't have multiple php versions running at the moment. I'm happy to take and suggestions about how to test this feature without breaking php < 8. Two possibilities I can think of are multiple classes or a single class with a conditional as below, either used in conjunction with the @require php8 annotation. I'm not sure if the parser lints a file on include, if that is the case I have no idea how to resolve this.

ReflectionPropertyFixture.php

if (\PHP_VERSION_ID >= 80000) {
    class ReflectionPropertyFixture
    {
        public int $a;
        public int|string $b;
    }
} else {
    class ReflectionPropertyFixture
    {
        public int $a;
    }
}

@derrabus derrabus added this to the 5.x milestone Mar 12, 2021
@derrabus
Copy link
Member

Thank you for working on this bugfix.

This commit fixes a crash when dumping ReflectionUnionType. This targets 5.2 due to it being a bugfix that only impacts PHP 8.

Your pull request targets 5.x right now which is probably mot what you wanted. If you can reproduce the crash on 4.4 as well (and I’m pretty sure that this is the case), then we should target 4.4 instead.

Thanks for Symfony it's a wonderful project.

❤️

derrabus
derrabus requested changes Mar 12, 2021
View reviewed changes
@michaeldnelson michaeldnelson force-pushed the reflection_union_type_fix branch from 60f82a0 to 2cf92fc Compare March 13, 2021 02:22
@michaeldnelson michaeldnelson changed the base branch from 5.x to 4.4 March 13, 2021 02:29
@michaeldnelson
Copy link
Author
michaeldnelson commented Mar 13, 2021 •
edited
Loading

@derrabus I've rebased onto 4.4 and resolved the php7 issues as suggested. CI is stuck due to a perceived conflict before I switched the target branch of the PR so I am unable to verify it is passing. Please let me know if there are any further issues, and thanks again.

@michaeldnelson michaeldnelson requested a review from derrabus March 13, 2021 02:47
derrabus
derrabus requested changes Mar 13, 2021
View reviewed changes
@michaeldnelson michaeldnelson force-pushed the reflection_union_type_fix branch 2 times, most recently from ab4cd83 to 8a0bdee Compare March 13, 2021 18:33
@michaeldnelson michaeldnelson force-pushed the reflection_union_type_fix branch 4 times, most recently from d3e73af to 8692abd Compare March 13, 2021 22:22
@michaeldnelson michaeldnelson requested a review from derrabus March 13, 2021 22:30
@michaeldnelson michaeldnelson force-pushed the reflection_union_type_fix branch 3 times, most recently from 39d4b45 to bb79b64 Compare March 13, 2021 22:44
nicolas-grekas
nicolas-grekas approved these changes Mar 16, 2021
View reviewed changes
Copy link
Member
@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with minor comments, thanks.

derrabus
derrabus approved these changes Mar 16, 2021
View reviewed changes
Copy link
Member
@derrabus derrabus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏻 (after Nicolas' comments have been addressed)

@michaeldnelson michaeldnelson force-pushed the reflection_union_type_fix branch from bb79b64 to 6dc7cac Compare March 17, 2021 00:11
@derrabus derrabus modified the milestones: 5.x, 4.4 Mar 17, 2021
@michaeldnelson michaeldnelson force-pushed the reflection_union_type_fix branch from 6dc7cac to ccbf5d3 Compare March 17, 2021 00:20
@fabpot fabpot force-pushed the reflection_union_type_fix branch from 46bd481 to 1a11491 Compare March 17, 2021 06:30
@fabpot
Copy link
Member
fabpot commented Mar 17, 2021

Thank you @michaeldnelson.

@fabpot fabpot merged commit 550489a into symfony:4.4 Mar 17, 2021
This was referenced Mar 29, 2021
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stof stof stof left review comments

@derrabus derrabus derrabus approved these changes

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

Assignees
No one assigned
Labels
Bug Status: Reviewed VarDumper
Projects
None yet
Milestone
4.4
Development

Successfully merging this pull request may close these issues.
7 participants
@michaeldnelson @carsonbot @derrabus @fabpot @nicolas-grekas @stof @mndevel
0