[TwigComponent] Add new ComponentReflection to extract properties from TwigComponent #2498

Halleck45 · 2025-01-10T10:03:09Z

Q	A
Bug fix?	no
New feature?	yes
Issues
License	MIT

Hello,

I'm currently trying to build a tool that analyzes TwigComponent to document (automatically) the components of a project. A bit like what StoryBook does, but for Twig.

For this, I need to retrieve the properties of a component. The TwigComponentDebugCommand command already does this. This PR extracts the relevant code, and moves it to the new ComponentPropertiesExtractor class.

A BC is introduced: I need to inject the ComponentPropertiesExtractor class into the TwigComponentDebugCommand, but the last argument of the constructor is optional. This is normally not a problem: the class is internal
~~If necessary, I can keep backward compatibility by building the object directly in the code (rather than injecting it), but in the long run this may introduce testability issues.~~
Edit: fixed thanks to @Kocal suggestion

Thank you for your feedback!

Kocal

Thanks for the feature, that's really interesting and could be really useful for some use cases, and we have new tests! :D

Just a minor comment about the BC break.

cc @smnandre

src/TwigComponent/src/Command/TwigComponentDebugCommand.php

src/TwigComponent/src/DependencyInjection/TwigComponentExtension.php

…omponent

Halleck45 · 2025-01-10T11:28:05Z

Thanks for the feature, that's really interesting and could be really useful for some use cases, and we have new tests! :D

Just a minor comment about the BC break.

@Kocal I'm glad you like it! 😊 Thanks for the feedback. The BC break is now fixed and pushed!

smnandre · 2025-01-10T11:30:34Z

Hi! I need to digg deeper in the code (later today), but already a big thank you for opening this and contributing!

First reaction is: some code here is redondant with the ComponentProperties class.

I think we should differenciate here the extraction of props from templates, and props from classes.

Making ComponentProperties call different sources (maybe more "Loaders" than "extractors" in the end).

And starting after that building the ComponentMetadataFactory / ies we need.

smnandre · 2025-01-10T11:31:51Z

src/TwigComponent/CHANGELOG.md

@@ -1,5 +1,9 @@
 # CHANGELOG

+## 2.23.0
+
+- Add `ComponentPropertiesExtractor` to extract component properties from a Twig component


I don't think we want to expose this class (with BC promise) for something like this. At least in a first step.

Hi, BC is resolved now. Do you still want me to remove this change?

No, my point is i'm not sure we should make this class "public" for now. Because once it's done, we cannot go back.

And, as i said in the previous message, we need to clarifiy first whose class has which responsibility, the contracts, etc.

src/TwigComponent/src/ComponentPropertiesExtractor.php

smnandre · 2025-01-10T11:34:21Z

src/TwigComponent/src/ComponentPropertiesExtractor.php

+    /**
+     * @return array<string, string>
+     */


Not the good type (you return an array of string => array{name: display: type: ...} for no)

smnandre · 2025-01-10T11:36:38Z

I started this work here : 2.x...smnandre:ux:dev/template-properties

WebMamba · 2025-01-12T15:06:14Z

Hey there 👋 Thanks @Halleck45 for your PR! I am interested about your debugging tool! Why not using : https://packagist.org/packages/sensiolabs/storybook-bundle ? What your debugging tool gonna do ? 😁

WebMamba

I thinks that it's a good idea to move the logic to extract property information into a dedicated service, and we should do the same for the service to find the components. But I thinks all of that should not be separate from the core. I don't have any solution yet 😅

smnandre · 2025-01-14T09:11:40Z

Any thought about my main points @Halleck45 ? (ComponentProperties / ComponentPropertyExtractor and the metadata thing)

smnandre · 2025-01-15T17:44:58Z

Hmm that does not feel entirely right to me right now.. :|

And i understand you need this for something. But we cannot introduce a class that, at least for a part, does what is already done by another, with cache and preloading instead of runtime.

So i'm very sorry for this, but yeah it's probably a safer choice in these conditions to go back at your original proposal.

The problems of code duplication and maintaining this class afterwards being still here in both case, i'd suggest the following:

move it into a Debug folder (or something like that in the name, you know, to clearly identify it) .... (and probably not include it when not APP_DEBUG=true, or do i miss something here?)
but most importantly i'd set this class as experimental, allowing us to revert this change if we roll out a clean Metadata Factory/Loader before the end of the year.

I know this seems not much, but you cannot imagine the time i'm spending on debug/maintenance on tiny things like this, and I really would love to spend more of my time on new things :)

Halleck45 · 2025-01-16T09:12:03Z

Hmm that does not feel entirely right to me right now.. :|

No problem, I just reverted the last commit.

I'll keep the branch handy anyway, as I think it would be cleaner to have strong typing on what's being extracted.

Kocal

Some minor comments, and I think we will be ready to merge it :)

src/TwigComponent/src/ComponentPropertiesExtractor.php

Kocal · 2025-01-16T20:29:41Z

src/TwigComponent/src/ComponentPropertiesExtractor.php

+    }
+
+    /**
+     * @return array<array{display: string, name: string, type: string, default: mixed}>


We can be a little bit more precise by setting string as the array key:

Suggested change

* @return array<array{display: string, name: string, type: string, default: mixed}>

* @return array<string, array{display: string, name: string, type: string, default: mixed}>

src/TwigComponent/src/ComponentPropertiesExtractor.php

src/TwigComponent/CHANGELOG.md

Co-authored-by: Hugo Alliaume <[email protected]>

Co-authored-by: Simon André <[email protected]>

src/TwigComponent/src/ComponentPropertiesExtractor.php

smnandre · 2025-01-21T05:00:30Z

src/TwigComponent/src/ComponentPropertiesExtractor.php

+                $propertyDisplay = $typeName.' $'.$propertyName.(null !== $value ? ' = '.json_encode(
+                    $value
+                ) : '');


Suggested change

$propertyDisplay = $typeName.' $'.$propertyName.(null !== $value ? ' = '.json_encode(

$value

) : '');

$propertyDisplay = $typeName.' $'.$propertyName.(null !== $value ? ' = '.json_encode($value) : '');

smnandre · 2025-01-21T05:03:13Z

src/TwigComponent/src/ComponentPropertiesExtractor.php

+    /**
+     * Extract properties from {% props %} tag in anonymous template.
+     *
+     * @return array<array{display: string, name: string, type: string, default: mixed}>


Suggested change

* @return array<array{display: string, name: string, type: string, default: mixed}>

* @return array<array{display: string, name: string, type: string, default: ?string}>

smnandre · 2025-01-21T05:04:20Z

src/TwigComponent/src/ComponentPropertiesExtractor.php

+                    'name' => $propertyName,
+                    'display' => $propertyDisplay,
+                    'type' => $typeName,
+                    'default' => $value,


Should be json encoded as the other one

smnandre · 2025-01-21T05:04:48Z

src/TwigComponent/src/ComponentPropertiesExtractor.php

+    }
+
+    /**
+     * @return array<string, array{display: string, name: string, type: string, default: mixed}>


Suggested change

* @return array<string, array{display: string, name: string, type: string, default: mixed}>

* @return array<string, array{display: string, name: string, type: string, default: ?string}>

(once fixed the value json encoding)

smnandre · 2025-01-21T05:07:35Z

src/TwigComponent/src/ComponentPropertiesExtractor.php

+    /**
+     * @return array<string, array{display: string, name: string, type: string, default: mixed}>
+     */
+    private function getNonAnonymousComponentProperties(ComponentMetadata $metadata): array


Let's avoid double negation if possible. Anonymous component are a special type of components.

But "non anonymous components" are just "components".

src/TwigComponent/src/Command/TwigComponentDebugCommand.php

smnandre · 2025-01-21T05:11:40Z

src/TwigComponent/src/Command/TwigComponentDebugCommand.php

+        $propertiesAsArrayOfStrings = array_filter(array_map(
+            fn (array $property) => $property['display'],
+            $properties,
+        ));


Suggested change

$propertiesAsArrayOfStrings = array_filter(array_map(

fn (array $property) => $property['display'],

$properties,

));

$properties = array_column($properties, 'display');

1/3

smnandre · 2025-01-21T05:11:59Z

src/TwigComponent/src/Command/TwigComponentDebugCommand.php

        // Anonymous Component
        if ($metadata->isAnonymous()) {
            $table->addRows([
                ['Type', '<comment>Anonymous</comment>'],
                new TableSeparator(),
-                ['Properties', implode("\n", $this->getAnonymousComponentProperties($metadata))],
+                ['Properties', implode("\n", $propertiesAsArrayOfStrings)],


Suggested change

['Properties', implode("\n", $propertiesAsArrayOfStrings)],

['Properties', implode("\n", $properties)],

2/3

smnandre · 2025-01-21T05:12:19Z

src/TwigComponent/src/Command/TwigComponentDebugCommand.php

@@ -229,7 +237,7 @@ private function displayComponentDetails(SymfonyStyle $io, string $name): void
            new TableSeparator(),
            // ['Attributes Var', $metadata->get('attributes_var')],
            ['Public Props', $metadata->isPublicPropsExposed() ? 'Yes' : 'No'],
-            ['Properties', implode("\n", $this->getComponentProperties($metadata))],
+            ['Properties', implode("\n", $propertiesAsArrayOfStrings)],


Suggested change

['Properties', implode("\n", $propertiesAsArrayOfStrings)],

['Properties', implode("\n", $properties)],

3/3

smnandre · 2025-01-21T20:03:48Z

(last comments and LGTM ;) )

WebMamba · 2025-01-23T14:12:18Z

Hey @Halleck45! Thanks for your interest, but I am not sure we gonna merge this PR. The reason is that I see the value of exposing such classes to your project, but I think there is less value for the Symfony UX project. We don't have any plan to use this code in another place than in the debug command; this is why I think we don't need to move the code to a dedicated class. Exposing such classes has cost (maintenance, BC, ...), so I don't think it gonna be valuable for us to have this class. Sorry about that, don't take it personally, we really appreciate having you here. @smnandre @Kocal WDYT ?

Kocal · 2025-01-23T14:24:29Z

I think it's fine to make it public but with the @experimental annotation, so we are free to not respect the BC promise, but the users must use it carefully.

We can also tag it as internal, with a warning message This class is not covered by the Symfony backward compatibility promise. like PHPUnit does.

Halleck45 · 2025-01-30T16:26:35Z

No worries about closing this PR. I think I can move forward in another way. Thanks for your feedback anyway!

@WebMamba, I couldn't get storybook-bundle to work, but it makes me want to dig deeper.

See you soon!

Halleck45 force-pushed the properties_extractor branch 5 times, most recently from 88c149a to 5eb4948 Compare January 10, 2025 10:43

Halleck45 changed the title ~~Add new ComponentPropertiesExtractor to extract properties from TwigComponent~~ [TwigComponent] Add new ComponentPropertiesExtractor to extract properties from TwigComponent Jan 10, 2025

Halleck45 force-pushed the properties_extractor branch from 5eb4948 to afc9f40 Compare January 10, 2025 10:52

Halleck45 marked this pull request as ready for review January 10, 2025 10:54

carsonbot added Feature New Feature TwigComponent Status: Needs Review Needs to be reviewed labels Jan 10, 2025

Kocal reviewed Jan 10, 2025

View reviewed changes

src/TwigComponent/src/Command/TwigComponentDebugCommand.php Outdated Show resolved Hide resolved

src/TwigComponent/src/DependencyInjection/TwigComponentExtension.php Outdated Show resolved Hide resolved

Add new ComponentPropertiesExtractor to extract properties from TwigC…

567d81a

…omponent

Halleck45 force-pushed the properties_extractor branch from afc9f40 to 567d81a Compare January 10, 2025 11:23

smnandre reviewed Jan 10, 2025

View reviewed changes

src/TwigComponent/src/ComponentPropertiesExtractor.php Outdated Show resolved Hide resolved

smnandre reviewed Jan 10, 2025

View reviewed changes

fixed incorrect phpdoc return type

091a2a3

smnandre added Status: Reviewing Review is ongoing, refining with author and removed Status: Needs Review Needs to be reviewed labels Jan 10, 2025

WebMamba reviewed Jan 12, 2025

View reviewed changes

Halleck45 requested a review from Kocal January 14, 2025 08:38

carsonbot added the Status: Needs Review Needs to be reviewed label Jan 14, 2025

Halleck45 requested review from smnandre and WebMamba January 14, 2025 08:38

Halleck45 changed the title ~~[TwigComponent] Add new ComponentPropertiesExtractor to extract properties from TwigComponent~~ [TwigComponent] Add new ComponentReflection to extract properties from TwigComponent Jan 15, 2025

Halleck45 force-pushed the properties_extractor branch from 111715c to 091a2a3 Compare January 16, 2025 09:10

Added headers

7d5973e

Kocal reviewed Jan 16, 2025

View reviewed changes

smnandre reviewed Jan 18, 2025

View reviewed changes

src/TwigComponent/src/ComponentPropertiesExtractor.php Outdated Show resolved Hide resolved

smnandre reviewed Jan 18, 2025

View reviewed changes

src/TwigComponent/CHANGELOG.md Outdated Show resolved Hide resolved

Halleck45 and others added 6 commits January 20, 2025 10:47

Update src/TwigComponent/src/ComponentPropertiesExtractor.php

e191828

Co-authored-by: Hugo Alliaume <[email protected]>

Update src/TwigComponent/src/ComponentPropertiesExtractor.php

342b65b

Co-authored-by: Hugo Alliaume <[email protected]>

Update src/TwigComponent/src/ComponentPropertiesExtractor.php

12bef21

Co-authored-by: Hugo Alliaume <[email protected]>

Update src/TwigComponent/src/ComponentPropertiesExtractor.php

ff1b904

Co-authored-by: Simon André <[email protected]>

Update src/TwigComponent/CHANGELOG.md

ef28179

Co-authored-by: Simon André <[email protected]>

applied suggestions about phpdoc and style

2034498

Kocal approved these changes Jan 20, 2025

View reviewed changes

smnandre reviewed Jan 21, 2025

View reviewed changes

src/TwigComponent/src/ComponentPropertiesExtractor.php Show resolved Hide resolved

smnandre reviewed Jan 21, 2025

View reviewed changes

src/TwigComponent/src/ComponentPropertiesExtractor.php Show resolved Hide resolved

smnandre reviewed Jan 21, 2025

View reviewed changes

src/TwigComponent/src/Command/TwigComponentDebugCommand.php Show resolved Hide resolved

smnandre reviewed Jan 21, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[TwigComponent] Add new ComponentReflection to extract properties from TwigComponent #2498

[TwigComponent] Add new ComponentReflection to extract properties from TwigComponent #2498

Halleck45 commented Jan 10, 2025 •

edited

Loading

Kocal left a comment

Halleck45 commented Jan 10, 2025

smnandre commented Jan 10, 2025

smnandre Jan 10, 2025

Halleck45 Jan 10, 2025

smnandre Jan 10, 2025

smnandre Jan 10, 2025

smnandre commented Jan 10, 2025

WebMamba commented Jan 12, 2025

WebMamba left a comment

smnandre commented Jan 14, 2025

smnandre commented Jan 15, 2025

Halleck45 commented Jan 16, 2025

Kocal left a comment

Kocal Jan 16, 2025

smnandre Jan 21, 2025

smnandre Jan 21, 2025

smnandre Jan 21, 2025

smnandre Jan 21, 2025

smnandre Jan 21, 2025

smnandre Jan 21, 2025

smnandre Jan 21, 2025

smnandre Jan 21, 2025

smnandre commented Jan 21, 2025

WebMamba commented Jan 23, 2025

Kocal commented Jan 23, 2025

Halleck45 commented Jan 30, 2025

	* @return array<array{display: string, name: string, type: string, default: mixed}>
	* @return array<string, array{display: string, name: string, type: string, default: mixed}>

	['Properties', implode("\n", $propertiesAsArrayOfStrings)],
	['Properties', implode("\n", $properties)],

[TwigComponent] Add new ComponentReflection to extract properties from TwigComponent #2498

Are you sure you want to change the base?

[TwigComponent] Add new ComponentReflection to extract properties from TwigComponent #2498

Conversation

Halleck45 commented Jan 10, 2025 • edited Loading

Kocal left a comment

Choose a reason for hiding this comment

Halleck45 commented Jan 10, 2025

smnandre commented Jan 10, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smnandre commented Jan 10, 2025

WebMamba commented Jan 12, 2025

WebMamba left a comment

Choose a reason for hiding this comment

smnandre commented Jan 14, 2025

smnandre commented Jan 15, 2025

Halleck45 commented Jan 16, 2025

Kocal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smnandre commented Jan 21, 2025

WebMamba commented Jan 23, 2025

Kocal commented Jan 23, 2025

Halleck45 commented Jan 30, 2025

Halleck45 commented Jan 10, 2025 •

edited

Loading