Skip to content
This repository has been archived by the owner on Dec 3, 2023. It is now read-only.

add example with constant as key #4441

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

tacman
Copy link
Contributor

@tacman tacman commented Oct 18, 2022

This is a shorter example. I think the issue is using a constant as the key, or maybe it's using a class constant rather than a PHP constant. Likely this test can be merged with constant.yaml.

Addresses #4436

@TomasVotruba
Copy link
Member

TomasVotruba commented Oct 18, 2022

This looks much better 👏

tacman and others added 2 commits October 25, 2022 07:49
@alexander-schranz
Copy link
Contributor

alexander-schranz commented Nov 23, 2022

I shortly analysed the problem the related code lines are:

Sadly I was not yet able to find a way to support all cases. Maybe someone else with better regex skill can make this work :)

A Regex 101 link: https://regex101.com/r/gz7K1V/1

Expected:

parameters:
     class_constant: %const(Symplify\ConfigTransformer\Tests\Converter\ConfigFormatConverter\YamlToPhp\YamlToPhpTest::TEST)
     class: %const(Symplify\ConfigTransformer\Tests\Converter\ConfigFormatConverter\YamlToPhp\YamlToPhpTest::class)
     class2: %const(Symplify\ConfigTransformer\Tests\Converter\ConfigFormatConverter\YamlToPhp\YamlToPhpTest::class)
     const: %const(PHP_INT_MAX)
     const2: %const(PHP_INT_MAX)
     unexisting_constant: %const(SomeClass::Constant)
-    %const(PHP_INT_MAX: some_value)
-    %const(App\Entity\Plant::TRANSITION_PLANT: some_value)
-    %const(App\Entity\Plant::TRANSITION_TWO: some_value)
+    %const(PHP_INT_MAX): some_value
+    %const(App\Entity\Plant::TRANSITION_PLANT): some_value
+    %const(App\Entity\Plant::TRANSITION_TWO): some_value

@Wirone
Copy link
Contributor

Wirone commented Nov 24, 2022

Check this one 😉

image

@tacman
Copy link
Contributor Author

tacman commented Nov 24, 2022 via email

@alexander-schranz
Copy link
Contributor

@tacman Thx for th ehint, for the upgrade process to convert both need to be supported if somebody uses older version :) at the end it should both be converted to:

->set('transitions', [OurConstant::class => 'value'])

@alexander-schranz
Copy link
Contributor

@Wirone Nice, thank you 👍 do you want to create the pull request, don't want to steal your contribution :)

@Wirone
Copy link
Contributor

Wirone commented Nov 24, 2022

@alexander-schranz do I understand correctly that such PR should provide changes in 2 files that you mentioned earlier (plus 3rd revision of the regex)?

@alexander-schranz
Copy link
Contributor

@Wirone the lines are in the same file. The regex need to be adopted and the lines where the regex it is used. And we should adopt existing tests or use the one provided here :) I sadly did not test it yet out if there are more changes required, but I don't think it.

@Wirone
Copy link
Contributor

Wirone commented Nov 25, 2022

FYI: I did not add fixtures for constants use as keys, so @tacman can do it, but I suggest to merge #4484 first and then add such examples to packages/config-transformer/tests/Converter/ConfigFormatConverter/YamlToPhp/Fixture/normal/constant.yaml and packages/config-transformer/tests/Converter/ConfigFormatConverter/YamlToPhp/Fixture/normal/class_constant.yaml.

But be aware, I tested it and I got:
image

😅

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants