Skip to content

[VarDumper] Add support for virtual properties #57833

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
Aug 22, 2024
Merged

[VarDumper] Add support for virtual properties #57833

merged 1 commit into from
Aug 22, 2024

Conversation

alexandre-daubois
Copy link
Member

@alexandre-daubois alexandre-daubois commented Jul 25, 2024 •
edited
Loading

Q A
Branch? 7.2
Bug fix? no
New feature? yes
Deprecations? no
Issues -
License MIT

This brings support for hooked virtual properties. Given the following code:

class VirtualHookedClass
{
    public string $firstName = 'John';
    private string $lastName = 'Doe';

    public string $fullName {
        get {
            return $this->firstName . ' ' . $this->lastName;
        }
    }

    private $untypedFullName {
        get {
            return $this->firstName . ' ' . $this->lastName;
        }
    }
}

dd(new VirtualHookedClass());

The following is dumped:

image

Currently, it dumps the property as an uninitialized one, as +fullName: ? string. I think it would be nice to know that more than being uninitialized, it's actually a virtual property.

Given that hooks contain arbitrary code, I'm not sure it would be a good idea to try to display the resulting value in the dump of the virtual property.

@carsonbot carsonbot added this to the 7.2 milestone Jul 25, 2024
@alexandre-daubois alexandre-daubois force-pushed the virtual-hooked-props branch 3 times, most recently from cc24170 to 92dadd7 Compare July 26, 2024 11:41
@alexandre-daubois alexandre-daubois force-pushed the virtual-hooked-props branch 2 times, most recently from 5f83426 to 1820d2b Compare July 29, 2024 07:23
@alexandre-daubois alexandre-daubois changed the title [VarDumper] Add support for dumping virtual hooked properties [VarDumper] Add support for dumping virtual properties Jul 29, 2024
nicolas-grekas
nicolas-grekas reviewed Aug 13, 2024
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.

If we want to be more consistent, virtual properties should be displayed using visual hints, not explicit wordings.
I suggest using italic for virtual property names.
Then, for the value, I agree not calling the hook. I'd just make this closer to how UndefinedStub works (same without the ? prefix, and with some placeholder for the no-type case)
I'd also name the stub VirtualStub (same naming convention as UninitializedStub.
WDYT?

@alexandre-daubois
Copy link
Member Author

alexandre-daubois commented Aug 13, 2024 •
edited
Loading

I agree with you! I wasn't a big fan of the current render. I updated the description with this screenshot, looks nice!

image

Not sure of (no type), but I couldn't come up with another wording for now

@alexandre-daubois alexandre-daubois force-pushed the virtual-hooked-props branch 4 times, most recently from 686eb84 to 4f744cc Compare August 13, 2024 20:18
nicolas-grekas
nicolas-grekas reviewed Aug 20, 2024
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.

I fixed the implementation.
Can you please have a look at tests, and add some for html?

@alexandre-daubois
Copy link
Member Author

alexandre-daubois commented Aug 20, 2024 •
edited
Loading

I pushed a revision containing the following changes (see https://github.com/symfony/symfony/compare/f73c776c1ad8add7b96f1ca557e7b2ff5d43b144..68c9ae6d3c1bd6f03fa185e144ae6e5f31fedafe):

  • Check for color support to display italic
  • Use ~ to identify a virtual property (for the record, this character is already used with Caster::PREFIX_VIRTUAL)
  • Added a few tests for CliDumper and HtmlDumper
  • Description updated 🖼️

Thank you for helping me fix the implementation!

@alexandre-daubois alexandre-daubois force-pushed the virtual-hooked-props branch 2 times, most recently from 50e1537 to b185923 Compare August 20, 2024 15:38
@alexandre-daubois alexandre-daubois changed the title [VarDumper] Add support for dumping virtual properties [VarDumper] Add support for virtual properties Aug 21, 2024
@alexandre-daubois alexandre-daubois force-pushed the virtual-hooked-props branch 2 times, most recently from 317f8a4 to 5169f22 Compare August 21, 2024 19:58
@nicolas-grekas
Copy link
Member

Thank you @alexandre-daubois.

@nicolas-grekas nicolas-grekas merged commit 4daa12b into symfony:7.2 Aug 22, 2024
5 of 10 checks passed
@alexandre-daubois alexandre-daubois deleted the virtual-hooked-props branch August 22, 2024 07:38
@fabpot fabpot mentioned this pull request Oct 27, 2024
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

@OskarStark OskarStark OskarStark left review comments

@derrabus derrabus derrabus approved these changes

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

Assignees
No one assigned
Labels
Feature Status: Reviewed VarDumper
Projects
None yet
Milestone
7.2
Development

Successfully merging this pull request may close these issues.
6 participants
@alexandre-daubois @nicolas-grekas @stof @OskarStark @derrabus @carsonbot